| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3ed82f97-3f92-8f61-d297-8162aaf823c6@nvidia.com>
Date: Wed, 8 Mar 2023 10:22:36 +0800
From: Gavin Li <gavinl@...dia.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>,
Simon Horman <simon.horman@...igine.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, roopa@...dia.com,
eng.alaamohamedsoliman.am@...il.com, bigeasy@...utronix.de,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
gavi@...dia.com, roid@...dia.com, maord@...dia.com,
saeedm@...dia.com
Subject: Re: [PATCH RESEND net-next v4 0/4] net/mlx5e: Add GBP VxLAN HW
offload support
On 3/8/2023 12:58 AM, Alexander Lobakin wrote:
> External email: Use caution opening links or attachments
>
>
> From: Gavin Li <gavinl@...dia.com>
> 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@...dia.com>
>>> 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?
I had fixed it with "cast-away in THE GENERIC INLINE" in V3 and got
comments and concern
from Simon Horman. So, it was reverted.
"But I really do wonder if this patch masks rather than fixes the
problem."----From Simon.
I thought you were OK to revert the changes based on the reply.
From my understanding, the function always return a non-const pointer
regardless the type of the
input one, which is slightly different from your examples.
Any comments, Simon?
If both or you are OK with option #1, I'll follow.
>>>> 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@intel.com
> [1]
> https://lore.kernel.org/netdev/ca729a48-35a1-ef05-59d3-ef1539003051@nvidia.com
>
> Thanks,
> Olek
Powered by blists - more mailing lists