Message-ID: <531bac44-23ba-d4f3-f350-8146b6fb063a@intel.com>
Date:   Wed, 8 Mar 2023 14:34:28 +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>,
        Simon Horman <simon.horman@corigine.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>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <3ed82f97-3f92-8f61-d297-8162aaf823c6@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?ZHhYNTRMcG1adTFWcXJudU91eGMvYkJtU01LV0FnV1hFdWMwKzBOWlpLaDJz?=
 =?utf-8?B?eXJYdjBEc3BqTzd6TFhLb3pSdGlGcHFydklTZFBDc3VzS3VVVU55YWx6Z0wz?=
 =?utf-8?B?V1FuL0JZUzVmYkpMOEloZjJ5WXYwTTNwdEcyZk1xV0dwRUxUaVdIdm9CYysr?=
 =?utf-8?B?R2plUFJuSVR4YXRuZU93Q2RFNUZhVDUzckNmbE9ISjZ2cHpZN056OVUyUUNJ?=
 =?utf-8?B?ZytDMStTM0swU05sKzA0NCtFQ1B1ZFEydkZiNEcwQkdJZ2tqanVPZE9mcHdB?=
 =?utf-8?B?cnJQY0F6LzEzT1V0V2k2TlVzTC82Qys4dTF5RHhhaVBjaXNzU2NoRUJydG84?=
 =?utf-8?B?YURyVzlSeDZMWklQS3NLdzgxQW44K0FOclhIbGtTMDd0Wkk5SHYzd0JxMUhR?=
 =?utf-8?B?UFJkVWFPT3J3dHRUenhOZmhQOVZRZ214NSsxRGpHVFVYeCtxd2R0UTNKeVFx?=
 =?utf-8?B?RVhQL0t6dUZibVJEQ05RZHEyS3NER3NUQzk2bUZyejdrT0FaZStFeklnMWlu?=
 =?utf-8?B?bFgyejg5cE9iZWEyYlJHa0JLRzZvN0RUU1pDSUNQS2IrK0g1d01QeU5hcXN1?=
 =?utf-8?B?WTkvdDkxNS9hUm53YnUzaWFBbG9RcEJoRnNnY29IenRjV3grM2VtOUZodklq?=
 =?utf-8?B?eGx1c1JuOWNESlI0MStCc1d4am9MY2Q0STRRbWgrQW1Gd0JGRHhWaHZjQS9M?=
 =?utf-8?B?ekZEZGtLSzFMMzFSUnk1U28wSnB6UTJWOWFxd2U3MFM4M2JVN1IweXowRzVa?=
 =?utf-8?B?VUYxQkNJZDV2YUMzM2VhMURhclo5bFVqREYzUzVkQko1K2ZETTVtZmVLM2JO?=
 =?utf-8?B?YmZDYXR6VkZSZlJxdTRyTWZXTm95cGFDS0ZadWRTVEtUT3hDd2JLdnFKSG41?=
 =?utf-8?B?bHo1L3IvSGd6MlY1UU9XK2RXMnZuQTY5MlZzVGg1VEUzbHBEOVM5ZTFjNzNI?=
 =?utf-8?B?Q1A3SDJSekxPR3lnYTFNRjlaeTRuYk1pRGlnUGl2VVIvb1BBait4UHZTMmFB?=
 =?utf-8?B?OTFsRGd6d1RZK1o5ZklnRzlKYW9ZZGh1R0FaV0xZWGRSUTdFZ05vdWIrakxP?=
 =?utf-8?B?T1UrTHhpOFREN2FyNlNOWnpIbFNoUmZ1OUxxOWcxUnFEbnlJQVE5NkFXcHFC?=
 =?utf-8?B?cXM5WDh0Q09DaHo4WkdPM0FOSnVScEx5cTlsS2RXbC9reHRqY3RsL210T3FR?=
 =?utf-8?B?MjA3TythM2EwaTNHbkRraVltOW5uSngxY25YQXZ3RUgwS1lCNFNpSmFrZXp6?=
 =?utf-8?B?NzB6RmUrRnh2YjdndnhSZDA5Wm4zQ0JPTUVWb1RXTmtTQkp0L3BEcG1WdnRY?=
 =?utf-8?B?dlR2QTE0NnZHaGJReVpSd2xWNlNZZnVjMkN5Ykl3WkY3dFFLS0QyV0JyK2gv?=
 =?utf-8?B?L25rajdWYWUzVENVdnJLUDdiQkNWMlJjWFRESTA4N1FVMEZkTHRaZXVhaSs3?=
 =?utf-8?B?NzZOd2dLS0s3M2pLSlpERWxSV3YvT2NaVnRXME5vM0tGVUsydFYvSUZ2NS9r?=
 =?utf-8?B?WHJsN3pMMmVkeDVKM2FreWYvcUk5WHV1dVNHeEgzeTJCOVNWQUNtbHJyV0Iw?=
 =?utf-8?B?Tk5lTVMwQlJZcExoRmgra25mN090YjFodkQ4NmxEWHRiYnhocEFQcUZGMEZa?=
 =?utf-8?B?SnIwd2p1SFVVVzFsemppVllycXlMTUlOTXdKaFlMb2swTDNHZ2ZVcjhEYUpO?=
 =?utf-8?B?VWRib0xGb2poZ3VabFNrUlV0Z0NDWGtXSTlPakRnSmsvVzNvV2hWWVBQclhG?=
 =?utf-8?B?dFlmVTcwNFZueHcrU05wNXJEVnJuSWxLL0RsTXpRQ1ozdGE1TkJCdEszaThY?=
 =?utf-8?B?MDhLM0x5MEcxY08zSENMOXEwU0xQaXF0TU5GY3pSRjRxemtIY2k2MUhIR3Jw?=
 =?utf-8?B?QTB5U0d0TStaODg1L3JaeDVRWWRnVXB5S0M1d3AzTWJCMnNycjNFMGdZL0M1?=
 =?utf-8?B?NCtNaXB3ZXF3bmhDbStNYkhWZHc5d2UvUXVCNGhWaVMwLzdiUWFDc2ViRkFC?=
 =?utf-8?B?MVMwQXlBZXA3akczenE1dnd4VCtjVWh3Lzc1MlJGem56eXNtT0dNb0N5TUwr?=
 =?utf-8?B?b01mRHFXTXNJeUJLZnNGa3pWV0RkUzRLZXdVRkFxeGt4TmdTb1pUWkR0MUFi?=
 =?utf-8?B?akZFTmdwV1I2T1prSVF1b3Q2bEdZdUJPRWZ1TFMwMGNPSkZ5VWkyRGRpd3dO?=
 =?utf-8?Q?Jnw/1pka2NgJN9/xuRwP7vk=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5c808cfa-975f-4c87-c155-08db1fd9f962
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Mar 2023 13:35:25.6982
 (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: FYOGHzjpEU783XnW3fABSjerka93+mEOdzh2pmao3/ZV2PBlk+h+4J3xGus8xKt95RkY/KF/zkOxLyPvefUUr+nGNZk3ykN6Zr/vTS6c6T4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7097
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: 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.
> 
>>>>> 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

Thanks,
Olek