Messages in this thread | | | Date | Thu, 9 Mar 2023 19:57:32 +0800 | Subject | Re: [PATCH RESEND net-next v4 0/4] net/mlx5e: Add GBP VxLAN HW offload support | From | 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@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
| |