[ovs-dev] [PATCH 00/20] netdev datapath actions offload

[Eli Britstein]

On 11/22/2019 6:34 PM, Ilya Maximets wrote:
> On 20.11.2019 16:28, Eli Britstein wrote:
>> Currently, netdev datapath offload only accelerates the flow match
>> sequence by associating a mark per flow. This series introduces the full
>> offload of netdev datapath flows by having the HW also perform the flow
>> actions.
>>
>> This series adds HW offload for output, drop, set MAC, set IPv4, set
>> TCP/UDP ports and tunnel push actions using the DPDK rte_flow API.
>>
>> Travis passed: https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Felibritstein%2FOVS%2Fbuilds%2F614511472&data=02%7C01%7Celibr%40mellanox.com%7C893d54b1da3748fab12408d76f69dbcf%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637100372770275434&sdata=031VCVHeS7HxQB%2F6PvZHZOIWNGMl1BcZqMM0JbDKaa8%3D&reserved=0
>>
>> Eli Britstein (18):
>>    sparse: rte_flow.h: Adapt according to DPDK 18.11.2
>>    netdev-offload-dpdk: Refactor flow patterns and actions code
>>    netdev-offload-dpdk-flow: Dynamically allocate pattern items
>>    netdev-offload-dpdk: Fix typo
>>    netdev-dpdk: Improve HW offload flow debuggability
>>    netdev-dpdk: Introduce flow_flush method
>>    netdev-offload-dpdk: Introduce flow flush callback
>>    netdev-offload-dpdk: Framework for actions offload
>>    netdev-dpdk: Introduce rte flow query function
>>    netdev-offload-dpdk: Implement flow stats get
>>    dpif-netdev: Populate dpif class field in offload struct
>>    netdev-dpdk: Getter function for dpdk port id API
>>    netdev-offload-dpdk-flow: Support offload of output action
>>    netdev-offload-dpdk-flow: Support offload of drop action
>>    netdev-offload-dpdk-flow: Support offload of set MAC actions
>>    netdev-offload-dpdk-flow: Support offload of set IPv4 actions
>>    netdev-offload-dpdk-flow: Support offload of clone tnl_push/output
>>      actions
>>    netdev-offload-dpdk-flow: Support offload of set TCP/UDP ports actions
>>
>> Ophir Munk (2):
>>    netdev: Add netdev function: flow_stats_get()
>>    dpif-netdev: Read hw stats during flow_dump_next() call
>>
>>   NEWS                              |   3 +
>>   include/sparse/rte_flow.h         | 810 ++++++++++++++++++++++++++++++---
>>   lib/automake.mk                   |   4 +-
>>   lib/dpif-netdev.c                 |  42 +-
>>   lib/netdev-dpdk.c                 |  76 ++++
>>   lib/netdev-dpdk.h                 |  11 +
>>   lib/netdev-offload-dpdk-flow.c    | 916 ++++++++++++++++++++++++++++++++++++++
>>   lib/netdev-offload-dpdk-private.h |  65 +++
>>   lib/netdev-offload-dpdk.c         | 535 +++-------------------
>>   lib/netdev-offload-provider.h     |   7 +
>>   lib/netdev-offload.c              |  12 +
>>   lib/netdev-offload.h              |   3 +
>>   12 files changed, 1969 insertions(+), 515 deletions(-)
>>   create mode 100644 lib/netdev-offload-dpdk-flow.c
>>   create mode 100644 lib/netdev-offload-dpdk-private.h
>>
> Hi. Thanks for working on this!
>
> I really roughly read the patch-set and made a few comments for
> the places that caught my eyes.
>
> Few general comments for the patches:
>
> * I don't understand why all of this refactoring was done. Maybe it's because
>    I didn't try to apply the series and look at it as a complete change, but
>    for now it's unclear for me, especially because we already refactored this
>    code few times before and you're reverting some of these changes.
>    Also the need to have netdev-offload-dpdk-flow.c is not clear.

I wanted to make it more ordered, and split to a new file as we plan to 
submit more patches in this area for VXLAN offload and later CT. Those 
will include a lot of code additions and maybe even more files. So, we 
can either keep it so a single file won't grow to be very large, or we 
can postpone it for later if it really grows like this. I prefer to keep 
it as is, unless you object.

What have I reverted?

>
> * Commit messages are needed.  Please, add some with a brief explanation
>    on what happens and why.
I'll elaborate a bit more, though there are some commits (last ones) 
that there is really nothing to elaborate more than the subject.
>
> * If it's not hard, since you will re-spin the series anyway, please, add
>    some periods to the ends of a subject lines.
Actually, I've seen that there is inconsistency of this style in OVS. 
Some commits have this period and some don't. For example in kernel, 
iproute2, dpdk gits, they are consistent *without* it. I think it should 
be consistent. I don't see the added value of it, and I think maybe we 
should adopt the dot-less style. What do you think?
>
> * You need to also update the 'Flow Hardware Offload' section in the
>    Documentation/howto/dpdk.rst.
OK. In v2
>
> * And, please, mark these new features as experimental in NEWS.
OK. In v2
>
> Best regards, Ilya Maximets.