On 20/06/2023 18:02, Kevin Traynor wrote:
On 14/06/2023 16:49, David Marchand wrote:On Wed, Jun 14, 2023 at 3:37 PM Kevin Traynor <ktray...@redhat.com> wrote:Max requested sleep time and status for a PMD thread is logged at start up or when changed, but it can be convenient to have a command to dump this information explicitly. It is envisaged that this will be expanded when future additions are added. Signed-off-by: Kevin Traynor <ktray...@redhat.com> --- Documentation/topics/dpdk/pmd.rst | 4 ++++ NEWS | 2 ++ lib/dpif-netdev.c | 40 +++++++++++++++++++++++++++---- tests/pmd.at | 29 ++++++++++++++++++++++ 4 files changed, 71 insertions(+), 4 deletions(-) diff --git a/Documentation/topics/dpdk/pmd.rst b/Documentation/topics/dpdk/pmd.rst index b261e9254..bedd42194 100644 --- a/Documentation/topics/dpdk/pmd.rst +++ b/Documentation/topics/dpdk/pmd.rst @@ -349,4 +349,8 @@ time not processing packets will be determined by the sleep and processor wake-up times and should be tested with each system configuration. +The max sleep time that a PMD thread may request can be shown with:: + + $ ovs-appctl dpif-netdev/pmd-sleep-show + Sleep time statistics for 10 secs can be seen with:: diff --git a/NEWS b/NEWS index 1ccc6f948..32b9e8167 100644 --- a/NEWS +++ b/NEWS @@ -39,4 +39,6 @@ Post-v3.1.0 - Userspace datapath: * Rename 'pmd-maxsleep' other_config to 'pmd-sleep-max'. + * Add 'ovs-appctl dpif-netdev/pmd-sleep-show' command to get the + max sleep request setting of PMD thread cores. diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 94c8ca943..dadf17b70 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -702,4 +702,5 @@ enum pmd_info_type { PMD_INFO_SHOW_RXQ, /* Show poll lists of pmd threads. */ PMD_INFO_PERF_SHOW, /* Show pmd performance details. */ + PMD_INFO_MAX_SLEEP_SHOW, /* Show max sleep performance details. */ }; @@ -884,4 +885,18 @@ sorted_poll_list(struct dp_netdev_pmd_thread *pmd, struct rxq_poll **list, } +static void +pmd_max_sleep_show(struct ds *reply, struct dp_netdev_pmd_thread *pmd, + uint64_t default_max_sleep) +{ + if (pmd->core_id != NON_PMD_CORE_ID) { + ds_put_format(reply, + "PMD thread core %3u NUMA %2d: " + "Max sleep request set to", + pmd->core_id, pmd->numa_id); + ds_put_format(reply, " %4"PRIu64" usecs.", default_max_sleep); + ds_put_cstr(reply, "\n"); + } +} +Nit: I would move this new function later, and let pmd_info_show_rxq() and sorted_poll_list() close to each other.Will add to v2. I placed it after the other dpif_netdev_pmd_info() related functions.static void pmd_info_show_rxq(struct ds *reply, struct dp_netdev_pmd_thread *pmd, @@ -1442,5 +1457,6 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int argc, const char *argv[], unsigned long long max_secs = (PMD_INTERVAL_LEN * PMD_INTERVAL_MAX) / INTERVAL_USEC_TO_SEC; - bool first_show_rxq = true; + bool first_pmd = true; + uint64_t default_max_sleep = 0;You can move this new variable in the only block that cares about it.Yes, I can do that if I remove the initialization. It will work now, but not being able to initialize feels like it makes the code a bit more brittle as that nuance could be missed if a default initialization was ever was needed in future (I don't have something in mind). What do you think? I can go with your preference.
Discussed with David offline. Working in practice, but it's not fully safe to rely that default_max_sleep will keep the read value after the loop if we move the declaration. So will stick with current declaration.
ovs_mutex_lock(&dp_netdev_mutex); @@ -1490,5 +1506,5 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int argc, const char *argv[], } if (type == PMD_INFO_SHOW_RXQ) { - if (first_show_rxq) { + if (first_pmd) { if (!secs || secs > max_secs) { secs = max_secs; @@ -1499,5 +1515,5 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int argc, const char *argv[], ds_put_format(&reply, "Displaying last %u seconds " "pmd usage %%\n", secs); - first_show_rxq = false; + first_pmd = false; } pmd_info_show_rxq(&reply, pmd, secs); @@ -1508,4 +1524,16 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int argc, const char *argv[], } else if (type == PMD_INFO_PERF_SHOW) { pmd_info_show_perf(&reply, pmd, (struct pmd_perf_params *)aux); + } else if (type == PMD_INFO_MAX_SLEEP_SHOW) { + if (first_pmd) { + atomic_read_relaxed(&dp->pmd_max_sleep, &default_max_sleep); + ds_put_format(&reply, "PMD max sleep request is %"PRIu64" " + "usecs.", default_max_sleep); + ds_put_cstr(&reply, "\n"); + ds_put_format(&reply, "PMD load based sleeps are %s.", + default_max_sleep ? "enabled" : "disabled"); + ds_put_cstr(&reply, "\n"); + first_pmd = false; + } + pmd_max_sleep_show(&reply, pmd, default_max_sleep); } } @@ -1608,5 +1636,6 @@ dpif_netdev_init(void) static enum pmd_info_type show_aux = PMD_INFO_SHOW_STATS, clear_aux = PMD_INFO_CLEAR_STATS, - poll_aux = PMD_INFO_SHOW_RXQ; + poll_aux = PMD_INFO_SHOW_RXQ, + sleep_aux = PMD_INFO_MAX_SLEEP_SHOW; unixctl_command_register("dpif-netdev/pmd-stats-show", "[-pmd core] [dp]", @@ -1620,4 +1649,7 @@ dpif_netdev_init(void) 0, 5, dpif_netdev_pmd_info, (void *)&poll_aux); + unixctl_command_register("dpif-netdev/pmd-sleep-show", "[-pmd core] [dp]", + 0, 3, dpif_netdev_pmd_info, + (void *)&sleep_aux); unixctl_command_register("dpif-netdev/pmd-perf-show", "[-nh] [-it iter-history-len]" diff --git a/tests/pmd.at b/tests/pmd.at index 64d8f6e2b..a158d0753 100644 --- a/tests/pmd.at +++ b/tests/pmd.at @@ -1278,8 +1278,17 @@ dnl Check default CHECK_DP_SLEEP_MAX([0], [disabled], []) +AT_CHECK([ovs-appctl dpif-netdev/pmd-sleep-show], [0], [dnl +PMD max sleep request is 0 usecs. +PMD load based sleeps are disabled. +]) +Functionnally, we are checking the same thing than CHECK_DP_SLEEP_MAX(). I would check the pmd-sleep-show command output in a single helper.That's true at this patch stage, but it changes with the next patch. In that case it would also be checking about pmd core ids/numas etc so i didn't think it made sense to add to the macro.
_______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
