Messages in this thread | | | Date | Wed, 8 Mar 2023 21:13:31 +0100 | From | Simon Horman <> | 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@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.
| |