Message-ID: <ZAjsa538mpnEQ/QI@corigine.com>
Date:   Wed, 8 Mar 2023 21:13:31 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Alexander Lobakin <aleksander.lobakin@...el.com>
Cc:     Gavin Li <gavinl@...dia.com>, 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 Wed, Mar 08, 2023 at 02:34:28PM +0100, Alexander Lobakin wrote:
> From: Gavin Li <gavinl@...dia.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@...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.
> 
> 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.

Powered by blists - more mailing lists