.../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(-)
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
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(-)
--
2.31.1
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. ??? > 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
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. >> 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
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? > >>> 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
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
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
On Wed, Mar 08, 2023 at 02:34:28PM +0100, Alexander Lobakin wrote:
> 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.
I'd like suggest moving on from the who said what aspect of this conversation.
Clearly there has been some misunderstanding. Let's move on.
Regarding the more technical topic of constness.
Unless I am mistaken function in question looks like this:
static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
{
return info + 1;
}
My view is that if the input is const, the output should be const;
conversely, if the output is non-const then the input should be non-const.
It does seem to me that container_of_const has this property.
And from that perspective may be the basis of a good solution.
This is my opinion. I do understand that others may have different opinions.
From: Simon.horman@corigine.com <simon.horman@corigine.com>
Date: Wed, 8 Mar 2023 21:13:31 +0100
> On Wed, Mar 08, 2023 at 02:34:28PM +0100, Alexander Lobakin wrote:
>> From: Gavin Li <gavinl@nvidia.com>
>> Date: Wed, 8 Mar 2023 10:22:36 +0800
[...]
>>> Any comments, Simon?
>>>
>>> If both or you are OK with option #1, I'll follow.
>
> I'd like suggest moving on from the who said what aspect of this conversation.
> Clearly there has been some misunderstanding. Let's move on.
>
> Regarding the more technical topic of constness.
> Unless I am mistaken function in question looks like this:
>
> static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
> {
> return info + 1;
> }
>
> My view is that if the input is const, the output should be const;
> conversely, if the output is non-const then the input should be non-const.
>
> It does seem to me that container_of_const has this property.
> And from that perspective may be the basis of a good solution.
>
> This is my opinion. I do understand that others may have different opinions.
Mine is basically the very same :)
Thanks,
Olek
On 3/9/2023 4:13 AM, Simon Horman wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Mar 08, 2023 at 02:34:28PM +0100, Alexander Lobakin wrote:
>> 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.
> I'd like suggest moving on from the who said what aspect of this conversation.
> Clearly there has been some misunderstanding. Let's move on.
>
> Regarding the more technical topic of constness.
> Unless I am mistaken function in question looks like this:
>
> static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
> {
> return info + 1;
> }
>
> My view is that if the input is const, the output should be const;
> conversely, if the output is non-const then the input should be non-const.
>
> It does seem to me that container_of_const has this property.
> And from that perspective may be the basis of a good solution.
>
> This is my opinion. I do understand that others may have different opinions.
ACK
© 2016 - 2026 Red Hat, Inc.