diff mbox series

[ovs-dev] dpif-netlink: Count the number of offloaded rules

Message ID 20201206081645.29731-1-roid@nvidia.com
State Accepted
Headers show
Series [ovs-dev] dpif-netlink: Count the number of offloaded rules | expand

Commit Message

Roi Dayan Dec. 6, 2020, 8:16 a.m. UTC 
From: Jianbo Liu <jianbol@nvidia.com>

Add a counter for the offloaded rules, and display it in the command
of "ovs-appctl upcall/show".

Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
---
 lib/dpif-netlink.c            | 9 +++++++++
 lib/dpif.h                    | 1 +
 ofproto/ofproto-dpif-upcall.c | 6 ++++++
 3 files changed, 16 insertions(+)

Comments

Simon Horman Dec. 7, 2020, 1:54 p.m. UTC | #1 
On Sun, Dec 06, 2020 at 10:16:45AM +0200, Roi Dayan wrote:
> From: Jianbo Liu <jianbol@nvidia.com>
> 
> Add a counter for the offloaded rules, and display it in the command
> of "ovs-appctl upcall/show".
> 
> Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>

Thanks, applied.
Ilya Maximets Dec. 7, 2020, 2:12 p.m. UTC | #2 
On 12/6/20 9:16 AM, Roi Dayan wrote:
> From: Jianbo Liu <jianbol@nvidia.com>
> 
> Add a counter for the offloaded rules, and display it in the command
> of "ovs-appctl upcall/show".
> 
> Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>

I don't think this patch handles flow_flush case that could be triggered,
for example, by dpctl/del-flows command.

Second is that users of dpif_get_dp_stats() does not initialize stats
beforehand, that means that with this patch userspace datapath returns
garbage that will be printed to a user.  So, please, add an implementation
for userspace datapath.  And a unit tests with dummy offload provider
would be nice.

Could you, please, fix that in a follow up patch?

Best regards, Ilya Maximets.

> ---
>  lib/dpif-netlink.c            | 9 +++++++++
>  lib/dpif.h                    | 1 +
>  ofproto/ofproto-dpif-upcall.c | 6 ++++++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 2f881e4fadf1..6858ba6128d7 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -208,6 +208,7 @@ struct dpif_netlink {
>      /* Change notification. */
>      struct nl_sock *port_notifier; /* vport multicast group subscriber. */
>      bool refresh_channels;
> +    struct atomic_count n_offloaded_flows;
>  };
>  
>  static void report_loss(struct dpif_netlink *, struct dpif_channel *,
> @@ -653,6 +654,7 @@ dpif_netlink_run(struct dpif *dpif_)
>  static int
>  dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats)
>  {
> +    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>      struct dpif_netlink_dp dp;
>      struct ofpbuf *buf;
>      int error;
> @@ -678,6 +680,7 @@ dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats)
>          }
>          ofpbuf_delete(buf);
>      }
> +    stats->n_offloaded_flows = atomic_count_get(&dpif->n_offloaded_flows);
>      return error;
>  }
>  
> @@ -2189,6 +2192,9 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
>          }
>  
>          err = parse_flow_put(dpif, put);
> +        if (!err && (put->flags & DPIF_FP_CREATE)) {
> +            atomic_count_inc(&dpif->n_offloaded_flows);
> +        }
>          log_flow_put_message(&dpif->dpif, &this_module, put, 0);
>          break;
>      }
> @@ -2203,6 +2209,9 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
>                                  dpif_normalize_type(dpif_type(&dpif->dpif)),
>                                  del->ufid,
>                                  del->stats);
> +        if (!err) {
> +            atomic_count_dec(&dpif->n_offloaded_flows);
> +        }
>          log_flow_del_message(&dpif->dpif, &this_module, del, 0);
>          break;
>      }
> diff --git a/lib/dpif.h b/lib/dpif.h
> index cb047dbe2bf6..7ef148c6d756 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -429,6 +429,7 @@ struct dpif_dp_stats {
>      uint64_t n_missed;          /* Number of flow table misses. */
>      uint64_t n_lost;            /* Number of misses not sent to userspace. */
>      uint64_t n_flows;           /* Number of flows present. */
> +    uint64_t n_offloaded_flows; /* Number of offloaded flows present. */
>      uint64_t n_mask_hit;        /* Number of mega flow masks visited for
>                                     flow table matches. */
>      uint32_t n_masks;           /* Number of mega flow masks. */
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index e022fde27f81..19b92dfe0067 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -175,6 +175,7 @@ struct udpif {
>  
>      /* n_flows_mutex prevents multiple threads updating these concurrently. */
>      atomic_uint n_flows;               /* Number of flows in the datapath. */
> +    atomic_uint n_offloaded_flows;     /* Number of the offloaded flows. */
>      atomic_llong n_flows_timestamp;    /* Last time n_flows was updated. */
>      struct ovs_mutex n_flows_mutex;
>  
> @@ -730,6 +731,8 @@ udpif_get_n_flows(struct udpif *udpif)
>          dpif_get_dp_stats(udpif->dpif, &stats);
>          flow_count = stats.n_flows;
>          atomic_store_relaxed(&udpif->n_flows, flow_count);
> +        atomic_store_relaxed(&udpif->n_offloaded_flows,
> +                             stats.n_offloaded_flows);
>          ovs_mutex_unlock(&udpif->n_flows_mutex);
>      } else {
>          atomic_read_relaxed(&udpif->n_flows, &flow_count);
> @@ -2904,6 +2907,7 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>      struct udpif *udpif;
>  
>      LIST_FOR_EACH (udpif, list_node, &all_udpifs) {
> +        unsigned int n_offloaded_flows;
>          unsigned int flow_limit;
>          bool ufid_enabled;
>          size_t i;
> @@ -2915,6 +2919,8 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>          ds_put_format(&ds, "  flows         : (current %lu)"
>              " (avg %u) (max %u) (limit %u)\n", udpif_get_n_flows(udpif),
>              udpif->avg_n_flows, udpif->max_n_flows, flow_limit);
> +        atomic_read_relaxed(&udpif->n_offloaded_flows, &n_offloaded_flows);
> +        ds_put_format(&ds, "  offloaded flows : %u\n", n_offloaded_flows);
>          ds_put_format(&ds, "  dump duration : %lldms\n", udpif->dump_duration);
>          ds_put_format(&ds, "  ufid enabled : ");
>          if (ufid_enabled) {
>
Roi Dayan Dec. 7, 2020, 3:35 p.m. UTC | #3 
On 2020-12-07 4:12 PM, Ilya Maximets wrote:
> On 12/6/20 9:16 AM, Roi Dayan wrote:
>> From: Jianbo Liu <jianbol@nvidia.com>
>>
>> Add a counter for the offloaded rules, and display it in the command
>> of "ovs-appctl upcall/show".
>>
>> Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
>> Reviewed-by: Roi Dayan <roid@nvidia.com>
> 
> I don't think this patch handles flow_flush case that could be triggered,
> for example, by dpctl/del-flows command.
> 
> Second is that users of dpif_get_dp_stats() does not initialize stats
> beforehand, that means that with this patch userspace datapath returns
> garbage that will be printed to a user.  So, please, add an implementation
> for userspace datapath.  And a unit tests with dummy offload provider
> would be nice.
> 
> Could you, please, fix that in a follow up patch?
> 
> Best regards, Ilya Maximets.
> 

thanks Ilya. we'll check this.

>> ---
>>   lib/dpif-netlink.c            | 9 +++++++++
>>   lib/dpif.h                    | 1 +
>>   ofproto/ofproto-dpif-upcall.c | 6 ++++++
>>   3 files changed, 16 insertions(+)
>>
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index 2f881e4fadf1..6858ba6128d7 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -208,6 +208,7 @@ struct dpif_netlink {
>>       /* Change notification. */
>>       struct nl_sock *port_notifier; /* vport multicast group subscriber. */
>>       bool refresh_channels;
>> +    struct atomic_count n_offloaded_flows;
>>   };
>>   
>>   static void report_loss(struct dpif_netlink *, struct dpif_channel *,
>> @@ -653,6 +654,7 @@ dpif_netlink_run(struct dpif *dpif_)
>>   static int
>>   dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats)
>>   {
>> +    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
>>       struct dpif_netlink_dp dp;
>>       struct ofpbuf *buf;
>>       int error;
>> @@ -678,6 +680,7 @@ dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats)
>>           }
>>           ofpbuf_delete(buf);
>>       }
>> +    stats->n_offloaded_flows = atomic_count_get(&dpif->n_offloaded_flows);
>>       return error;
>>   }
>>   
>> @@ -2189,6 +2192,9 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
>>           }
>>   
>>           err = parse_flow_put(dpif, put);
>> +        if (!err && (put->flags & DPIF_FP_CREATE)) {
>> +            atomic_count_inc(&dpif->n_offloaded_flows);
>> +        }
>>           log_flow_put_message(&dpif->dpif, &this_module, put, 0);
>>           break;
>>       }
>> @@ -2203,6 +2209,9 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
>>                                   dpif_normalize_type(dpif_type(&dpif->dpif)),
>>                                   del->ufid,
>>                                   del->stats);
>> +        if (!err) {
>> +            atomic_count_dec(&dpif->n_offloaded_flows);
>> +        }
>>           log_flow_del_message(&dpif->dpif, &this_module, del, 0);
>>           break;
>>       }
>> diff --git a/lib/dpif.h b/lib/dpif.h
>> index cb047dbe2bf6..7ef148c6d756 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -429,6 +429,7 @@ struct dpif_dp_stats {
>>       uint64_t n_missed;          /* Number of flow table misses. */
>>       uint64_t n_lost;            /* Number of misses not sent to userspace. */
>>       uint64_t n_flows;           /* Number of flows present. */
>> +    uint64_t n_offloaded_flows; /* Number of offloaded flows present. */
>>       uint64_t n_mask_hit;        /* Number of mega flow masks visited for
>>                                      flow table matches. */
>>       uint32_t n_masks;           /* Number of mega flow masks. */
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index e022fde27f81..19b92dfe0067 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -175,6 +175,7 @@ struct udpif {
>>   
>>       /* n_flows_mutex prevents multiple threads updating these concurrently. */
>>       atomic_uint n_flows;               /* Number of flows in the datapath. */
>> +    atomic_uint n_offloaded_flows;     /* Number of the offloaded flows. */
>>       atomic_llong n_flows_timestamp;    /* Last time n_flows was updated. */
>>       struct ovs_mutex n_flows_mutex;
>>   
>> @@ -730,6 +731,8 @@ udpif_get_n_flows(struct udpif *udpif)
>>           dpif_get_dp_stats(udpif->dpif, &stats);
>>           flow_count = stats.n_flows;
>>           atomic_store_relaxed(&udpif->n_flows, flow_count);
>> +        atomic_store_relaxed(&udpif->n_offloaded_flows,
>> +                             stats.n_offloaded_flows);
>>           ovs_mutex_unlock(&udpif->n_flows_mutex);
>>       } else {
>>           atomic_read_relaxed(&udpif->n_flows, &flow_count);
>> @@ -2904,6 +2907,7 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>       struct udpif *udpif;
>>   
>>       LIST_FOR_EACH (udpif, list_node, &all_udpifs) {
>> +        unsigned int n_offloaded_flows;
>>           unsigned int flow_limit;
>>           bool ufid_enabled;
>>           size_t i;
>> @@ -2915,6 +2919,8 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>           ds_put_format(&ds, "  flows         : (current %lu)"
>>               " (avg %u) (max %u) (limit %u)\n", udpif_get_n_flows(udpif),
>>               udpif->avg_n_flows, udpif->max_n_flows, flow_limit);
>> +        atomic_read_relaxed(&udpif->n_offloaded_flows, &n_offloaded_flows);
>> +        ds_put_format(&ds, "  offloaded flows : %u\n", n_offloaded_flows);
>>           ds_put_format(&ds, "  dump duration : %lldms\n", udpif->dump_duration);
>>           ds_put_format(&ds, "  ufid enabled : ");
>>           if (ufid_enabled) {
>>
>
diff mbox series

Patch

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 2f881e4fadf1..6858ba6128d7 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -208,6 +208,7 @@  struct dpif_netlink {
     /* Change notification. */
     struct nl_sock *port_notifier; /* vport multicast group subscriber. */
     bool refresh_channels;
+    struct atomic_count n_offloaded_flows;
 };
 
 static void report_loss(struct dpif_netlink *, struct dpif_channel *,
@@ -653,6 +654,7 @@  dpif_netlink_run(struct dpif *dpif_)
 static int
 dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats)
 {
+    struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
     struct dpif_netlink_dp dp;
     struct ofpbuf *buf;
     int error;
@@ -678,6 +680,7 @@  dpif_netlink_get_stats(const struct dpif *dpif_, struct dpif_dp_stats *stats)
         }
         ofpbuf_delete(buf);
     }
+    stats->n_offloaded_flows = atomic_count_get(&dpif->n_offloaded_flows);
     return error;
 }
 
@@ -2189,6 +2192,9 @@  try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
         }
 
         err = parse_flow_put(dpif, put);
+        if (!err && (put->flags & DPIF_FP_CREATE)) {
+            atomic_count_inc(&dpif->n_offloaded_flows);
+        }
         log_flow_put_message(&dpif->dpif, &this_module, put, 0);
         break;
     }
@@ -2203,6 +2209,9 @@  try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
                                 dpif_normalize_type(dpif_type(&dpif->dpif)),
                                 del->ufid,
                                 del->stats);
+        if (!err) {
+            atomic_count_dec(&dpif->n_offloaded_flows);
+        }
         log_flow_del_message(&dpif->dpif, &this_module, del, 0);
         break;
     }
diff --git a/lib/dpif.h b/lib/dpif.h
index cb047dbe2bf6..7ef148c6d756 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -429,6 +429,7 @@  struct dpif_dp_stats {
     uint64_t n_missed;          /* Number of flow table misses. */
     uint64_t n_lost;            /* Number of misses not sent to userspace. */
     uint64_t n_flows;           /* Number of flows present. */
+    uint64_t n_offloaded_flows; /* Number of offloaded flows present. */
     uint64_t n_mask_hit;        /* Number of mega flow masks visited for
                                    flow table matches. */
     uint32_t n_masks;           /* Number of mega flow masks. */
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index e022fde27f81..19b92dfe0067 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -175,6 +175,7 @@  struct udpif {
 
     /* n_flows_mutex prevents multiple threads updating these concurrently. */
     atomic_uint n_flows;               /* Number of flows in the datapath. */
+    atomic_uint n_offloaded_flows;     /* Number of the offloaded flows. */
     atomic_llong n_flows_timestamp;    /* Last time n_flows was updated. */
     struct ovs_mutex n_flows_mutex;
 
@@ -730,6 +731,8 @@  udpif_get_n_flows(struct udpif *udpif)
         dpif_get_dp_stats(udpif->dpif, &stats);
         flow_count = stats.n_flows;
         atomic_store_relaxed(&udpif->n_flows, flow_count);
+        atomic_store_relaxed(&udpif->n_offloaded_flows,
+                             stats.n_offloaded_flows);
         ovs_mutex_unlock(&udpif->n_flows_mutex);
     } else {
         atomic_read_relaxed(&udpif->n_flows, &flow_count);
@@ -2904,6 +2907,7 @@  upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
     struct udpif *udpif;
 
     LIST_FOR_EACH (udpif, list_node, &all_udpifs) {
+        unsigned int n_offloaded_flows;
         unsigned int flow_limit;
         bool ufid_enabled;
         size_t i;
@@ -2915,6 +2919,8 @@  upcall_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
         ds_put_format(&ds, "  flows         : (current %lu)"
             " (avg %u) (max %u) (limit %u)\n", udpif_get_n_flows(udpif),
             udpif->avg_n_flows, udpif->max_n_flows, flow_limit);
+        atomic_read_relaxed(&udpif->n_offloaded_flows, &n_offloaded_flows);
+        ds_put_format(&ds, "  offloaded flows : %u\n", n_offloaded_flows);
         ds_put_format(&ds, "  dump duration : %lldms\n", udpif->dump_duration);
         ds_put_format(&ds, "  ufid enabled : ");
         if (ufid_enabled) {