On Wed, Oct 5, 2022 at 2:53 PM Kevin Traynor <[email protected]> wrote: > > pmd-rxq-show shows the Rx queue to pmd assignments as well as the > pmd usage of each Rx queue. > > Up until now a tail length of 60 seconds pmd usage was shown > for each Rx queue, as this is the value used during rebalance > to avoid any spike affects.
effects* > > When debugging or tuning, it is also convienent to display the convenient* > pmd usage of an Rx queue over a shorter time frame, so any changes > config or traffic that impact pmd usage can be evaulated more quickly. evaluated* > > A parameter is added that allows pmd-rxq-show stats pmd usage to > be shown for a shorter time frame. Values are rounded up to the > nearest 5 seconds as that is the measurement granularity and the value > used is displayed. e.g. > > $ ovs-appctl dpif-netdev/pmd-rxq-show -secs 5 > Displaying last 5 seconds pmd usage % > pmd thread numa_id 0 core_id 4: > isolated : false > port: dpdk0 queue-id: 0 (enabled) pmd usage: 95 % > overhead: 4 % > > The default time frame has not changed and the maximum value > is limited to the maximum stored tail length (60 seconds). > > Signed-off-by: Kevin Traynor <[email protected]> I was expecting the doc and test update as part of this patch. Not a big deal if you prefer it separate in patch 2/3. Overall, the series lgtm, I have some comments on this first patch, see below. > --- > lib/dpif-netdev-private-thread.h | 2 +- > lib/dpif-netdev.c | 91 ++++++++++++++++++++++++-------- > tests/pmd.at | 9 ++++ > 3 files changed, 80 insertions(+), 22 deletions(-) > > diff --git a/lib/dpif-netdev-private-thread.h > b/lib/dpif-netdev-private-thread.h > index 4472b199d..1ec3cd794 100644 > --- a/lib/dpif-netdev-private-thread.h > +++ b/lib/dpif-netdev-private-thread.h > @@ -115,5 +115,5 @@ struct dp_netdev_pmd_thread { > > /* Write index for 'busy_cycles_intrvl'. */ > - unsigned int intrvl_idx; > + atomic_count intrvl_idx; > /* Busy cycles in last PMD_INTERVAL_MAX intervals. */ > atomic_ullong *busy_cycles_intrvl; > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index a45b46014..a4e44b657 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -161,9 +161,11 @@ static struct odp_support dp_netdev_support = { > /* Time in microseconds of the interval in which rxq processing cycles used > * in rxq to pmd assignments is measured and stored. */ > -#define PMD_INTERVAL_LEN 10000000LL > +#define PMD_INTERVAL_LEN 5000000LL > +/* For converting PMD_INTERVAL_LEN to secs. */ > +#define INTERVAL_USEC_TO_SEC 1000000LL > > /* Number of intervals for which cycles are stored > * and used during rxq to pmd assignment. */ > -#define PMD_INTERVAL_MAX 6 > +#define PMD_INTERVAL_MAX 12 > > /* Time in microseconds to try RCU quiescing. */ > @@ -429,5 +431,5 @@ struct dp_netdev_rxq { > queue doesn't need to be pinned to > a > particular core. */ > - unsigned intrvl_idx; /* Write index for 'cycles_intrvl'. */ > + atomic_count intrvl_idx; /* Write index for 'cycles_intrvl'. */ > struct dp_netdev_pmd_thread *pmd; /* pmd thread that polls this queue. > */ > bool is_vhost; /* Is rxq of a vhost port. */ > @@ -617,4 +619,7 @@ dp_netdev_rxq_set_intrvl_cycles(struct dp_netdev_rxq *rx, > static uint64_t > dp_netdev_rxq_get_intrvl_cycles(struct dp_netdev_rxq *rx, unsigned idx); > +static uint64_t > +get_interval_values(atomic_ullong *source, atomic_count *cur_idx, > + int num_to_read); > static void > dpif_netdev_xps_revalidate_pmd(const struct dp_netdev_pmd_thread *pmd, > @@ -871,5 +876,6 @@ sorted_poll_list(struct dp_netdev_pmd_thread *pmd, struct > rxq_poll **list, > > static void > -pmd_info_show_rxq(struct ds *reply, struct dp_netdev_pmd_thread *pmd) > +pmd_info_show_rxq(struct ds *reply, struct dp_netdev_pmd_thread *pmd, > + int secs) > { > if (pmd->core_id != NON_PMD_CORE_ID) { > @@ -879,4 +885,5 @@ pmd_info_show_rxq(struct ds *reply, struct > dp_netdev_pmd_thread *pmd) > uint64_t busy_cycles = 0; > uint64_t total_rxq_proc_cycles = 0; > + unsigned int intervals; > > ds_put_format(reply, > @@ -890,13 +897,12 @@ pmd_info_show_rxq(struct ds *reply, struct > dp_netdev_pmd_thread *pmd) > /* Get the total pmd cycles for an interval. */ > atomic_read_relaxed(&pmd->intrvl_cycles, &total_cycles); > + /* Calculate how many intervals are to be used. */ > + intervals = DIV_ROUND_UP(secs, > + PMD_INTERVAL_LEN / INTERVAL_USEC_TO_SEC); > /* Estimate the cycles to cover all intervals. */ > - total_cycles *= PMD_INTERVAL_MAX; > - > - for (int j = 0; j < PMD_INTERVAL_MAX; j++) { > - uint64_t cycles; > - > - atomic_read_relaxed(&pmd->busy_cycles_intrvl[j], &cycles); > - busy_cycles += cycles; > - } > + total_cycles *= intervals; > + busy_cycles = get_interval_values(pmd->busy_cycles_intrvl, > + &pmd->intrvl_idx, > + intervals); > if (busy_cycles > total_cycles) { > busy_cycles = total_cycles; > @@ -908,7 +914,7 @@ pmd_info_show_rxq(struct ds *reply, struct > dp_netdev_pmd_thread *pmd) > uint64_t rxq_proc_cycles = 0; > > - for (int j = 0; j < PMD_INTERVAL_MAX; j++) { > - rxq_proc_cycles += dp_netdev_rxq_get_intrvl_cycles(rxq, j); > - } > + rxq_proc_cycles = get_interval_values(rxq->cycles_intrvl, > + &rxq->intrvl_idx, > + intervals); > total_rxq_proc_cycles += rxq_proc_cycles; > ds_put_format(reply, " port: %-16s queue-id: %2d", name, > @@ -1424,4 +1430,8 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int > argc, const char *argv[], > bool filter_on_pmd = false; > size_t n; > + unsigned int secs = 0; > + unsigned long long max_secs = (PMD_INTERVAL_LEN * PMD_INTERVAL_MAX) > + / INTERVAL_USEC_TO_SEC; > + bool first_show_rxq = true; > > ovs_mutex_lock(&dp_netdev_mutex); > @@ -1434,4 +1444,10 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int > argc, const char *argv[], > argc -= 2; > argv += 2; > + } else if (!strcmp(argv[1], "-secs") && argc > 2) { > + if (!str_to_uint(argv[2], 10, &secs)) { > + secs = max_secs; > + } > + argc -= 2; > + argv += 2; > } else { > dp = shash_find_data(&dp_netdevs, argv[1]); - Maybe add a type == PMD_INFO_SHOW_RXQ check before looking into argv[1] ? Otherwise, other commands like dpif-netdev/pmd-stats-show (for example) may parse a -secs option, but do nothing with it. - Additionnally, the pmd-rxq-show command registration needs some update. unixctl_command_register("dpif-netdev/pmd-rxq-show", "[-pmd core] [dp]", 0, 3, dpif_netdev_pmd_info, (void *)&poll_aux); * The usage() string does not describe the -secs new option. * Plus, I don't see why we can't pass both -pmd and -secs, so I would declare that this command can now take up to 5 params. > @@ -1463,5 +1479,16 @@ dpif_netdev_pmd_info(struct unixctl_conn *conn, int > argc, const char *argv[], > } > if (type == PMD_INFO_SHOW_RXQ) { > - pmd_info_show_rxq(&reply, pmd); > + if (first_show_rxq) { > + if (!secs || secs > max_secs) { > + secs = max_secs; > + } else { > + secs = ROUND_UP(secs, > + PMD_INTERVAL_LEN / INTERVAL_USEC_TO_SEC); > + } > + ds_put_format(&reply, "Displaying last %u seconds " > + "pmd usage %%\n", secs); > + first_show_rxq = false; Always displaying this banner regardless of a pmd matching would make this code smaller. Plus, secs can be computed once, before this per pmd loop. Wdyt? > + } > + pmd_info_show_rxq(&reply, pmd, secs); > } else if (type == PMD_INFO_CLEAR_STATS) { > pmd_perf_stats_clear(&pmd->perf_stats); > @@ -5151,5 +5178,5 @@ dp_netdev_rxq_set_intrvl_cycles(struct dp_netdev_rxq > *rx, > unsigned long long cycles) > { > - unsigned int idx = rx->intrvl_idx++ % PMD_INTERVAL_MAX; > + unsigned int idx = atomic_count_inc(&rx->intrvl_idx) % PMD_INTERVAL_MAX; > atomic_store_relaxed(&rx->cycles_intrvl[idx], cycles); > } > @@ -6892,4 +6919,7 @@ reload: > atomic_count_init(&pmd->pmd_overloaded, 0); > > + pmd->intrvl_tsc_prev = 0; > + atomic_store_relaxed(&pmd->intrvl_cycles, 0); > + > if (!dpdk_attached) { > dpdk_attached = dpdk_attach_thread(pmd->core_id); > @@ -6923,10 +6953,8 @@ reload: > } > > - pmd->intrvl_tsc_prev = 0; > - atomic_store_relaxed(&pmd->intrvl_cycles, 0); > for (i = 0; i < PMD_INTERVAL_MAX; i++) { > atomic_store_relaxed(&pmd->busy_cycles_intrvl[i], 0); > } > - pmd->intrvl_idx = 0; > + atomic_count_set(&pmd->intrvl_idx, 0); > cycles_counter_update(s); > > @@ -9911,5 +9939,5 @@ dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread > *pmd, > curr_tsc - pmd->intrvl_tsc_prev); > } > - idx = pmd->intrvl_idx++ % PMD_INTERVAL_MAX; > + idx = atomic_count_inc(&pmd->intrvl_idx) % PMD_INTERVAL_MAX; > atomic_store_relaxed(&pmd->busy_cycles_intrvl[idx], tot_proc); > pmd->intrvl_tsc_prev = curr_tsc; > @@ -9934,4 +9962,25 @@ dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread > *pmd, > } > > +/* Returns the sum of a specified number of newest to > + * oldest interval values. 'cur_idx' is where the next > + * write will be and wrap around needs to be handled. > + */ > +static uint64_t > +get_interval_values(atomic_ullong *source, atomic_count *cur_idx, > + int num_to_read) { > + unsigned int i; > + uint64_t total = 0; > + > + i = atomic_count_get(cur_idx) % PMD_INTERVAL_MAX; > + for (int read = 0; read < num_to_read; read++) { > + uint64_t interval_value; > + > + i = i ? i-1 : PMD_INTERVAL_MAX - 1; > + atomic_read_relaxed(&source[i], &interval_value); > + total += interval_value; > + } > + return total; > +} > + > /* Insert 'rule' into 'cls'. */ > static void > diff --git a/tests/pmd.at b/tests/pmd.at > index 10879a349..61682a79f 100644 > --- a/tests/pmd.at > +++ b/tests/pmd.at > @@ -71,4 +71,5 @@ CHECK_PMD_THREADS_CREATED() > > AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN], > [0], [dnl > +Displaying last 60 seconds pmd usage % > pmd thread numa_id <cleared> core_id <cleared>: > isolated : false > @@ -103,4 +104,5 @@ dummy@ovs-dummy: hit:0 missed:0 > > AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN], > [0], [dnl > +Displaying last 60 seconds pmd usage % > pmd thread numa_id <cleared> core_id <cleared>: > isolated : false > @@ -135,4 +137,5 @@ dummy@ovs-dummy: hit:0 missed:0 > > AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN], > [0], [dnl > +Displaying last 60 seconds pmd usage % > pmd thread numa_id <cleared> core_id <cleared>: > isolated : false > @@ -184,4 +187,5 @@ CHECK_PMD_THREADS_CREATED([1], [], [+$TMP]) > > AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN], > [0], [dnl > +Displaying last 60 seconds pmd usage % > pmd thread numa_id <cleared> core_id <cleared>: > isolated : false > @@ -216,4 +220,5 @@ dummy@ovs-dummy: hit:0 missed:0 > > AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show | sed SED_NUMA_CORE_PATTERN], > [0], [dnl > +Displaying last 60 seconds pmd usage % > pmd thread numa_id <cleared> core_id <cleared>: > isolated : false > @@ -281,4 +286,5 @@ OVS_WAIT_UNTIL([tail -n +$TMP ovs-vswitchd.log | grep > "Performing pmd to rx queu > > AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show], [0], [dnl > +Displaying last 60 seconds pmd usage % > pmd thread numa_id 1 core_id 1: > isolated : false > @@ -303,4 +309,5 @@ OVS_WAIT_UNTIL([tail -n +$TMP ovs-vswitchd.log | grep > "Performing pmd to rx queu > > AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show], [0], [dnl > +Displaying last 60 seconds pmd usage % > pmd thread numa_id 1 core_id 1: > isolated : false > @@ -323,4 +330,5 @@ OVS_WAIT_UNTIL([tail -n +$TMP ovs-vswitchd.log | grep > "Performing pmd to rx queu > > AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show], [0], [dnl > +Displaying last 60 seconds pmd usage % > pmd thread numa_id 1 core_id 1: > isolated : false > @@ -344,4 +352,5 @@ CHECK_PMD_THREADS_CREATED([1], [1], [+$TMP]) > > AT_CHECK([ovs-appctl dpif-netdev/pmd-rxq-show], [0], [dnl > +Displaying last 60 seconds pmd usage % > pmd thread numa_id 1 core_id 0: > isolated : false -- David Marchand _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
