[ovs-dev] [PATCH 02/20] netdev-offload-dpdk: Refactor flow patterns and actions code

[Ilya Maximets]

On 03.12.2019 15:55, Sriharsha Basavapatna wrote:
> Hi Eli,
> 
> Thanks for this full-offload patch set. I have some comments inline.
> 
> Thanks,
> -Harsha
> 
> On Wed, Nov 20, 2019 at 9:07 PM Eli Britstein <elibr at mellanox.com> wrote:
>>
>> Refactor the flow patterns and actions code to a new source files for
>> better readability and towards adding more code to it.
>>
>> Signed-off-by: Eli Britstein <elibr at mellanox.com>
>> Reviewed-by: Oz Shlomo <ozsh at mellanox.com>
>> ---
>>  lib/automake.mk                   |   4 +-
>>  lib/netdev-offload-dpdk-flow.c    | 479 ++++++++++++++++++++++++++++++++++++++
>>  lib/netdev-offload-dpdk-private.h |  69 ++++++
>>  lib/netdev-offload-dpdk.c         | 466 +-----------------------------------
>>  4 files changed, 562 insertions(+), 456 deletions(-)
>>  create mode 100644 lib/netdev-offload-dpdk-flow.c
>>  create mode 100644 lib/netdev-offload-dpdk-private.h
>>
>> diff --git a/lib/automake.mk b/lib/automake.mk
>> index 17b36b43d..3a813af85 100644
>> --- a/lib/automake.mk
>> +++ b/lib/automake.mk
>> @@ -142,6 +142,7 @@ lib_libopenvswitch_la_SOURCES = \
>>         lib/netdev-dummy.c \
>>         lib/netdev-offload.c \
>>         lib/netdev-offload.h \
>> +       lib/netdev-offload-dpdk-private.h \
>>         lib/netdev-offload-provider.h \
>>         lib/netdev-provider.h \
>>         lib/netdev-vport.c \
>> @@ -426,7 +427,8 @@ if DPDK_NETDEV
>>  lib_libopenvswitch_la_SOURCES += \
>>         lib/dpdk.c \
>>         lib/netdev-dpdk.c \
>> -       lib/netdev-offload-dpdk.c
>> +       lib/netdev-offload-dpdk.c \
>> +       lib/netdev-offload-dpdk-flow.c
> 
> 1. The existing netdev-offload-dpdk.c file is not too big - around
> 700+ lines.   Why not add these new functions to the same file ? May
> be add them in a separate section in that file ?

So, it seems like not only my opinion.  Another point here is that
since OVS repo has almost flat structure it's better to minimize
the number of files, otherwise it starts looking ugly with all that
huge file names.

In fact, if you'll drop the refactoring part from the patch set it
will be much shorter and easier to review.  So, I'm suggesting to
do that.


> 2. The functions in the new file begin with the prefix netdev_dpdk_.
> But that same prefix is used by functions in netdev-dpdk.c and it can
> be confusing.

Agree.  All the exported functions should have unique prefix.

Best regards, Ilya Maximets.