| Message ID | 1503617885-32501-4-git-send-email-ktraynor@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Darrell Ball |
| Headers | show |
> 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
On 08/28/2017 02:28 PM, Ilya Maximets wrote: >> 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. > I agree the name is not intuitive now (or maybe ever). I will change to rxq_next_cycle_store to avoid confusion between it and the #define. >> >> /* 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. > Keeping names consistent with other patch. >> 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. > I don't want to reset it each time based on it being correct as is and reply to comment below. > 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. > I understand this, but we will use some data for the calculations. For existing queues it will either be the measured data or zero. On balance I choose to use the measured cycles. I don't think using zero for existing queues will make things better in general. >> + 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 > Yeah, semantically you could say this is not an optimization but storing data for a later optimization but I think it's ok as is. >> + >> if (now > pmd->next_optimization) { >> /* Try to obtain the flow lock to block out revalidator threads. >> -- >> 1.8.3.1 >
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; /* 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); 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++) { + 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; + } + if (now > pmd->next_optimization) { /* Try to obtain the flow lock to block out revalidator threads.
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@redhat.com> --- lib/dpif-netdev.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 71 insertions(+), 4 deletions(-)