[ovs-dev] [PATCH v12 00/11] Add offload support for sFlow

[Chris Mi]

On 3/1/2021 8:48 PM, Ilya Maximets wrote:
> On 3/1/21 9:30 AM, Chris Mi wrote:
>> Hi Simon, Ilya,
>>
>> Could I know what should we do to make progress for this patch set?
>> It has been posted in the community for a long time 😁
> In general, the way to get your patches reviewed is to review other
> patches.  It's simply because we still have a huge review backlog
> (214 patches right now in patchwork and most of them needs review)
> and bugfixes usually has a bit higher priority.  By reviewing other
> patches you're reducing amount of work for maintainers so they can
> get to your patches faster.
>
> For the series and comments from Eelco:
> I didn't read the patches carefully, only a quick glance, but I still
> do not understand why we need a separate thread to poll psample events.
> Why can't we just allow usual handler threads to do that?  From the
> architecture perspective it's not a good thing to call ofproto code
> from the offload-provider.  This introduces lots of complications
> and might cause lots of issues in the future.
>
> I'd say that design should look like this:
>
>    handler thread ->
>      dpif_recv() ->
>        dpif_netlink_recv() ->
>          netdev_offload_recv() ->
>            netdev_offload_tc_recv() ->
>              nl_sock_recv()
>
> Last three calls should be implemented.
>
> The better version of this will be to throw away dpif part from
> above call chain and make it:
>
>    handler thread ->
>      netdev_offload_recv() ->
>        netdev_offload_tc_recv() ->
>          nl_sock_recv()
>
> This way we could avoid touching dpif-netdev and still have psample
> offloading for the case where netdev-offload-tc is used from the
> userspace datapath.
>
> Above architecture also implies implementation of:
>   - netdev_offload_recv_wait()
>   - netdev_offload_recv_purge()
>   - and the netdev_offload_tc_* counterparts.
Thanks for your above suggestion, Ilya. I'll check what need to be improved.

Regards,
Chris
> Best regards, Ilya Maximets.
>
>> Thanks,
>> Chris
>>
>> On 2/23/2021 5:08 PM, Roi Dayan wrote:
>>>
>>> On 2021-01-27 8:23 AM, Chris Mi wrote:
>>>> This patch set adds offload support for sFlow.
>>>>
>>>> Psample is a genetlink channel for packet sampling. TC action act_sample
>>>> uses psample to send sampled packets to userspace.
>>>>
>>>> When offloading sample action to TC, userspace creates a unique ID to
>>>> map sFlow action and tunnel info and passes this ID to kernel instead
>>>> of the sFlow info. psample will send this ID and sampled packet to
>>>> userspace. Using the ID, userspace can recover the sFlow info and send
>>>> sampled packet to the right sFlow monitoring host.
>>>>
>>>> v2-v1:
>>>> - Fix robot errors.
>>>> v3-v2:
>>>> - Remove Gerrit Change-Id.
>>>> - Add patch #9 to fix older kernels build issue.
>>>> - Add travis test result.
>>>> v4-v3:
>>>> - Fix offload issue when sampling rate is 1.
>>>> v5-v4:
>>>> - Move polling thread from ofproto to netdev-offload-tc.
>>>> v6-v5:
>>>> - Rebase.
>>>> - Add GitHub Actions test result.
>>>> v7-v6:
>>>> - Remove Gerrit Change-Id.
>>>> - Fix "ERROR: Inappropriate spacing around cast"
>>>> v8-v7
>>>> - Address Eelco Chaudron's comment for patch #11.
>>>> v9-v8
>>>> - Remove sflow_len from struct dpif_sflow_attr.
>>>> - Log a debug message for other userspace actions.
>>>> v10-v9
>>>> - Address Eelco Chaudron's comments on v9.
>>>> v11-v10
>>>> - Fix a bracing error.
>>>> v12-v11
>>>> - Add duplicate sample group id check.
>>>>
>>>> Chris Mi (11):
>>>>     compat: Add psample and tc sample action defines for older kernels
>>>>     ovs-kmod-ctl: Load kernel module psample
>>>>     dpif: Introduce register sFlow upcall callback API
>>>>     ofproto: Add upcall callback to process sFlow packet
>>>>     netdev-offload: Introduce register sFlow upcall callback API
>>>>     netdev-offload-tc: Implement register sFlow upcall callback API
>>>>     dpif-netlink: Implement register sFlow upcall callback API
>>>>     netdev-offload-tc: Introduce group ID management API
>>>>     netdev-offload-tc: Create psample netlink socket
>>>>     netdev-offload-tc: Add psample receive handler
>>>>     netdev-offload-tc: Add offload support for sFlow
>>>>
>>>>    include/linux/automake.mk        |   4 +-
>>>>    include/linux/psample.h          |  58 +++
>>>>    include/linux/tc_act/tc_sample.h |  25 ++
>>>>    lib/dpif-netdev.c                |   1 +
>>>>    lib/dpif-netlink.c               |  27 ++
>>>>    lib/dpif-netlink.h               |   4 +
>>>>    lib/dpif-provider.h              |  10 +
>>>>    lib/dpif.c                       |   8 +
>>>>    lib/dpif.h                       |  23 ++
>>>>    lib/netdev-offload-provider.h    |   3 +
>>>>    lib/netdev-offload-tc.c          | 659 ++++++++++++++++++++++++++++++-
>>>>    lib/netdev-offload.c             |  30 ++
>>>>    lib/netdev-offload.h             |   4 +
>>>>    lib/tc.c                         |  61 ++-
>>>>    lib/tc.h                         |  16 +-
>>>>    ofproto/ofproto-dpif-upcall.c    |  42 ++
>>>>    utilities/ovs-kmod-ctl.in        |  14 +
>>>>    17 files changed, 973 insertions(+), 16 deletions(-)
>>>>    create mode 100644 include/linux/psample.h
>>>>    create mode 100644 include/linux/tc_act/tc_sample.h
>>>>
>>>
>>> Hi Simon, Ilya,
>>>
>>> Can you help review for this series?
>>> do you have any comments you want us to handle?
>>>
>>> Thanks,
>>> Roi