[ovs-dev,v3] 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
---
 NEWS              | 2 ++
 lib/dpctl.c       | 4 ++++
 lib/dpif-netdev.c | 8 ++++++++
 lib/dpif.h        | 1 +
 4 files changed, 15 insertions(+)

On 15.01.2020 17:19, Emma Finn wrote:
> 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
> ---
>  NEWS              | 2 ++
>  lib/dpctl.c       | 4 ++++
>  lib/dpif-netdev.c | 8 ++++++++
>  lib/dpif.h        | 1 +
>  4 files changed, 15 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..bd33ac4 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -825,6 +825,10 @@ 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:miniflow_bits%s",
> +                      f->attrs.dp_extra_info);

Please, make 'miniflow_bits' part of f->attrs.dp_extra_info.
This doesn't make sense for other datapaths.

And the string leaked here.

> +    }
>  }
>  
>  struct dump_types {
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 079bd1b..51def10 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3101,6 +3101,14 @@ 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;
> +
> +    ds_put_format(&extra_info, "(0x%X,0x%X)",
> +                  count_1bits(netdev_flow->cr.mask->mf.map.bits[0]),
> +                  count_1bits(netdev_flow->cr.mask->mf.map.bits[1]));
> +    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..064f884 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;

Comment required.  Keep it generic, please, i.e. no mentioning of 'miniflow_bits'.

>  };
>  
>  struct dpif_flow_dump_types {
>

> -----Original Message-----
> From: Ilya Maximets <i.maximets@ovn.org>
> Sent: Wednesday 15 January 2020 17:27
> To: Finn, Emma <emma.finn@intel.com>; dev@openvswitch.org
> Cc: i.maximets@ovn.org; ian.stokes@intel.org
> Subject: Re: [v3] dpif-netdev: Modified ovs-appctl dpctl/dump-flows
> command
> 
> On 15.01.2020 17:19, Emma Finn wrote:
> > 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
> > ---
> >  NEWS              | 2 ++
> >  lib/dpctl.c       | 4 ++++
> >  lib/dpif-netdev.c | 8 ++++++++
> >  lib/dpif.h        | 1 +
> >  4 files changed, 15 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..bd33ac4 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > @@ -825,6 +825,10 @@ 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:miniflow_bits%s",
> > +                      f->attrs.dp_extra_info);
> 
> Please, make 'miniflow_bits' part of f->attrs.dp_extra_info.
> This doesn't make sense for other datapaths.
> 
> And the string leaked here.

Hi Ilya, Can you explain further what you are referring to here?
> 
> > +    }
> >  }
> >
> >  struct dump_types {
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> > 079bd1b..51def10 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -3101,6 +3101,14 @@ 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;
> > +
> > +    ds_put_format(&extra_info, "(0x%X,0x%X)",
> > +                  count_1bits(netdev_flow->cr.mask->mf.map.bits[0]),
> > +                  count_1bits(netdev_flow->cr.mask->mf.map.bits[1]));
> > +    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..064f884 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;
> 
> Comment required.  Keep it generic, please, i.e. no mentioning of
> 'miniflow_bits'.
> 
/* Stores extra information on DP*/  - Is this comment Ok?
> >  };
> >
> >  struct dpif_flow_dump_types {
> >

On 16.01.2020 11:18, Finn, Emma wrote:
> 
> 
>> -----Original Message-----
>> From: Ilya Maximets <i.maximets@ovn.org>
>> Sent: Wednesday 15 January 2020 17:27
>> To: Finn, Emma <emma.finn@intel.com>; dev@openvswitch.org
>> Cc: i.maximets@ovn.org; ian.stokes@intel.org
>> Subject: Re: [v3] dpif-netdev: Modified ovs-appctl dpctl/dump-flows
>> command
>>
>> On 15.01.2020 17:19, Emma Finn wrote:
>>> 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
>>> ---
>>>  NEWS              | 2 ++
>>>  lib/dpctl.c       | 4 ++++
>>>  lib/dpif-netdev.c | 8 ++++++++
>>>  lib/dpif.h        | 1 +
>>>  4 files changed, 15 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..bd33ac4 100644
>>> --- a/lib/dpctl.c
>>> +++ b/lib/dpctl.c
>>> @@ -825,6 +825,10 @@ 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:miniflow_bits%s",
>>> +                      f->attrs.dp_extra_info);
>>
>> Please, make 'miniflow_bits' part of f->attrs.dp_extra_info.
>> This doesn't make sense for other datapaths.
>>
>> And the string leaked here.
> 
> Hi Ilya, Can you explain further what you are referring to here?

I mean that here should be:
    ds_put_format(ds, ", dp-extra-info:%s", f->attrs.dp_extra_info);

And in dpif-netdev.c:
    ds_put_format(&extra_info, "miniflow_bits(0x%X,0x%X)", ...


And someone needs to call free() on f->attrs.dp_extra_info in the end.
(Note that this should be done regardless of verbosity level).

>>
>>> +    }
>>>  }
>>>
>>>  struct dump_types {
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
>>> 079bd1b..51def10 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -3101,6 +3101,14 @@ 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;
>>> +
>>> +    ds_put_format(&extra_info, "(0x%X,0x%X)",
>>> +                  count_1bits(netdev_flow->cr.mask->mf.map.bits[0]),
>>> +                  count_1bits(netdev_flow->cr.mask->mf.map.bits[1]));

Again, what is the purpose of printing bits count in hex?
Bits count should be printed as decimal, while actual bitmap may be
printed as hex (I don't think we actually need to print it).

One more thing is that we could actually make above code independent from
the flowmap size like this:

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..064f884 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;
>>
>> Comment required.  Keep it generic, please, i.e. no mentioning of
>> 'miniflow_bits'.
>>
> /* Stores extra information on DP*/  - Is this comment Ok?

/* Extra information provided by DP. */ ?

>>>  };
>>>
>>>  struct dpif_flow_dump_types {
>>>