Message-ID: <7612377e-1dd0-1350-feb3-3a737710c261@intel.com>
Date:   Tue, 7 Mar 2023 17:58:49 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; 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
Content-Language: en-US
To:     Gavin Li <gavinl@nvidia.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>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <1abaf06a-83fb-8865-4a6e-c6a806cce421@nvidia.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aXJrRU0rWTllODFQVnl1OHgyOXVpRTZhQ1pRRFJEQVdTN1hKMVVyK1lKRldH?=
 =?utf-8?B?NTZHT01JUnoycTFXc3BITE5UU0N0aXptNGhyME9nbzNxMWJXSGZveXFNOHdI?=
 =?utf-8?B?RCtqZlVJNmFsUUcwVnB5WTU5MnZsaHpPV1hQQ2owNDBWRTRiT2xuSG50bWVD?=
 =?utf-8?B?blU2TmVSZG81Q0VicUk1OElYZFJxSVRJYzhCTmJoTFczTDdaTEtMYVFMbFpm?=
 =?utf-8?B?NEQyNWxtbDYxb29kRnhYcklQK2ExVnZtck5jK3VLWWZCWllKc1d4Z3BVTnRR?=
 =?utf-8?B?U2pKUnV3S1ZVQ3JQRDUwY0c1b085QU03bzB5NVUxeERPdDJSZW0vK0pHTUdx?=
 =?utf-8?B?d3lJbVo1SjlVL1JPcXJsOFc5OU5mZEd1TEtTamtTNllFbjhTZ3hJS2ZPdG0z?=
 =?utf-8?B?VU5ONlBCMjMwZ3VwV3NSbHZMZGZUWGNwQWx0RVJRK0NGWmhTOGxDNVk3cnlT?=
 =?utf-8?B?ZUdOajRheVY0eUEwdk9mbXRMbGQ1dm9PTFl0SVV0bDZNZllacWpwbWRpRTU1?=
 =?utf-8?B?bHhDU2I5RHdXWFA2ZEYrNEZOU0w4NXRweGVDZStDa3hzNlZXUWF4OFpCcEpG?=
 =?utf-8?B?YmYwejZTUHUxSEsxYUZaeDVXSWlQQ2lJYzI0TXdFZ1hpR1RXNkNoR281Si9s?=
 =?utf-8?B?ZEVTU0FyaHRRNXFOSm42WjhPZTBNZzI2amhORXhxVVRhZlp1L1R4Rks0b3o4?=
 =?utf-8?B?VlJXZmRjR2ZFcGJLblFDUjQxZDAxY3RCZTJqWm5sTVo0NFkrdmZFbVcvRXlr?=
 =?utf-8?B?TWdva2F2VGhBMTZ2eXVsUXU0T29vQkl5RGJUU29PUlFMVHQ4R1RPaGNTUmFD?=
 =?utf-8?B?RWxSYTJkczlaM1dOcTVFclA5c1c4c1ExUW4xVXRSZUNxOGpRZWlDbnpGRkxm?=
 =?utf-8?B?NVlBOVJ5M0RHaEsyREJaWWdtc3BFc3FzMm56YmlkdDNrdzVVeC9pSEtmeUFN?=
 =?utf-8?B?NTVzTko3R282NXI3RGw0TGhRZXArbDhLR1FMcmQ0dHlKUW5lYm9GY0ZYT3Rs?=
 =?utf-8?B?TWJDaEthMUdLZTZvOUo0RERKYWRrZ0FQSmdqdXc4TmV5L2Zaam56M2JZUXhP?=
 =?utf-8?B?Wld1U2pnU3MzSDlPS2RDUnBHUkhLb3dLWU5lSlZkQnAzWStZOGlBWWNYSXhh?=
 =?utf-8?B?RExGbE56SG1tYk9oNWxFdnNSNUZoUEtxS0VHN2hOSHR4dTFxUW0xSG84TkZr?=
 =?utf-8?B?bTN0cTE0TTVIMnJ3TnhvVHg3bnpPT1RsdVp2aUIwT3Zhak40cEc4RnhyS0F3?=
 =?utf-8?B?YVlvQ3MwSkovV2dYQTU5YTh3M3dsQ3JJSjIwM0NFbFhBRWhtb0FaendiVFB4?=
 =?utf-8?B?QUlzdzVIa0NFVzNva2RHNEVNdlAzZEEreGsrTFpqWkkxV0xUMFBjWGp4OHNP?=
 =?utf-8?B?WThIWTdVVDBJTVNHNUVsTU5CZFY5ZmxTS1dXYm5zendxRWFpYUlEV0ExTC96?=
 =?utf-8?B?TzZnRTVRZ2lsT0ladEJzUDNTSmJwTFpqbXVXRjFjaVorOFd5RXBMRy9pWi93?=
 =?utf-8?B?dnp3TEtnSDRZanZlaGxnZmZoNDRPalB4SXJ5RUxhbHRLTzZtRjB1TTZscGw3?=
 =?utf-8?B?US83ZWwyYUdFbFczYnJicEhhTDFRaHBYV1FUN3FjL21idnVNYTlKSG8zYk9M?=
 =?utf-8?B?d25Bc2FDUlpwSHIxYXVlVEhMWk1NU1p0Q0xGQzdSV2lpQTgyeXBFTEp6cjRn?=
 =?utf-8?B?OU5mUGRrMXNkWCtqc0pUUm92TVQ2QXFXRjlRYkNBS3k1d3JUWWl1SkNQVC9l?=
 =?utf-8?B?aW1pSEpuWC9peWNuYjVuODdPV2hRQm14eG51SlZWNGdnMUxLdVlDOFY1ZGRm?=
 =?utf-8?B?dDFEdGcybzE3Uy9EVWgwTjhmUnJMUkVyNytFMnZKTjdKSGxpNCs3aGdrL2tz?=
 =?utf-8?B?RGN3RE5kaGtlWjN6V0JSNzhEVW1XTitHelFidCtEMHdjWEEwNEE2K0N5eEpz?=
 =?utf-8?B?MUZNTHJIYkVxa2ZiMVZMVjNNdmxpeHpuMFJCekg1bmlLbGM3WUxjMCthK2pB?=
 =?utf-8?B?N3REcmpCbVEzQ3RhZUl0MFVJSHM4bWlzWWczRXlvaTJqL3pDckpkUmFHQllz?=
 =?utf-8?B?aC9aVVVBeXA0VFUxd0c3VkFaYjhEb1dXRGZWRFphK0gwN0N2bDRkVis4cDZS?=
 =?utf-8?B?YUNqcEdwd2dsWUZIbDZxcXZOVkVGa2dxcEVqWFoxSGZ1OVhDK2ZwbzJhYkdX?=
 =?utf-8?Q?fvJZdoTHBlugLcicCiFHZLc=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ef72c26d-b993-4d3b-f0bc-08db1f2d56ab
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Mar 2023 16:59:39.3811
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 9X0FiCBVBqrNwvwje5afQM9Jh3dMAzX8Of0aJwSUBV/gUcR9xdnPtexTLzlxoxTxhzJUQdztXrbwBL5HPglt6jFYHCLL4z64KQg/m4HWuLw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5199
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?

> 
>>> v2->v3
>>> - Addressed comments from Alexander Lobakin
>>> - Remove the WA by casting away
>>> v1->v2
>>> - Addressed comments from Alexander Lobakin
>>> - Add a separate pair of braces around bitops
>>> - Remove the WA by casting away
>>> - Fit all log messages into one line
>>> - Use NL_SET_ERR_MSG_FMT_MOD to print the invalid value on error
>>> ---
>>>
>>>   .../ethernet/mellanox/mlx5/core/en/tc_tun.h   |  3 +
>>>   .../mellanox/mlx5/core/en/tc_tun_encap.c      | 32 ++++++++
>>>   .../mellanox/mlx5/core/en/tc_tun_geneve.c     | 24 +-----
>>>   .../mellanox/mlx5/core/en/tc_tun_vxlan.c      | 76 ++++++++++++++++++-
>>>   drivers/net/vxlan/vxlan_core.c                | 27 +------
>>>   include/linux/mlx5/device.h                   |  6 ++
>>>   include/linux/mlx5/mlx5_ifc.h                 | 13 +++-
>>>   include/net/vxlan.h                           | 19 +++++
>>>   8 files changed, 149 insertions(+), 51 deletions(-)
>>>
>> Thanks,
>> Olek

[0]
https://lore.kernel.org/netdev/aefe00f0-2a15-9a43-2451-6d01e74cc48a@intel.com
[1]
https://lore.kernel.org/netdev/ca729a48-35a1-ef05-59d3-ef1539003051@nvidia.com

Thanks,
Olek