Message-ID: <3ed82f97-3f92-8f61-d297-8162aaf823c6@nvidia.com>
Date:   Wed, 8 Mar 2023 10:22:36 +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
Content-Language: en-US
To:     Alexander Lobakin <aleksander.lobakin@intel.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>
From:   Gavin Li <gavinl@nvidia.com>
In-Reply-To: <7612377e-1dd0-1350-feb3-3a737710c261@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NVBiaFlON0ZtTmE1aHNmSjlJdS82TkZwdGgyRzdRKzI1TW41NkJaamdyNGcr?=
 =?utf-8?B?STFSY2lENGdLNlJTaFoyVDlXdlkzYWlrZnl3VWVQK1ZiMWVZWFFwL3p0QmV4?=
 =?utf-8?B?bm51N1VSN3BzL29zUElObjRvNllwRjYwQzJrZEdKa2l1NjBYbFA2ZCtmMVFv?=
 =?utf-8?B?aG9xTXRoSzhDb25uRFJJeHpuTU1SMDROaysvcmJuSWd0Z1JVZ3VXVFFPelV4?=
 =?utf-8?B?THg2Mk0yVmdxZEhXSklSUWEyNE9JR0ErK3c1cTJKY3RJTko3Z0d2eTJwTnEy?=
 =?utf-8?B?TkwzQ0p5V3pWdWZoZlErM0kyV2VvbWg3ZU0zNmtzTE5DYjQ1STV3TC85SXFz?=
 =?utf-8?B?RS96OXlMM0NDbUZMeU01dGxvQlhiQ25KTUtlOFZUOHBNd2tJVlV5Ym5lQ2dZ?=
 =?utf-8?B?THhjazhneW03UkgwQmlFdXBJbFZ4eHpJL3RPN2ZLYUVaSGNEVVVnTXIzdUdu?=
 =?utf-8?B?WjVhTGF3Q0ZvYm5aWUlSamMveVRrTW42VDdva1ppamErdWZoMlpTNXZOYnhI?=
 =?utf-8?B?MWlCQURscVFRVXhwRlZiYmt2dW9iOFNpUmdmQzJ0S0RnTU92ZmhnaEJqR3Qz?=
 =?utf-8?B?YTNacDJQQUp5a3l5LzN0dzE5MTFPeFNiaVVhNnR1Y05QTHFQVEJLU0FxOWlY?=
 =?utf-8?B?dlhMQm4yWkRXeTQ5cm04Wkx4Zm1vaTRoZkRFaGRzYmI3cE0wQTVaRkVXMEdq?=
 =?utf-8?B?TjVYbUtlbkg4dUkwWEVsK3ZjNXBycHA1alluWFRSS1lpdFo1Y0M0SXdSazFF?=
 =?utf-8?B?eTUwaW9XelBHbzBKY2xhWDNpRlllVU1Pa3BxK096MDV3ako3eDIzNW5sS2sw?=
 =?utf-8?B?VnorMWozcHlmZ2x6Vy9LRDJ0ckZBY3RHbzBjTU5qVDlqNC9LcmpvaG9Nd3h4?=
 =?utf-8?B?N1B3K05KczRXd1VmZTV6SVF1V0tFakFRR29vVGVvbm5zbFVlZ3Y4blhEbmli?=
 =?utf-8?B?a25mL2tKczMrZEErWldmbnlrRlM1cjRJRDFVK1c3c0d6cDRsVExWR3NVeG1i?=
 =?utf-8?B?OHpzS1BkV04yd1FuMnlLRVNRUGZZTzhDOG5MNWxwRjNKTlAybFdWS2pmdHBq?=
 =?utf-8?B?RzQ1MjVhQitBVndhWWNzZTQ1TmZOZmF2eWxraS9haXZ6QytUTHl4cmtwcHB3?=
 =?utf-8?B?OE9qSnE3TGk3eXZHQW5IVDlieUFxbUNjTDE3WFMxbkl1SGtmZkR6MTV2UTZr?=
 =?utf-8?B?dUprb256Z2RRbms3ZkY4UmZhUXZHUkYrQXFmMHRldWJKU3R0RG9JS3YvTVN2?=
 =?utf-8?B?SmRFNkJjSnE3eGJtdzZHV1VDK1dCK3pQV3lNa3FMMjVTZ3Q5UHBJNXM1azho?=
 =?utf-8?B?WTBHalV6WnBQS1FlRkRNTEcwYkdkTklWc2VBM2ZJSXU5R2d5OVQ0eEd0Nmdm?=
 =?utf-8?B?dy9wc2ZSNVREV2dmM0RzbUZpTGNwQVFXUFp6d0pOQW0xZ0phblg0L0l5MFpa?=
 =?utf-8?B?eHlHSHhLeGhzNTJlTkEwSjBOUi9oTGJOMXZaWEtmdElLS2lpTENlWFBtbzBi?=
 =?utf-8?B?eEg0NTROTmxlVHF6MFc2V0hjcXUwdWNzS3gvM2xPT1hFaG9MMnRSRUxUbkgw?=
 =?utf-8?B?eHRVM0NIWm9QQW5qVjVJSGcyNUh5ZEs4L3RKTWo5R1BnaEJlSE01OTN3TUta?=
 =?utf-8?B?VkNTT0RmcDNITDZGajM2Y2dFblIvcklDNVA2Q2cyQW9VWC8wcFdoejZ1Wlha?=
 =?utf-8?B?YXZFTEkzcmpPTUxlVUNEOVFCbDA5VTZDVnhQMjF1WFZDTDlpUGRkUlQvZkln?=
 =?utf-8?B?NmpuLzIrMmVvTVdEdmhESE1aQUttNVpMa25wWjVJMjl5QTRuVHBOWVJjZ05Q?=
 =?utf-8?B?cDZNdXVBR2luWWxCNWgvT1hxOUZxcVhaQTRteFFzdjRONnl4YU9DRU53Q2hK?=
 =?utf-8?B?OWh5eXNnRVFqRUs1ZWNCa1c2a2ZsTVpFYVRwVCtRaEk5RG8yUGE2VStrb011?=
 =?utf-8?B?QXdTMzBSV0N5TXpubWpxdSs0VUppQWpxOEs2QTdJd2xob1V0SHh1WDVNYW5z?=
 =?utf-8?B?cFhKb0EvaFdhalFJVHF3NjkvRStiV0l0RlNPdnEzYklMQ0dkVzhpVzJQbC94?=
 =?utf-8?B?eGJnWVFQMWJRMHFraXJqbGE5TE9LQU1WcGNlWGRlVHB3d080c1RJRHBOVGt2?=
 =?utf-8?Q?/sIh1dqacHU4aeYkhEkmPBYpf?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 79095150-0ff9-4064-2959-08db1f7c05fa
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3835.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Mar 2023 02:22:54.4586
 (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: 4Jxn5mbq/hrXZIVo9mAQZPKX7+0ICP1Iyz7PuX3/GYbKG9mRaXPYkI8LK6qBLdYqI7djitHptgNsLvFDraMKcw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB7214
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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.

 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.

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