Message-ID: <6b7be975-c9e1-5833-9030-be87e713ecf5@nvidia.com>
Date:   Thu, 9 Mar 2023 19:57:32 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH RESEND net-next v4 0/4] net/mlx5e: Add GBP VxLAN HW
 offload support
To:     Simon Horman <simon.horman@corigine.com>,
        Alexander Lobakin <aleksander.lobakin@intel.com>
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, roopa@nvidia.com,
        eng.alaamohamedsoliman.am@gmail.com, bigeasy@linutronix.de,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        gavi@nvidia.com, roid@nvidia.com, maord@nvidia.com,
        saeedm@nvidia.com
References: <20230306030302.224414-1-gavinl@nvidia.com>
 <d086da44-5027-4b43-bd04-29e030e7eac7@intel.com>
 <1abaf06a-83fb-8865-4a6e-c6a806cce421@nvidia.com>
 <7612377e-1dd0-1350-feb3-3a737710c261@intel.com>
 <3ed82f97-3f92-8f61-d297-8162aaf823c6@nvidia.com>
 <531bac44-23ba-d4f3-f350-8146b6fb063a@intel.com>
 <ZAjsa538mpnEQ/QI@corigine.com>
Content-Language: en-US
From:   Gavin Li <gavinl@nvidia.com>
In-Reply-To: <ZAjsa538mpnEQ/QI@corigine.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Y3VyZW9PTkN3aGxNWDA5U3lwaGRvVDBJUzFsOUp1NnlKK3MydHlBRCtXU0Ez?=
 =?utf-8?B?K3pQY3h6aE1pMVhheDJpU00xZUUxcEh6MHFKUVNVbkJJZU5yUmlCWDlRYmFt?=
 =?utf-8?B?YldsOFoxQXcvWjVNeUxuSndpL2xEdTZ6VlpLZElVY05pZlhhOW1hT1Q5UzQ4?=
 =?utf-8?B?alljdFFMWCtiM3pVa0NHUUtENDV6TXl1RGNodThNYWZGaFdQSGEyUkh3SEFN?=
 =?utf-8?B?YTNqcDYwNnRBVlp0WGdCVCsvbnU5S2lmbTVyeERaY1EvYk8wbWhiN05qTkov?=
 =?utf-8?B?Y2k3SlByazAyUEJhcTJjMTlOUWUrbU5VOGF4V1VjMWRST3poMUowYjZNZFhk?=
 =?utf-8?B?KzNZbmhNWHdzcjcxVUhHUGRkTWdDVGxTMmJ6OWcyOEs5VjhlYUI2NzVDckIy?=
 =?utf-8?B?UG9DZEJXOGk3ZWpxa0FNRVZFQzVhdWNhRW1WNDNNODVKMXovZ1A4SWZXOGxI?=
 =?utf-8?B?S1JSOFZVK3pEdXNiUGlkVWpRc05XcFBWVnNPMEU5S25VZm9mQ09aUEtkcFVm?=
 =?utf-8?B?UFJTK3o3UVJnNUkwS3k3bDJ6dWtUbVE5OHZTbFhxTEgzUUgzMC9XbUJvVjd5?=
 =?utf-8?B?cTR6Y1ZhYjUzTm05bzZQZi9lQzVTOWhDdldXazhzeFFpamFOVk9zU0xvb3Ri?=
 =?utf-8?B?OFpVeXd1L1Z2UkRmd0FhRUJTeDdqL0dZRGtYZ2xvVU4zYitJR0dMOUtGT2ww?=
 =?utf-8?B?eU1ueXErdGNqaGVqTW0zeHplQXU3WE9lZDNNd09XaCtJck8zMUIxZUI0UmJJ?=
 =?utf-8?B?aGpwMzVtT3Vka0YrZDlWc3ZtWit6bGRWOUVVaGdqNGZvdElzdnhWTDRnWjBn?=
 =?utf-8?B?ZzlZZ2ZQaERhL2VjRG1ac2hPaXFSRTd1QVdFZWEzWVp2V1JtQ09raVRzNjBh?=
 =?utf-8?B?VHRSWXNXUnEvOU5mbENia2hpY0tJR29obWJjRXpvR0dGTU90WlJZUHJlcHE2?=
 =?utf-8?B?eUdWTWk5MUMzRkl5c1J2dTRWZ05kcUwwbnByNVVzSUwrRjNyMHpYZlpKcTVG?=
 =?utf-8?B?QnhCQURNeTdFM1ZtY2NuNXM2VzZaeS83UnNYcDJYQWh2cmxIcGdxQkl0dXNJ?=
 =?utf-8?B?aVdwOGpqbTRYcVljanZqZkorSlR2Z3MveGszNGhxYmwrUE5SaHBXTEhBRFdB?=
 =?utf-8?B?MHJXeE1LYUx3MkQ4KzQ3VFNpdDhzSjlWck56TmlqUlpsNENxUDZwajRKUXNq?=
 =?utf-8?B?anU2VjlaeThHSVI2a1NpakhPK0g0cVQ3dUpFTUltZkxCdVBPRUsreUpPVndp?=
 =?utf-8?B?QVdHdjRVcHRhRTFqNHFkWGwwcTRQOHdIaktQVFFUdVFyK1AzTmc4UnNOQjd3?=
 =?utf-8?B?WWt2RzM0ZTFZcHJzZHlVNVhLbENiOGZOOVBqVisra2gzTDlnSTlpc0ppaDlq?=
 =?utf-8?B?MmRjVU14NlAydnoySUVyVkxWcEl5S1Z3cWZxMGIzUHJYNHVFTFJZYUZqUjVh?=
 =?utf-8?B?dlZoUmJIV2VGQVA2Q1QrYUFUbDg2UHR3ZS9lVjgzM2dPWml3WkNDZnFTU2Fs?=
 =?utf-8?B?cjBVQlZLZVRqODRyQVhRRjl4dDJHU3kxSTN6ZlpWbm1jR2UrSjhQM2lSODZr?=
 =?utf-8?B?MGQ2cUlva1QxdkROcE9ESWVidjBEcFdRNGlIUnpYd0FDUXlpVUh5dXZXditW?=
 =?utf-8?B?dTFnQ3loYU5LT21EYUZUMUovekZKV3dwVVFYYXU2WEJ4SzhIK0tHcjZITENr?=
 =?utf-8?B?UnZHVkxxQ0FYRjJCeGFuczV4UUhVTGt6QUh2WTlwNnJ6T0NES1NFMU5HSzJX?=
 =?utf-8?B?WjZSakpDTys1NGdLMklRRHhMazllN2pDakZzaTIrVk5UaXpNczRMemgxbld3?=
 =?utf-8?B?RjBWZU1jM3Jkdkw0VTgxTDBueXJncmxUN1lGYWJXVjdqU2lzOU15Q0ZBTVMy?=
 =?utf-8?B?VGhNQTZiNzN2NFNRWklxdHRtd1g5eGdzMVdGZjU2UzJ6aExKUzQ4MHFTa0Za?=
 =?utf-8?B?QURFVXRxM3VXNThWZ2NFclpLZkMycVFGS2NuQ0hDamlxZlc4VjFWM0o2Q2xP?=
 =?utf-8?B?dEN5WDZ5UjBWdHFrNThMRWdDRkhFQWEzVnlSVzJ2eFFKMDh0QW5FbVVwSWl2?=
 =?utf-8?B?ZVVkU0tjNUJuR24yeG5xTkhMT1lZYVRYeTJ2ZXRvNFkzTE9ZblluQmZtanht?=
 =?utf-8?Q?IladgqizwfDmbDpqDJIUSW85V?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 301b35d8-31d3-4047-db72-08db20957fac
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3835.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Mar 2023 11:57:47.2151
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: v9oISjMtPI8oo/NzGhD0+y1DuYNQkDZx3euRmls/qAZemfiQHZbBLVUPjKIaJj0IDCAoKO7KY3sagfDPvLqtXA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB6087
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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