Messages in this thread | | | Date | Wed, 8 Mar 2023 10:22:36 +0800 | Subject | Re: [PATCH RESEND net-next v4 0/4] net/mlx5e: Add GBP VxLAN HW offload support | From | Gavin Li <> |
| |
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.
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
| |