[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