[ovs-dev] [PATCH V4 08/17] dpif-netdev: Update offloaded flows statistics

Oz Shlomo ozsh at mellanox.com
Wed Dec 18 10:02:09 UTC 2019


Hi Ilya,


On 12/17/2019 11:34 PM, Ilya Maximets wrote:
> On 17.12.2019 21:56, Eli Britstein wrote:
>> On 12/17/2019 9:07 PM, Ilya Maximets wrote:
>>> On 17.12.2019 18:38, Eli Britstein wrote:
>>>> On 12/16/2019 8:47 PM, Ilya Maximets wrote:
>>>>> 3. New 'dp_layer' types has to be documented in dpctl man.
>>>>>       Also, 'in_hw' doesn't look like a datapath layer name.
>>>>>       Suggesting to use 'dpdk' as netdev-offload-tc uses 'tc'.
>>>>>       We also could return specific dp_layer for partially offloaded
>>>>>       flows, i.e. 'ovs-partial'.
>>>> Thanks for the patch. I applied and tested. It works well.
>>>>
>>>> Re-thinking about the dp_layer and offloaded, I think it's wrong.
>>>>
>>>> The 'dp_layer' should be "ovs-dpdk" to differentiate from kernel module
>>>> datapath "ovs", and denote the datapath is by dpdk.
>>> I don't think we need to differentiate userspace and kernel datapaths
>>> like this just because they are kind of same datapath layer, also,
>>> you're dumping flows always from the specific datapath/bridge, i.e. you
>>> know what is your datapath type.  Second point is that it definitely
>>> should not be called as 'ovs-dpdk' because there might be no DPDK at all.
>>> My suggestion is to have 'ovs' layer for all the usual non-offloadded
>>> flows, 'tc' for flows offloaded to linux tc via netdev-offload-tc
>>> and 'dpdk' for fully offloaded flows via netdev-offload-dpdk.
>> I think it's not very clear that the DP is changed because of
>> offloading. If we go ahead with this, so
>>
>> there is no point make the offloaded flag with 3 states, as partial
>> offloaded can be noted by "ovs"
>>
>> and "offloaded=yes". It can just be formatted this way in dump-flows
>> (offloaded=partial, dp: ovs).
> Good point.  This might be easier to implement.

So we agree that (offloaded=partial, dp: ovs) will be added to indicate 
that the dp runs in ovs layer with the HW providing match acceleration.

We are discussing two alternatives to indicate that a flow is fully 
offloaded by DPDK:

1. (dp: dpdk, offloaded=yes)

2. (dp: ovs, offloaded=yes)

The thing is that there is no real "dpdk" data plane as DPDK's software 
data plane is ovs, unlike TC which is a kernel  software data plane.

In addition, the (dp: dpdk, offloaded="") is not a valid system permutation.

Therefore, I vote for option 2, giving the following permutations:

(dp: ovs, offloaded="")  - dp runs in ovs layer (kernel/dpdk)

(dp: ovs, offloaded="yes")  - dp runs in hardware (controlled by OVS)

(dp: ovs, offloaded="partial")  - dp runs in ovs layer with HW providing 
match acceleration

(dp: tc, offloaded="")  -  dp runs in tc kernel

(dp: tc, offloaded="yes")  - dp runs in hardware (controlled by TC)

What do you think?


>>>> It is true for any kind of offloading.
>>>>
>>>> This can be done in dp_netdev_flow_to_dpif_flow to handle offloading is
>>>> off, and filled in netdev-offload-dpdk (to the same value).
>>>>
>>>> The "offloaded" flag can be enhanced to 3 states, instead of just
>>>> boolean, as below (of course need to adapt throughout the code).
>>>>
>>>> So, we will be able to show "partial" or "yes" for "offloaded", in
>>>> dpctl/dump-flows.
>>> Yes, I think there were such idea in early discussion with Roni this year.
>>> So, we could implement this.  For partially offloaded flows it might
>>> make sense to have dp_layer = "ovs" since most of the processing happens
>>> in the main userspace datapath.
>>>
>>>> Please comment.
>>>>
>>>>
>>>> diff --git a/lib/dpif.h b/lib/dpif.h
>>>> index d96f854a3..dba2f3cbf 100644
>>>> --- a/lib/dpif.h
>>>> +++ b/lib/dpif.h
>>>> @@ -508,8 +508,14 @@ struct dpif_flow_detailed_stats {
>>>>         uint16_t tcp_flags;
>>>>     };
>>>>
>>>> +enum ol_state {
>>>> +    OFFLOADED_STATE_OFF,
>>>> +    OFFLOADED_STATE_ON,
>>>> +    OFFLOADED_STATE_PARTIAL,
>>> How about:
>>>
>>> enum dpif_flow_ol_state {
>>>       DPIF_FLOW_OL_STATE_NOT_OFFLOADED, /* Flow fully handled in software. */
>>>       DPIF_FLOW_OL_STATE_OFFLOADED,     /* Flow fully handled in hardware. */
>>>       DPIF_FLOW_OL_STATE_PARTIAL,       /* Flow matching handled in hardware. */
>>> };
>>>
>>>> +};
>>>> +
>>>>     struct dpif_flow_attrs {
>>>> -    bool offloaded;         /* True if flow is offloaded to HW. */
>>>> +    enum ol_state offloaded;
>>> Some minimal comment would be nice here.
>>>       enum dpif_flow_ol_state ol_state;  /* Status of HW offloading. */
>>>
>>>>         const char *dp_layer;   /* DP layer the flow is handled in. */
>>>>     };
>>>>


More information about the dev mailing list