[ovs-dev,v4] dpif-netdev: Modified ovs-appctl dpctl/dump-flows command
diff mbox series

Message ID 1579175088-45449-1-git-send-email-emma.finn@intel.com
State Superseded
Headers show
Series
  • [ovs-dev,v4] dpif-netdev: Modified ovs-appctl dpctl/dump-flows command
Related show

Commit Message

Finn, Emma Jan. 16, 2020, 11:44 a.m. UTC
Modified ovs-appctl dpctl/dump-flows command to output
the miniflow bits for a given flow when -m option is passed.

$ ovs-appctl dpctl/dump-flows -m

Signed-off-by: Emma Finn <emma.finn@intel.com>

---

RFC -> v1

* Changed revision from RFC to v1
* Reformatted based on comments
* Fixed same classifier being dumped multiple times
  flagged by Ilya
* Fixed print of subtables flagged by William
* Updated print count of bits as well as bits themselves

---

v1 -> v2

* Reformatted based on comments
* Refactored code to make output part of ovs-appctl
  dpctl/dump-flows -m command.

---

v2 -> v3

* Added attribute dp_extra_info to dpif_flow_attrs struct
  to store miniflow bits as a string

---

v3 -> v4

* Fixed string leak
* Refactored to code to make it  independent from the flowmap size
---
 NEWS              |  2 ++
 lib/dpctl.c       |  5 +++++
 lib/dpif-netdev.c | 14 ++++++++++++++
 lib/dpif.h        |  1 +
 4 files changed, 22 insertions(+)

Comments

Finn, Emma Jan. 16, 2020, 2:09 p.m. UTC | #1
Comments inline.

> -----Original Message-----
> From: Finn, Emma <emma.finn@intel.com>
> Sent: Thursday 16 January 2020 11:45
> To: dev@openvswitch.org
> Cc: i.maximets@ovn.org; ian.stokes@intel.org; Finn, Emma
> <emma.finn@intel.com>
> Subject: [v4] dpif-netdev: Modified ovs-appctl dpctl/dump-flows command
> 
> Modified ovs-appctl dpctl/dump-flows command to output the miniflow bits
> for a given flow when -m option is passed.
> 
> $ ovs-appctl dpctl/dump-flows -m
> 
> Signed-off-by: Emma Finn <emma.finn@intel.com>
> 
> ---
> 
> RFC -> v1
> 
> * Changed revision from RFC to v1
> * Reformatted based on comments
> * Fixed same classifier being dumped multiple times
>   flagged by Ilya
> * Fixed print of subtables flagged by William
> * Updated print count of bits as well as bits themselves
> 
> ---
> 
> v1 -> v2
> 
> * Reformatted based on comments
> * Refactored code to make output part of ovs-appctl
>   dpctl/dump-flows -m command.
> 
> ---
> 
> v2 -> v3
> 
> * Added attribute dp_extra_info to dpif_flow_attrs struct
>   to store miniflow bits as a string
> 
> ---
> 
> v3 -> v4
> 
> * Fixed string leak
> * Refactored to code to make it  independent from the flowmap size
> ---

Any other feedback? 
I will have to rebase a send v5.

Regards, 
Emma

>  NEWS              |  2 ++
>  lib/dpctl.c       |  5 +++++
>  lib/dpif-netdev.c | 14 ++++++++++++++
>  lib/dpif.h        |  1 +
>  4 files changed, 22 insertions(+)
> 
> diff --git a/NEWS b/NEWS
> index 965faca..1c9d2db 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -8,6 +8,8 @@ Post-v2.12.0
>       * Add option to enable, disable and query TCP sequence checking in
>         conntrack.
>       * Add support for conntrack zone limits.
> +     * Command "ovs-appctl dpctl/dump-flows" refactored to show subtable
> +       miniflow bits for userspace datapath.
>     - AF_XDP:
>       * New option 'use-need-wakeup' for netdev-afxdp to control enabling
>         of corresponding 'need_wakeup' flag in AF_XDP rings.  Enabled by
> default diff --git a/lib/dpctl.c b/lib/dpctl.c index a1ea25b..1b0a2bf 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -825,6 +825,11 @@ format_dpif_flow(struct ds *ds, const struct
> dpif_flow *f, struct hmap *ports,
>      }
>      ds_put_cstr(ds, ", actions:");
>      format_odp_actions(ds, f->actions, f->actions_len, ports);
> +    if (dpctl_p->verbosity && f->attrs.dp_extra_info) {
> +        ds_put_format(ds, ", dp-extra-info:%s",
> +                      f->attrs.dp_extra_info);
> +    }
> +    free(f->attrs.dp_extra_info);
>  }
> 
>  struct dump_types {
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 079bd1b..a640b49
> 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3101,6 +3101,20 @@ dp_netdev_flow_to_dpif_flow(const struct
> dp_netdev_flow *netdev_flow,
> 
>      flow->attrs.offloaded = false;
>      flow->attrs.dp_layer = "ovs";
> +
> +    struct ds extra_info = DS_EMPTY_INITIALIZER;
> +    size_t unit;
> +
> +    ds_put_cstr(&extra_info, "miniflow_bits("); FLOWMAP_FOR_EACH_UNIT
> (unit) {
> +        if (unit) {
> +            ds_put_char(&extra_info, ',');
> +        }
> +        ds_put_format(&extra_info, "%d",
> +                      count_1bits(netdev_flow->cr.mask->mf.map.bits[unit]));
> +    }
> +    ds_put_char(&extra_info, ')');
> +    flow->attrs.dp_extra_info = ds_steal_cstr(&extra_info);
> +    ds_destroy(&extra_info);
>  }
> 
>  static int
> diff --git a/lib/dpif.h b/lib/dpif.h
> index c21e897..59d82dc 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -513,6 +513,7 @@ struct dpif_flow_detailed_stats {  struct
> dpif_flow_attrs {
>      bool offloaded;         /* True if flow is offloaded to HW. */
>      const char *dp_layer;   /* DP layer the flow is handled in. */
> +    char *dp_extra_info;    /* Extra information provided by DP. */
>  };
> 
>  struct dpif_flow_dump_types {
> --
> 2.7.4
Ilya Maximets Jan. 16, 2020, 2:18 p.m. UTC | #2
On 16.01.2020 15:09, Finn, Emma wrote:
> Comments inline.
> 
>> -----Original Message-----
>> From: Finn, Emma <emma.finn@intel.com>
>> Sent: Thursday 16 January 2020 11:45
>> To: dev@openvswitch.org
>> Cc: i.maximets@ovn.org; ian.stokes@intel.org; Finn, Emma
>> <emma.finn@intel.com>
>> Subject: [v4] dpif-netdev: Modified ovs-appctl dpctl/dump-flows command
>>
>> Modified ovs-appctl dpctl/dump-flows command to output the miniflow bits
>> for a given flow when -m option is passed.
>>
>> $ ovs-appctl dpctl/dump-flows -m
>>
>> Signed-off-by: Emma Finn <emma.finn@intel.com>
>>
>> ---
>>
>> RFC -> v1
>>
>> * Changed revision from RFC to v1
>> * Reformatted based on comments
>> * Fixed same classifier being dumped multiple times
>>   flagged by Ilya
>> * Fixed print of subtables flagged by William
>> * Updated print count of bits as well as bits themselves
>>
>> ---
>>
>> v1 -> v2
>>
>> * Reformatted based on comments
>> * Refactored code to make output part of ovs-appctl
>>   dpctl/dump-flows -m command.
>>
>> ---
>>
>> v2 -> v3
>>
>> * Added attribute dp_extra_info to dpif_flow_attrs struct
>>   to store miniflow bits as a string
>>
>> ---
>>
>> v3 -> v4
>>
>> * Fixed string leak
>> * Refactored to code to make it  independent from the flowmap size
>> ---
> 
> Any other feedback? 
> I will have to rebase a send v5.

I didn't test this.  But since you're rebasing, couple of style
coments inline.

> 
> Regards, 
> Emma
> 
>>  NEWS              |  2 ++
>>  lib/dpctl.c       |  5 +++++
>>  lib/dpif-netdev.c | 14 ++++++++++++++
>>  lib/dpif.h        |  1 +
>>  4 files changed, 22 insertions(+)
>>
>> diff --git a/NEWS b/NEWS
>> index 965faca..1c9d2db 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -8,6 +8,8 @@ Post-v2.12.0
>>       * Add option to enable, disable and query TCP sequence checking in
>>         conntrack.
>>       * Add support for conntrack zone limits.
>> +     * Command "ovs-appctl dpctl/dump-flows" refactored to show subtable
>> +       miniflow bits for userspace datapath.
>>     - AF_XDP:
>>       * New option 'use-need-wakeup' for netdev-afxdp to control enabling
>>         of corresponding 'need_wakeup' flag in AF_XDP rings.  Enabled by
>> default diff --git a/lib/dpctl.c b/lib/dpctl.c index a1ea25b..1b0a2bf 100644
>> --- a/lib/dpctl.c
>> +++ b/lib/dpctl.c
>> @@ -825,6 +825,11 @@ format_dpif_flow(struct ds *ds, const struct
>> dpif_flow *f, struct hmap *ports,
>>      }
>>      ds_put_cstr(ds, ", actions:");
>>      format_odp_actions(ds, f->actions, f->actions_len, ports);
>> +    if (dpctl_p->verbosity && f->attrs.dp_extra_info) {
>> +        ds_put_format(ds, ", dp-extra-info:%s",
>> +                      f->attrs.dp_extra_info);

This should fit in a single line. Doesn't it?

>> +    }
>> +    free(f->attrs.dp_extra_info);
>>  }
>>
>>  struct dump_types {
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 079bd1b..a640b49
>> 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -3101,6 +3101,20 @@ dp_netdev_flow_to_dpif_flow(const struct
>> dp_netdev_flow *netdev_flow,
>>
>>      flow->attrs.offloaded = false;
>>      flow->attrs.dp_layer = "ovs";
>> +
>> +    struct ds extra_info = DS_EMPTY_INITIALIZER;
>> +    size_t unit;
>> +
>> +    ds_put_cstr(&extra_info, "miniflow_bits("); FLOWMAP_FOR_EACH_UNIT
>> (unit) {

Something bad happened here so lines was concatenated.

>> +        if (unit) {
>> +            ds_put_char(&extra_info, ',');
>> +        }
>> +        ds_put_format(&extra_info, "%d",
>> +                      count_1bits(netdev_flow->cr.mask->mf.map.bits[unit]));
>> +    }
>> +    ds_put_char(&extra_info, ')');
>> +    flow->attrs.dp_extra_info = ds_steal_cstr(&extra_info);
>> +    ds_destroy(&extra_info);
>>  }
>>
>>  static int
>> diff --git a/lib/dpif.h b/lib/dpif.h
>> index c21e897..59d82dc 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -513,6 +513,7 @@ struct dpif_flow_detailed_stats {  struct
>> dpif_flow_attrs {
>>      bool offloaded;         /* True if flow is offloaded to HW. */
>>      const char *dp_layer;   /* DP layer the flow is handled in. */
>> +    char *dp_extra_info;    /* Extra information provided by DP. */
>>  };
>>
>>  struct dpif_flow_dump_types {
>> --
>> 2.7.4
>

Patch
diff mbox series

diff --git a/NEWS b/NEWS
index 965faca..1c9d2db 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,8 @@  Post-v2.12.0
      * Add option to enable, disable and query TCP sequence checking in
        conntrack.
      * Add support for conntrack zone limits.
+     * Command "ovs-appctl dpctl/dump-flows" refactored to show subtable
+       miniflow bits for userspace datapath.
    - AF_XDP:
      * New option 'use-need-wakeup' for netdev-afxdp to control enabling
        of corresponding 'need_wakeup' flag in AF_XDP rings.  Enabled by default
diff --git a/lib/dpctl.c b/lib/dpctl.c
index a1ea25b..1b0a2bf 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -825,6 +825,11 @@  format_dpif_flow(struct ds *ds, const struct dpif_flow *f, struct hmap *ports,
     }
     ds_put_cstr(ds, ", actions:");
     format_odp_actions(ds, f->actions, f->actions_len, ports);
+    if (dpctl_p->verbosity && f->attrs.dp_extra_info) {
+        ds_put_format(ds, ", dp-extra-info:%s",
+                      f->attrs.dp_extra_info);
+    }
+    free(f->attrs.dp_extra_info);
 }
 
 struct dump_types {
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 079bd1b..a640b49 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -3101,6 +3101,20 @@  dp_netdev_flow_to_dpif_flow(const struct dp_netdev_flow *netdev_flow,
 
     flow->attrs.offloaded = false;
     flow->attrs.dp_layer = "ovs";
+
+    struct ds extra_info = DS_EMPTY_INITIALIZER;
+    size_t unit;
+
+    ds_put_cstr(&extra_info, "miniflow_bits("); FLOWMAP_FOR_EACH_UNIT (unit) {
+        if (unit) {
+            ds_put_char(&extra_info, ',');
+        }
+        ds_put_format(&extra_info, "%d",
+                      count_1bits(netdev_flow->cr.mask->mf.map.bits[unit]));
+    }
+    ds_put_char(&extra_info, ')');
+    flow->attrs.dp_extra_info = ds_steal_cstr(&extra_info);
+    ds_destroy(&extra_info);
 }
 
 static int
diff --git a/lib/dpif.h b/lib/dpif.h
index c21e897..59d82dc 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -513,6 +513,7 @@  struct dpif_flow_detailed_stats {
 struct dpif_flow_attrs {
     bool offloaded;         /* True if flow is offloaded to HW. */
     const char *dp_layer;   /* DP layer the flow is handled in. */
+    char *dp_extra_info;    /* Extra information provided by DP. */
 };
 
 struct dpif_flow_dump_types {