Re: [PATCH RESEND net-next v4 0/4] net/mlx5e: Add GBP VxLAN HW offload support

[Gavin Li]

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@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > > > 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@xxxxxxxxxx>
> > > > > > 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