Re: [ovs-dev] [PATCH 1/1] dpif-netdev: The pmd-*-show commands will show info in core order

[Ben Pfaff]

On Tue, Apr 25, 2017 at 05:12:49PM +0200, Eelco Chaudron wrote:
> On 15/04/17 06:33, Ben Pfaff wrote:
> >On Wed, Mar 22, 2017 at 11:10:23AM +0100, Eelco Chaudron wrote:
> >>The "ovs-appctl dpif-netdev/pmd-rxq-show" and "ovs-appctl
> >>dpif-netdev/pmd-stats-show" commands show their output per core_id,
> >>sorted on the hash location. My OCD was kicking in when using these
> >>commands, hence this change to display them in natural core_id order.
> >>
> >>In addition I had to change a test case that would fail if the cores
> >>where not in order in the hash list. This is due to OVS assigning
> >>queues to cores based on the order in the hash list. The test case now
> >>checks if any core has the set of queues in the given order.
> >>
> >>Manually tested this on my setup, and ran clang-analyze.
> >>
> >>Signed-off-by: Eelco Chaudron 
> >This should be helpful, thanks.
> >
> >sorted_poll_thread_list() worries me.  It calls cmap_count() and then
> >uses CMAP_FOR_EACH to iterate the list and assumes (asserts, even) that
> >the number of pmds does not change during iteration.  Is this safe?
> >If it is, then a clearer comment would be helpful, and it would be wise
> >to have an assertion for the exact number at the end.
> >
> >Thanks,
> >
> >Ben.
> 
> Hi Ben,
> 
> To be honest I copied it from a couple of functions above,
> sorted_poll_list(),
> without giving it the proper attention.
> 
> So when looking at it more in depth, I see it needs to be handled
> differently.
> Creation/deletion of the threads is protected with the dp->port_mutex,
> however
> the show commands are only protected by the dp_netdev_mutex. So in theory
> both
> activities can happen in parallel. So to avoid an assert, I guess just
> printing
> data in transition is safer.
> 
> I guess the following change should fix it:
> 
> CMAP_FOR_EACH (pmd, node, >poll_threads) {
> -   ovs_assert(k < n_pmds);
> +   if (k >= n_pmds) {
> +   break;
> +   }
> pmd_list[k++] = pmd;
> }
> 
> In addition I also changed the calling function, and made it safe in case
> the
> ltist shrunk:
> 
>   sorted_poll_thread_list(dp, _list, );
> for (i = 0; i < n; i++) {
> pmd = pmd_list[i];
> +   if (!pmd) {
> +   break;
> +   }
> 
> if (type == PMD_INFO_SHOW_RXQ) {
> 
> 
> Let me know if you want another patch?

Thank you for thinking it through, this makes more sense to me now.

Please do post a v2 when you are ready.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/1] dpif-netdev: The pmd-*-show commands will show info in core order

[Eelco Chaudron]

On 15/04/17 06:33, Ben Pfaff wrote:

On Wed, Mar 22, 2017 at 11:10:23AM +0100, Eelco Chaudron wrote:

The "ovs-appctl dpif-netdev/pmd-rxq-show" and "ovs-appctl
dpif-netdev/pmd-stats-show" commands show their output per core_id,
sorted on the hash location. My OCD was kicking in when using these
commands, hence this change to display them in natural core_id order.

In addition I had to change a test case that would fail if the cores
where not in order in the hash list. This is due to OVS assigning
queues to cores based on the order in the hash list. The test case now
checks if any core has the set of queues in the given order.

Manually tested this on my setup, and ran clang-analyze.

Signed-off-by: Eelco Chaudron 

This should be helpful, thanks.

sorted_poll_thread_list() worries me.  It calls cmap_count() and then
uses CMAP_FOR_EACH to iterate the list and assumes (asserts, even) that
the number of pmds does not change during iteration.  Is this safe?
If it is, then a clearer comment would be helpful, and it would be wise
to have an assertion for the exact number at the end.

Thanks,

Ben.


Hi Ben,

To be honest I copied it from a couple of functions above, 
sorted_poll_list(),

without giving it the proper attention.

So when looking at it more in depth, I see it needs to be handled 
differently.
Creation/deletion of the threads is protected with the dp->port_mutex, 
however
the show commands are only protected by the dp_netdev_mutex. So in 
theory both
activities can happen in parallel. So to avoid an assert, I guess just 
printing

data in transition is safer.

I guess the following change should fix it:

CMAP_FOR_EACH (pmd, node, >poll_threads) {
-   ovs_assert(k < n_pmds);
+   if (k >= n_pmds) {
+   break;
+   }
pmd_list[k++] = pmd;
}

In addition I also changed the calling function, and made it safe in 
case the

ltist shrunk:

  sorted_poll_thread_list(dp, _list, );
for (i = 0; i < n; i++) {
pmd = pmd_list[i];
+   if (!pmd) {
+   break;
+   }

if (type == PMD_INFO_SHOW_RXQ) {


Let me know if you want another patch?

Cheers,

Eelco


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/1] dpif-netdev: The pmd-*-show commands will show info in core order

[Ben Pfaff]

On Wed, Mar 22, 2017 at 11:10:23AM +0100, Eelco Chaudron wrote:
> The "ovs-appctl dpif-netdev/pmd-rxq-show" and "ovs-appctl
> dpif-netdev/pmd-stats-show" commands show their output per core_id,
> sorted on the hash location. My OCD was kicking in when using these
> commands, hence this change to display them in natural core_id order.
> 
> In addition I had to change a test case that would fail if the cores
> where not in order in the hash list. This is due to OVS assigning
> queues to cores based on the order in the hash list. The test case now
> checks if any core has the set of queues in the given order.
> 
> Manually tested this on my setup, and ran clang-analyze.
> 
> Signed-off-by: Eelco Chaudron 

This should be helpful, thanks.

sorted_poll_thread_list() worries me.  It calls cmap_count() and then
uses CMAP_FOR_EACH to iterate the list and assumes (asserts, even) that
the number of pmds does not change during iteration.  Is this safe?
If it is, then a clearer comment would be helpful, and it would be wise
to have an assertion for the exact number at the end.

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/1] dpif-netdev: The pmd-*-show commands will show info in core order

[Eelco Chaudron]

The "ovs-appctl dpif-netdev/pmd-rxq-show" and "ovs-appctl
dpif-netdev/pmd-stats-show" commands show their output per core_id,
sorted on the hash location. My OCD was kicking in when using these
commands, hence this change to display them in natural core_id order.

In addition I had to change a test case that would fail if the cores
where not in order in the hash list. This is due to OVS assigning
queues to cores based on the order in the hash list. The test case now
checks if any core has the set of queues in the given order.

Manually tested this on my setup, and ran clang-analyze.

Signed-off-by: Eelco Chaudron 
---
 lib/dpif-netdev.c | 50 +-
 tests/pmd.at  |  7 ---
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index a14a2eb..b44c03b 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -923,13 +923,57 @@ pmd_info_show_rxq(struct ds *reply, struct 
dp_netdev_pmd_thread *pmd)
 }
 }
 
+static int
+compare_poll_thread_list(const void *a_, const void *b_)
+{
+const struct dp_netdev_pmd_thread *a, *b;
+
+a = *(struct dp_netdev_pmd_thread **)a_;
+b = *(struct dp_netdev_pmd_thread **)b_;
+
+if (a->core_id < b->core_id) {
+return -1;
+}
+if (a->core_id > b->core_id) {
+return 1;
+}
+return 0;
+}
+
+/* Create a sorted list of pmd's from the dp->poll_threads cmap. We can use
+ * this list, as long as we do not got to quiescent state. */
+static void
+sorted_poll_thread_list(struct dp_netdev *dp,
+struct dp_netdev_pmd_thread ***list,
+size_t *n)
+{
+struct dp_netdev_pmd_thread *pmd;
+struct dp_netdev_pmd_thread **pmd_list;
+size_t k = 0, n_pmds;
+
+n_pmds = cmap_count(>poll_threads);
+pmd_list = xcalloc(n_pmds, sizeof *pmd_list);
+
+CMAP_FOR_EACH (pmd, node, >poll_threads) {
+ovs_assert(k < n_pmds);
+pmd_list[k++] = pmd;
+}
+
+qsort(pmd_list, k, sizeof *pmd_list, compare_poll_thread_list);
+
+*list = pmd_list;
+*n = k;
+}
+
 static void
 dpif_netdev_pmd_info(struct unixctl_conn *conn, int argc, const char *argv[],
  void *aux)
 {
 struct ds reply = DS_EMPTY_INITIALIZER;
 struct dp_netdev_pmd_thread *pmd;
+struct dp_netdev_pmd_thread **pmd_list;
 struct dp_netdev *dp = NULL;
+size_t i, n;
 enum pmd_info_type type = *(enum pmd_info_type *) aux;
 
 ovs_mutex_lock(_netdev_mutex);
@@ -948,7 +992,10 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int argc, 
const char *argv[],
 return;
 }
 
-CMAP_FOR_EACH (pmd, node, >poll_threads) {
+sorted_poll_thread_list(dp, _list, );
+for (i = 0; i < n; i++) {
+pmd = pmd_list[i];
+
 if (type == PMD_INFO_SHOW_RXQ) {
 pmd_info_show_rxq(, pmd);
 } else {
@@ -971,6 +1018,7 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int argc, 
const char *argv[],
 }
 }
 }
+free(pmd_list);
 
 ovs_mutex_unlock(_netdev_mutex);
 
diff --git a/tests/pmd.at b/tests/pmd.at
index 5686bed..7263d4b 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -53,6 +53,7 @@ m4_define([CHECK_PMD_THREADS_CREATED], [
 ])
 
 m4_define([SED_NUMA_CORE_PATTERN], ["s/\(numa_id \)[[0-9]]*\( core_id 
\)[[0-9]]*:/\1\2:/"])
+m4_define([SED_NUMA_CORE_QUEUE_PATTERN], ["s/\(numa_id \)[[0-9]]*\( core_id 
\)[[0-9]]*:/\1\2:/;s/\(queue-id: \)\(0 2 4 6\|1 3 5 
7\)/\1/"])
 m4_define([DUMMY_NUMA], [--dummy-numa="0,0,0,0"])
 
 AT_SETUP([PMD - creating a thread/add-port])
@@ -126,13 +127,13 @@ TMP=$(cat ovs-vswitchd.log | wc -l | tr -d [[:blank:]])
 AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:pmd-cpu-mask=0x3])
 CHECK_PMD_THREADS_CREATED([2], [], [+$TMP])
 
-AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN], 
[0], [dnl
+AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed 
SED_NUMA_CORE_QUEUE_PATTERN], [0], [dnl
 pmd thread numa_id  core_id :
isolated : false
-   port: p0queue-id: 0 2 4 6
+   port: p0queue-id: 
 pmd thread numa_id  core_id :
isolated : false
-   port: p0queue-id: 1 3 5 7
+   port: p0queue-id: 
 ])
 
 TMP=$(cat ovs-vswitchd.log | wc -l | tr -d [[:blank:]])
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev