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. */ >>>> }; >>>> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev