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).
>
>> 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
