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

Ilya Maximets i.maximets at ovn.org
Thu Jan 16 10:59:49 UTC 2020


On 16.01.2020 11:18, Finn, Emma wrote:
> 
> 
>> -----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?

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 {
>>>


More information about the dev mailing list