Re: [PATCH RESEND net-next v4 0/4] net/mlx5e: Add GBP VxLAN HW offload support

[Alexander Lobakin]

From: Gavin Li <gavinl@xxxxxxxxxx>
Date: Tue, 7 Mar 2023 17:19:35 +0800

> 
> On 3/6/2023 10:47 PM, Alexander Lobakin wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: Gavin Li <gavinl@xxxxxxxxxx>
>> Date: Mon, 6 Mar 2023 05:02:58 +0200
>>
>>> Patch-1: Remove unused argument from functions.
>>> Patch-2: Expose helper function vxlan_build_gbp_hdr.
>>> Patch-3: Add helper function for encap_info_equal for tunnels with
>>> options.
>>> Patch-4: Add HW offloading support for TC flows with VxLAN GBP
>>> encap/decap
>>>          in mlx ethernet driver.
>>>
>>> Gavin Li (4):
>>>    vxlan: Remove unused argument from vxlan_build_gbp_hdr( ) and
>>>      vxlan_build_gpe_hdr( )
>>> ---
>>> changelog:
>>> v2->v3
>>> - Addressed comments from Paolo Abeni
>>> - Add new patch
>>> ---
>>>    vxlan: Expose helper vxlan_build_gbp_hdr
>>> ---
>>> changelog:
>>> v1->v2
>>> - Addressed comments from Alexander Lobakin
>>> - Use const to annotate read-only the pointer parameter
>>> ---
>>>    net/mlx5e: Add helper for encap_info_equal for tunnels with options
>>> ---
>>> changelog:
>>> v3->v4
>>> - Addressed comments from Alexander Lobakin
>>> - Fix vertical alignment issue
>>> v1->v2
>>> - Addressed comments from Alexander Lobakin
>>> - Replace confusing pointer arithmetic with function call
>>> - Use boolean operator NOT to check if the function return value is
>>> not zero
>>> ---
>>>    net/mlx5e: TC, Add support for VxLAN GBP encap/decap flows offload
>>> ---
>>> changelog:
>>> v3->v4
>>> - Addressed comments from Simon Horman
>>> - Using cast in place instead of changing API
>> I don't remember me acking this. The last thing I said is that in order
>> to avoid cast-aways you need to use _Generic(). 2 times. IIRC you said
>> "Ack" and that was the last message in that thread.
>> Now this. Without me in CCs, so I noticed it accidentally.
>> ???
> 
> Not asked by you but you said you were OK if I used cast-aways. So I did
> the
> 
> change in V3 and reverted back to using cast-away in V4.

My last reply was[0]:

"
You wouldn't need to W/A it each time in each driver, just do it once in
the inline itself.
I did it once in __skb_header_pointer()[0] to be able to pass data
pointer as const to optimize code a bit and point out explicitly that
the function doesn't modify the packet anyhow, don't see any reason to
not do the same here.
Or, as I said, you can use macros + __builtin_choose_expr() or _Generic.
container_of_const() uses the latter[1]. A __builtin_choose_expr()
variant could rely on the __same_type() macro to check whether the
pointer passed from the driver const or not.

[...]

[0]
https://elixir.bootlin.com/linux/v6.2-rc8/source/include/linux/skbuff.h#L3992
[1]
https://elixir.bootlin.com/linux/v6.2-rc8/source/include/linux/container_of.h#L33
"

Where did I say here I'm fine with W/As in the drivers? I mentioned two
options: cast-away in THE GENERIC INLINE, not the driver, or, more
preferred, following the way of container_of_const().
Then your reply[1]:

"ACK"

What did you ack then if you picked neither of those 2 options?

> 
>>> v2->v3
>>> - Addressed comments from Alexander Lobakin
>>> - Remove the WA by casting away
>>> v1->v2
>>> - Addressed comments from Alexander Lobakin
>>> - Add a separate pair of braces around bitops
>>> - Remove the WA by casting away
>>> - Fit all log messages into one line
>>> - Use NL_SET_ERR_MSG_FMT_MOD to print the invalid value on error
>>> ---
>>>
>>>   .../ethernet/mellanox/mlx5/core/en/tc_tun.h   |  3 +
>>>   .../mellanox/mlx5/core/en/tc_tun_encap.c      | 32 ++++++++
>>>   .../mellanox/mlx5/core/en/tc_tun_geneve.c     | 24 +-----
>>>   .../mellanox/mlx5/core/en/tc_tun_vxlan.c      | 76 ++++++++++++++++++-
>>>   drivers/net/vxlan/vxlan_core.c                | 27 +------
>>>   include/linux/mlx5/device.h                   |  6 ++
>>>   include/linux/mlx5/mlx5_ifc.h                 | 13 +++-
>>>   include/net/vxlan.h                           | 19 +++++
>>>   8 files changed, 149 insertions(+), 51 deletions(-)
>>>
>> Thanks,
>> Olek

[0]
https://lore.kernel.org/netdev/aefe00f0-2a15-9a43-2451-6d01e74cc48a@xxxxxxxxx
[1]
https://lore.kernel.org/netdev/ca729a48-35a1-ef05-59d3-ef1539003051@xxxxxxxxxx

Thanks,
Olek