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 <[email protected]> > >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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
