[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 <echaudro at redhat.com>
> >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, &dp->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, &pmd_list, &n);
>     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.