[ovs-dev,v6,2/6] dpif-netdev: Add rxq processing cycle counters.
diff mbox

Message ID 1503617885-32501-3-git-send-email-ktraynor@redhat.com
State Accepted
Delegated to: Darrell Ball
Headers show

Commit Message

Kevin Traynor Aug. 24, 2017, 11:38 p.m. UTC
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(-)

Comments

Ilya Maximets Aug. 28, 2017, 1:28 p.m. UTC | #1
> 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
Kevin Traynor Aug. 30, 2017, 4:55 p.m. UTC | #2
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
> 
>

Patch
diff mbox

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);