> Count the cycles used for processing an rxq during the
> pmd rxq interval. As this is an in flight counter and
> pmds run independently, also store the total cycles used
> during the last full interval.
>
> Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
> ---
> lib/dpif-netdev.c | 75
> ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 71 insertions(+), 4 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 8731435..cdf2662 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -182,4 +182,8 @@ struct emc_cache {
> #define DPCLS_OPTIMIZATION_INTERVAL 1000
>
> +/* Time in ms of the interval in which rxq processing cycles used in
> + * rxq to pmd assignments is measured and stored. */
> +#define PMD_RXQ_INTERVAL_LEN 10000
> +
> /* Number of intervals for which cycles are stored
> * and used during rxq to pmd assignment. */
> @@ -570,4 +574,7 @@ struct dp_netdev_pmd_thread {
> /* Periodically sort subtable vectors according to hit frequencies */
> long long int next_optimization;
> + /* End of the next time interval for which processing cycles
> + are stored for each polled rxq. */
> + long long int rxq_interval;
This variable has a misleading name. It's *not* an interval but it's time
when next cycles will be stored. Also it looks like it has some relation
with PMD_RXQ_INTERVAL_MAX, but it's not.
>
> /* Statistics. */
> @@ -696,5 +703,16 @@ static void pmd_load_cached_ports(struct
> dp_netdev_pmd_thread *pmd)
> OVS_REQUIRES(pmd->port_mutex);
> static inline void
> -dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd);
> +dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd,
> + struct polled_queue *poll_list, int poll_cnt);
> +static void
> +dp_netdev_rxq_set_cycles(struct dp_netdev_rxq *rx,
> + enum rxq_cycles_counter_type type,
> + unsigned long long cycles);
> +static uint64_t
> +dp_netdev_rxq_get_cycles(struct dp_netdev_rxq *rx,
> + enum rxq_cycles_counter_type type);
> +static void
> +dp_netdev_rxq_set_intrvl_cycles(struct dp_netdev_rxq *rx,
> + unsigned long long cycles);
Same comment about shortened name.
> static void
> dpif_netdev_xps_revalidate_pmd(const struct dp_netdev_pmd_thread *pmd,
> @@ -3126,4 +3144,29 @@ cycles_count_intermediate(struct dp_netdev_pmd_thread
> *pmd,
> }
>
> +static void
> +dp_netdev_rxq_set_cycles(struct dp_netdev_rxq *rx,
> + enum rxq_cycles_counter_type type,
> + unsigned long long cycles)
> +{
> + atomic_store_relaxed(&rx->cycles[type], cycles);
> +}
> +
> +static uint64_t
> +dp_netdev_rxq_get_cycles(struct dp_netdev_rxq *rx,
> + enum rxq_cycles_counter_type type)
> +{
> + unsigned long long processing_cycles;
> + atomic_read_relaxed(&rx->cycles[type], &processing_cycles);
> + return processing_cycles;
> +}
> +
> +static void
> +dp_netdev_rxq_set_intrvl_cycles(struct dp_netdev_rxq *rx,
> + unsigned long long cycles)
> +{
> + atomic_store_relaxed(&rx->cycles_intrvl[rx->intrvl_idx++
> + % PMD_RXQ_INTERVAL_MAX], cycles);
> +}
> +
> static int
> dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread *pmd,
> @@ -3179,4 +3222,5 @@ port_reconfigure(struct dp_netdev_port *port)
> port->rxqs[i].rx = NULL;
> }
> + unsigned last_nrxq = port->n_rxq;
> port->n_rxq = 0;
>
> @@ -3199,4 +3243,12 @@ port_reconfigure(struct dp_netdev_port *port)
> for (i = 0; i < netdev_n_rxq(netdev); i++) {
> port->rxqs[i].port = port;
> + if (i >= last_nrxq) {
> + /* Only reset cycle stats for new queues */
> + dp_netdev_rxq_set_cycles(&port->rxqs[i], RXQ_CYCLES_PROC_CURR,
> 0);
> + dp_netdev_rxq_set_cycles(&port->rxqs[i], RXQ_CYCLES_PROC_HIST,
> 0);
> + for (unsigned j = 0; j < PMD_RXQ_INTERVAL_MAX; j++) {
rx->intrvl_idx isn't initialized at this point. I see, that the code will
work correctly with any starting value, but, IMHO, it's much more clear to
have explicit initialization.
Beside that, Why do you think that we need to initialize only new queues?
My concern is that if number of queues changed than the traffic pattern through
existing queues also will be changed significantly. This means that an old
statistics will not be relevant anymore. RSS may distribute traffic in a
completely different way after changing the number of queues.
> + dp_netdev_rxq_set_intrvl_cycles(&port->rxqs[i], 0);
> + }
> + }
> err = netdev_rxq_open(netdev, &port->rxqs[i].rx, i);
> if (err) {
> @@ -3879,5 +3931,5 @@ reload:
> dp_netdev_process_rxq_port(pmd, poll_list[i].rxq->rx,
> poll_list[i].port_no);
> - cycles_count_intermediate(pmd, NULL,
> + cycles_count_intermediate(pmd, poll_list[i].rxq,
> process_packets ? PMD_CYCLES_PROCESSING
> : PMD_CYCLES_IDLE);
> @@ -3890,5 +3942,5 @@ reload:
>
> coverage_try_clear();
> - dp_netdev_pmd_try_optimize(pmd);
> + dp_netdev_pmd_try_optimize(pmd, poll_list, poll_cnt);
> if (!ovsrcu_try_quiesce()) {
> emc_cache_slow_sweep(&pmd->flow_cache);
> @@ -4334,4 +4386,5 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread
> *pmd, struct dp_netdev *dp,
> cmap_init(&pmd->classifiers);
> pmd->next_optimization = time_msec() + DPCLS_OPTIMIZATION_INTERVAL;
> + pmd->rxq_interval = time_msec() + PMD_RXQ_INTERVAL_LEN;
> hmap_init(&pmd->poll_list);
> hmap_init(&pmd->tx_ports);
> @@ -5771,9 +5824,23 @@ dpcls_sort_subtable_vector(struct dpcls *cls)
>
> static inline void
> -dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd)
> +dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd,
> + struct polled_queue *poll_list, int poll_cnt)
> {
> struct dpcls *cls;
> long long int now = time_msec();
>
> + if (now > pmd->rxq_interval) {
> + /* Get the cycles that were used to process each queue and store. */
> + for (unsigned i = 0; i < poll_cnt; i++) {
> + uint64_t rxq_cyc_curr =
> dp_netdev_rxq_get_cycles(poll_list[i].rxq,
> +
> RXQ_CYCLES_PROC_CURR);
> + dp_netdev_rxq_set_intrvl_cycles(poll_list[i].rxq, rxq_cyc_curr);
> + dp_netdev_rxq_set_cycles(poll_list[i].rxq, RXQ_CYCLES_PROC_CURR,
> + 0);
> + }
> + /* Start new measuring interval */
> + pmd->rxq_interval = now + PMD_RXQ_INTERVAL_LEN;
> + }
IMHO, above code shouldn't be there. This function is for classifier
optimization.
Also, above code can't be called as "optimization", it just calculates some
statistics.
There should be different function for this and placed in more relevant part of
dpif-netdev.c
> +
> if (now > pmd->next_optimization) {
> /* Try to obtain the flow lock to block out revalidator threads.
> --
> 1.8.3.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
