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

[Ilya Maximets]

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.

> 
> 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. */
>   };
>