| Message ID | 1503617885-32501-3-git-send-email-ktraynor@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Darrell Ball |
| Headers | show |
> Add counters to dp_netdev_rxq which will later be used for storing the > processing cycles of an rxq. Processing cycles will be stored in reference > to a defined time interval. We will store the cycles of the current in progress > interval, a number of completed intervals and the sum of the completed > intervals. > > cycles_count_intermediate was used to count cycles for a pmd. With some small > additions we can also use it to count the cycles used for processing an rxq. > > Signed-off-by: Kevin Traynor <ktraynor at redhat.com> > --- > lib/dpif-netdev.c | 30 +++++++++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index f35c079..8731435 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -182,4 +182,8 @@ struct emc_cache { > #define DPCLS_OPTIMIZATION_INTERVAL 1000 > > +/* Number of intervals for which cycles are stored > + * and used during rxq to pmd assignment. */ > +#define PMD_RXQ_INTERVAL_MAX 6 > + > struct dpcls { > struct cmap_node node; /* Within dp_netdev_pmd_thread.classifiers */ > @@ -340,4 +344,13 @@ enum pmd_cycles_counter_type { > }; > > +enum rxq_cycles_counter_type { > + RXQ_CYCLES_PROC_CURR, /* Cycles spent successfully polling and > + processing packets during the current > + interval. */ > + RXQ_CYCLES_PROC_HIST, /* Total cycles of all intervals that are used > + during rxq to pmd assignment. */ > + RXQ_N_CYCLES > +}; All patches wide: Multi-line comments should have a '*' on each line. > + > #define XPS_TIMEOUT_MS 500LL > > @@ -351,4 +364,11 @@ struct dp_netdev_rxq { > particular core. */ > struct dp_netdev_pmd_thread *pmd; /* pmd thread that polls this queue. */ > + > + /* Counters of cycles spent successfully polling and processing pkts. */ > + atomic_ullong cycles[RXQ_N_CYCLES]; > + /* We store PMD_RXQ_INTERVAL_MAX intervals of data for an rxq and then > + sum them to yield the cycles used for an rxq. */ > + atomic_ullong cycles_intrvl[PMD_RXQ_INTERVAL_MAX]; > + unsigned intrvl_idx; /* Write index for 'cycles_intrvl'. */ Does it matter to save 2 letters in a variable names? It looks ugly and unreadable. > }; > > @@ -677,5 +697,4 @@ static void pmd_load_cached_ports(struct dp_netdev_pmd_thread *pmd) > static inline void > dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd); > - Is it necessary to remove this line? It was here to split xps related functions from others. > static void > dpif_netdev_xps_revalidate_pmd(const struct dp_netdev_pmd_thread *pmd, > @@ -3092,4 +3111,5 @@ cycles_count_end(struct dp_netdev_pmd_thread *pmd, > static inline void > cycles_count_intermediate(struct dp_netdev_pmd_thread *pmd, > + struct dp_netdev_rxq *rxq, > enum pmd_cycles_counter_type type) > OVS_NO_THREAD_SAFETY_ANALYSIS > @@ -3100,4 +3120,8 @@ cycles_count_intermediate(struct dp_netdev_pmd_thread *pmd, > > non_atomic_ullong_add(&pmd->cycles.n[type], interval); > + if (rxq && (type == PMD_CYCLES_PROCESSING)) { > + /* Add to the amount of current processing cycles. */ > + non_atomic_ullong_add(&rxq->cycles[RXQ_CYCLES_PROC_CURR], interval); > + } > } > > @@ -3668,5 +3692,5 @@ dpif_netdev_run(struct dpif *dpif) > port->rxqs[i].rx, > port->port_no); > - cycles_count_intermediate(non_pmd, process_packets ? > + cycles_count_intermediate(non_pmd, NULL, process_packets ? > PMD_CYCLES_PROCESSING > : PMD_CYCLES_IDLE); It's not yours, but it'll be nice to fix here: According to coding style, '?' should be on the next line near to arguments. Also, IMHO, the whole construction should have the same level of indention. Like this: cycles_count_intermediate(non_pmd, NULL, process_packets ? PMD_CYCLES_PROCESSING : PMD_CYCLES_IDLE); Or this: cycles_count_intermediate( non_pmd, NULL, process_packets ? PMD_CYCLES_PROCESSING : PMD_CYCLES_IDLE); > @@ -3855,5 +3879,5 @@ reload: > dp_netdev_process_rxq_port(pmd, poll_list[i].rxq->rx, > poll_list[i].port_no); > - cycles_count_intermediate(pmd, > + cycles_count_intermediate(pmd, NULL, > process_packets ? PMD_CYCLES_PROCESSING > : PMD_CYCLES_IDLE); > -- > 1.8.3.1
Hi Ilya, These patches were already committed into the OVS-DPDK sub-tree that Darrell is maintaining after they were under review for a long time (4 months and 7 revisions). I will reply to each comment and address the comments around correctness by way of follow up patches. thanks, Kevin. On 08/28/2017 02:28 PM, Ilya Maximets wrote: >> Add counters to dp_netdev_rxq which will later be used for storing the >> processing cycles of an rxq. Processing cycles will be stored in reference >> to a defined time interval. We will store the cycles of the current in progress >> interval, a number of completed intervals and the sum of the completed >> intervals. >> >> cycles_count_intermediate was used to count cycles for a pmd. With some small >> additions we can also use it to count the cycles used for processing an rxq. >> >> Signed-off-by: Kevin Traynor <ktraynor at redhat.com> >> --- >> lib/dpif-netdev.c | 30 +++++++++++++++++++++++++++--- >> 1 file changed, 27 insertions(+), 3 deletions(-) >> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index f35c079..8731435 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -182,4 +182,8 @@ struct emc_cache { >> #define DPCLS_OPTIMIZATION_INTERVAL 1000 >> >> +/* Number of intervals for which cycles are stored >> + * and used during rxq to pmd assignment. */ >> +#define PMD_RXQ_INTERVAL_MAX 6 >> + >> struct dpcls { >> struct cmap_node node; /* Within dp_netdev_pmd_thread.classifiers */ >> @@ -340,4 +344,13 @@ enum pmd_cycles_counter_type { >> }; >> >> +enum rxq_cycles_counter_type { >> + RXQ_CYCLES_PROC_CURR, /* Cycles spent successfully polling and >> + processing packets during the current >> + interval. */ >> + RXQ_CYCLES_PROC_HIST, /* Total cycles of all intervals that are used >> + during rxq to pmd assignment. */ >> + RXQ_N_CYCLES >> +}; > > All patches wide: Multi-line comments should have a '*' on each line. > This struct follows the coding style "comments" section example struct. I'll check the other ones. >> + >> #define XPS_TIMEOUT_MS 500LL >> >> @@ -351,4 +364,11 @@ struct dp_netdev_rxq { >> particular core. */ >> struct dp_netdev_pmd_thread *pmd; /* pmd thread that polls this queue. */ >> + >> + /* Counters of cycles spent successfully polling and processing pkts. */ >> + atomic_ullong cycles[RXQ_N_CYCLES]; >> + /* We store PMD_RXQ_INTERVAL_MAX intervals of data for an rxq and then >> + sum them to yield the cycles used for an rxq. */ >> + atomic_ullong cycles_intrvl[PMD_RXQ_INTERVAL_MAX]; >> + unsigned intrvl_idx; /* Write index for 'cycles_intrvl'. */ > > Does it matter to save 2 letters in a variable names? It looks ugly and unreadable. > I think the current name is ok. Maybe not perfect, but ok. >> }; >> >> @@ -677,5 +697,4 @@ static void pmd_load_cached_ports(struct dp_netdev_pmd_thread *pmd) >> static inline void >> dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd); >> - > > Is it necessary to remove this line? It was here to split xps related functions > from others. > no, it wasn't necessary to remove, but not worthwhile of patch now. >> static void >> dpif_netdev_xps_revalidate_pmd(const struct dp_netdev_pmd_thread *pmd, >> @@ -3092,4 +3111,5 @@ cycles_count_end(struct dp_netdev_pmd_thread *pmd, >> static inline void >> cycles_count_intermediate(struct dp_netdev_pmd_thread *pmd, >> + struct dp_netdev_rxq *rxq, >> enum pmd_cycles_counter_type type) >> OVS_NO_THREAD_SAFETY_ANALYSIS >> @@ -3100,4 +3120,8 @@ cycles_count_intermediate(struct dp_netdev_pmd_thread *pmd, >> >> non_atomic_ullong_add(&pmd->cycles.n[type], interval); >> + if (rxq && (type == PMD_CYCLES_PROCESSING)) { >> + /* Add to the amount of current processing cycles. */ >> + non_atomic_ullong_add(&rxq->cycles[RXQ_CYCLES_PROC_CURR], interval); >> + } >> } >> >> @@ -3668,5 +3692,5 @@ dpif_netdev_run(struct dpif *dpif) >> port->rxqs[i].rx, >> port->port_no); >> - cycles_count_intermediate(non_pmd, process_packets ? >> + cycles_count_intermediate(non_pmd, NULL, process_packets ? >> PMD_CYCLES_PROCESSING >> : PMD_CYCLES_IDLE); > > It's not yours, but it'll be nice to fix here: > According to coding style, '?' should be on the next line near to arguments. > Also, IMHO, the whole construction should have the same level of indention. > > Like this: > cycles_count_intermediate(non_pmd, NULL, > process_packets > ? PMD_CYCLES_PROCESSING > : PMD_CYCLES_IDLE); > Or this: > cycles_count_intermediate( > non_pmd, NULL, process_packets ? PMD_CYCLES_PROCESSING > : PMD_CYCLES_IDLE); > I don't generally like to mix fixes with new code, even if it's small as a reviewer has to think about it and it alters the commit message. There's another coding style issue you caught elsewhere, so I'll fix them together in a follow up patch. >> @@ -3855,5 +3879,5 @@ reload: >> dp_netdev_process_rxq_port(pmd, poll_list[i].rxq->rx, >> poll_list[i].port_no); >> - cycles_count_intermediate(pmd, >> + cycles_count_intermediate(pmd, NULL, >> process_packets ? PMD_CYCLES_PROCESSING >> : PMD_CYCLES_IDLE); >> -- >> 1.8.3.1 > >
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index f35c079..8731435 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -182,4 +182,8 @@ struct emc_cache { #define DPCLS_OPTIMIZATION_INTERVAL 1000 +/* Number of intervals for which cycles are stored + * and used during rxq to pmd assignment. */ +#define PMD_RXQ_INTERVAL_MAX 6 + struct dpcls { struct cmap_node node; /* Within dp_netdev_pmd_thread.classifiers */ @@ -340,4 +344,13 @@ enum pmd_cycles_counter_type { }; +enum rxq_cycles_counter_type { + RXQ_CYCLES_PROC_CURR, /* Cycles spent successfully polling and + processing packets during the current + interval. */ + RXQ_CYCLES_PROC_HIST, /* Total cycles of all intervals that are used + during rxq to pmd assignment. */ + RXQ_N_CYCLES +}; + #define XPS_TIMEOUT_MS 500LL @@ -351,4 +364,11 @@ struct dp_netdev_rxq { particular core. */ struct dp_netdev_pmd_thread *pmd; /* pmd thread that polls this queue. */ + + /* Counters of cycles spent successfully polling and processing pkts. */ + atomic_ullong cycles[RXQ_N_CYCLES]; + /* We store PMD_RXQ_INTERVAL_MAX intervals of data for an rxq and then + sum them to yield the cycles used for an rxq. */ + atomic_ullong cycles_intrvl[PMD_RXQ_INTERVAL_MAX]; + unsigned intrvl_idx; /* Write index for 'cycles_intrvl'. */ }; @@ -677,5 +697,4 @@ static void pmd_load_cached_ports(struct dp_netdev_pmd_thread *pmd) static inline void dp_netdev_pmd_try_optimize(struct dp_netdev_pmd_thread *pmd); - static void dpif_netdev_xps_revalidate_pmd(const struct dp_netdev_pmd_thread *pmd, @@ -3092,4 +3111,5 @@ cycles_count_end(struct dp_netdev_pmd_thread *pmd, static inline void cycles_count_intermediate(struct dp_netdev_pmd_thread *pmd, + struct dp_netdev_rxq *rxq, enum pmd_cycles_counter_type type) OVS_NO_THREAD_SAFETY_ANALYSIS @@ -3100,4 +3120,8 @@ cycles_count_intermediate(struct dp_netdev_pmd_thread *pmd, non_atomic_ullong_add(&pmd->cycles.n[type], interval); + if (rxq && (type == PMD_CYCLES_PROCESSING)) { + /* Add to the amount of current processing cycles. */ + non_atomic_ullong_add(&rxq->cycles[RXQ_CYCLES_PROC_CURR], interval); + } } @@ -3668,5 +3692,5 @@ dpif_netdev_run(struct dpif *dpif) port->rxqs[i].rx, port->port_no); - cycles_count_intermediate(non_pmd, process_packets ? + cycles_count_intermediate(non_pmd, NULL, process_packets ? PMD_CYCLES_PROCESSING : PMD_CYCLES_IDLE); @@ -3855,5 +3879,5 @@ reload: dp_netdev_process_rxq_port(pmd, poll_list[i].rxq->rx, poll_list[i].port_no); - cycles_count_intermediate(pmd, + cycles_count_intermediate(pmd, NULL, process_packets ? PMD_CYCLES_PROCESSING : PMD_CYCLES_IDLE);
Add counters to dp_netdev_rxq which will later be used for storing the processing cycles of an rxq. Processing cycles will be stored in reference to a defined time interval. We will store the cycles of the current in progress interval, a number of completed intervals and the sum of the completed intervals. cycles_count_intermediate was used to count cycles for a pmd. With some small additions we can also use it to count the cycles used for processing an rxq. Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- lib/dpif-netdev.c | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-)