[ovs-dev] [v3] dpif-netdev: Modified ovs-appctl dpctl/dump-flows command

Finn, Emma emma.finn at intel.com
Thu Jan 16 10:18:45 UTC 2020



> -----Original Message-----
> From: Ilya Maximets <i.maximets at ovn.org>
> Sent: Wednesday 15 January 2020 17:27
> To: Finn, Emma <emma.finn at intel.com>; dev at openvswitch.org
> Cc: i.maximets at ovn.org; ian.stokes at 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 at 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 {
> >


More information about the dev mailing list