Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 8 Mar 2023 14:34:28 +0100 | Subject | Re: [PATCH RESEND net-next v4 0/4] net/mlx5e: Add GBP VxLAN HW offload support | From | Alexander Lobakin <> |
| |
From: Gavin Li <gavinl@nvidia.com> Date: Wed, 8 Mar 2023 10:22:36 +0800
> > On 3/8/2023 12:58 AM, Alexander Lobakin wrote: >> External email: Use caution opening links or attachments >> >> >> From: Gavin Li <gavinl@nvidia.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@nvidia.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.
No I wasn't. Yes, it masks, because you need to return either const or non-const depending on the input pointer qualifier. container_of_const(), telling this 4th time.
> > 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.
See above.
> > 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
Thanks, Olek
| ![\](/images/icornerr.gif) |