Message-ID: <aefe00f0-2a15-9a43-2451-6d01e74cc48a@intel.com>
Date:   Thu, 16 Feb 2023 16:19:53 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH net-next v1 3/3] net/mlx5e: TC, Add support for VxLAN GBP
 encap/decap flows offload
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>,
        Roi Dayan <roid@nvidia.com>, Maor Dickman <maord@nvidia.com>,
        Saeed Mahameed <saeedm@nvidia.com>
References: <20230214134137.225999-1-gavinl@nvidia.com>
 <20230214134137.225999-4-gavinl@nvidia.com>
 <711630a2-b810-f8b0-2dcf-1eb7056ecf1d@intel.com>
 <231a227d-dda6-fe15-e39a-68aee72a1d59@nvidia.com>
 <92d83584-2238-f8e8-3ed6-f292223e4061@nvidia.com>
 <dcc5578e-89d1-589f-3175-eb8bcd58f7ec@intel.com>
 <76b74086-ea65-7bb3-1eb0-391f79d1c615@nvidia.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <76b74086-ea65-7bb3-1eb0-391f79d1c615@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?N1VoQzBaL2dBWnNRQVFjWDhoTHB4VnV0K0tURU5HczNRZ2tDbXFqblhpbysw?=
 =?utf-8?B?bDcwaExsY1pQcEhFUmNKVzhmUDhjNi81aDlHMDVOcXN0d0lRMEptTU9SS21H?=
 =?utf-8?B?K0lSeU5WY0Z4RHZlWEllNVBzME14ODZKOWFLZlE2R2YwdjF5ak84T3c2ZlBu?=
 =?utf-8?B?OCs0WDFLM1hzRDd1cU1tTDNycUc0amQyUmV0K3lWVEFkOGdkRUpxRW5jREJM?=
 =?utf-8?B?aGQ1QUZDemlCY1pBQ1RZZlZqbjVzUndhb1o1VjVWS1VULzNPU1htc0RRUklq?=
 =?utf-8?B?bDVHUHdScUpOYWx1UUFVZ3BQeXdGL2ZqRDFjRkVqL2tFcDRvTGZPY3QvUWxs?=
 =?utf-8?B?VHZkZk5BbTkyQ0ZERkJEYTRydlA3azN2clk1RFhFMDN0dnIwdEZiRm1rMXlt?=
 =?utf-8?B?ZlhJekVRTjBsZmRWN0FWUXFRVXU4elpvY2pYUjFETGdzWUZSN1ZiWDZFN215?=
 =?utf-8?B?OFR2N256RjhZd25laG5YdHJhL0VPUUVqaWlMS2UrSzhoTjVEYThmbkFsR2Mr?=
 =?utf-8?B?WDlSVUh0WlRwRWVndm0xb3N1ZkF4ek9OcXZOd0tUVksrZURMY1BFTDFVVzJ1?=
 =?utf-8?B?QTVJOXordzYrbVFFU1E2bjVRdGtRNXplaUdjL1RnWTVRajNoNVJVSUwyZlFK?=
 =?utf-8?B?ZDdpNm5EYmJ1dDlGNXNqTHkyZm0zbTh1RVNpdTMyVTBrdGtnS0FtNGhqZjN3?=
 =?utf-8?B?NWNMcEhFOXd4dmtVL3RIY1gxZ2RTNEc0MDI4M3BFTVJwWjN1NmRvRFVFVTk4?=
 =?utf-8?B?b0JFNnJ5SzVwcmExVGpGSWw1dTl5cno5VEVHQzBFM2RlWVVkRjVWeWdOR3pC?=
 =?utf-8?B?YnQxKzZudlBTdFJST2NwOU1BUUZFY21EVjBjVis3Q2dIVWZPc0pFeE9CbHlP?=
 =?utf-8?B?dVZBSjkyMy9aUENGTHlzYjNNcUhFRlJ4Z1drdWpndmtsSEdXNnNOWjlWTlRr?=
 =?utf-8?B?UE9MRFRRTjNHRWxESmk4OHU5SWk3NGZxR0VRemdTQW04c2VSMFZnd3ovSUY1?=
 =?utf-8?B?RzlnVjc5WE5ZVGNyekxhWEFyN2I1MDNyZDhUSzY4NnhiTHNublJtZ0lmMWVC?=
 =?utf-8?B?Um5GeVJzMk9yWCtuZnZ3WHI3aVY2dDhIMGF3cU5LZFpRQ1pFU2pBM09XbFIr?=
 =?utf-8?B?VFRyK2p4ZlRCM0NOeTNKeUZUdis3SXZXeFk4emNHTnhSd1pvai9ZNHNzaVdw?=
 =?utf-8?B?VUZzU1RCcVE0RmJkZ09acHRnSml0aGM0YXBaU1M4K3JDTnJHQmFkYmEvWmNJ?=
 =?utf-8?B?b3R1ZXBTQTFqd2pmNGFZblEvTFViMXZrcld2dzU5WWRaU3haNDNFbWttTUYr?=
 =?utf-8?B?STJuVTFYT3pTRVlCQllWTkJsUERQRHhYRGJ6L1F5aHpVaG5aK3Bwcnh0TkRu?=
 =?utf-8?B?RDM2V0lNSkpFR1M3UjBmTzNtOVNUK0lKYlMydVF4eTlkdTVlN1FFSUxWMi9n?=
 =?utf-8?B?THBZaTFrcXY2eFpSdUZTOXhlSGJMN2xhWDF6QXBhbDkwR0sxeENGMWF0UjAr?=
 =?utf-8?B?RkxEN1pmWFpJbm9PaWd1b3JSSmRBV0FFUDE5eUN0b0h0R3dRK3ZWOWU5QkJB?=
 =?utf-8?B?WHV4S29QdjJqMzJkd1JkQWprYVh0WnhoMHBrR2VwL2J3SDVjVlFBUE16ZGRO?=
 =?utf-8?B?Y3Jac09lY2ltVndGSWlFTW9EYWFtN3djTGZYdzkrK0pYeTZSU3hSeXRVeEc3?=
 =?utf-8?B?R2k4cnNzbnRQTkhkUHJoNWNZaERlWEZjanE1ZGxjK01acUNaakxwT3JXK1ZR?=
 =?utf-8?B?RW1YMmJ0K2U2WElNcjZmenZ5UlVjU1ZQa21vYXdBdCtMSkxNS2hSV0J5aE5B?=
 =?utf-8?B?UUFTdWMzVWZMVmM3Tkh0QU14N2FvZE5EbnNyemNOS1JHQko1OThsa3haaHFz?=
 =?utf-8?B?YXdJRWVnbTltNXNxTzFtVDhQZDhnbzNvY3NValllYzBJWXNKUlJPZERNMFJr?=
 =?utf-8?B?L3lMTjdQNWJlWFlhZnJ4QXFLWVlaV2h5SldjeUw1eWppWTNaK1lTTG5xbFhm?=
 =?utf-8?B?NVVGZ1FoTUYwbEJ0VVFvRFVwQTIvOTd2WXN4S2V5SFp1cFBYbEFuRkZDQ2F0?=
 =?utf-8?B?MmFrSVZhMEEyQStvVW44cFdYSmZFUFdObkZsY21mUXJneFd0K0JjWFFnZFBP?=
 =?utf-8?B?SS83TjlDQ2F2NGp3QVBFQmZlbVMvdVlqTjN5aDM4VmcvMDlFNzZ1aGx4VUhO?=
 =?utf-8?Q?tVdaoXLkQlEGpNgllZss9lc=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0fbf796a-de5a-4848-55f5-08db103176cb
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Feb 2023 15:21:23.8817
 (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: bOeAHZVoE1I59JBWAHDxLdKARsZu77aDhEYsMIzZnP8AaR7oYWl9SNx3eQzv2g8h6NPLA7C099lgPWUSsATrHCPv/CUwFfmn7rMnuaNDuFo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB7834
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: Thu, 16 Feb 2023 16:40:33 +0800

> 
> On 2/16/2023 1:01 AM, Alexander Lobakin wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: Gavin Li <gavinl@nvidia.com>
>> Date: Wed, 15 Feb 2023 16:30:04 +0800
>>
>>> On 2/15/2023 11:36 AM, Gavin Li wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> From: Gavin Li <gavinl@nvidia.com>
>>>>> Date: Tue, 14 Feb 2023 15:41:37 +0200
>> [...]
>>
>>>>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>>>>> buf[],
>>>>>>         udp->dest = tun_key->tp_dst;
>>>>>>         vxh->vx_flags = VXLAN_HF_VNI;
>>>>>>         vxh->vx_vni = vxlan_vni_field(tun_id);
>>>>>> +     if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>>>>> +             md = ip_tunnel_info_opts((struct ip_tunnel_info
>>>>>> *)e->tun_info);
>>>>>> +             vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>>>>> +                                 (struct vxlan_metadata *)md);
>>>>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>>>>> arguments instead of working around by casting away?
>>>> ACK. Sorry for the confusion---I misunderstood the comment.
>>> This ip_tunnel_info_opts is tricky to use const to annotate the arg
>>> because it will have to cast from const to non-const again upon
>>> returning.
>> It's okay to cast away for the `void *` returned.
>> Alternatively, use can convert it to a macro and use
>> __builtin_choose_expr() or _Generic to return const or non-const
>> depending on whether the argument is constant. That's what was recently
>> done for container_of() IIRC.
> 
> I've fixed vxlan_build_gbp_hdr in V2. For ip_tunnel_info_opts, it's
> confusing to me.
> 
> It would be as below after constifying the parameter.
> 
> static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
> {
>     return (void *)(info + 1);
> }
> Is there any value gained by this change?

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.

> 
>>
>>>>>> +     }
>>>>>> +
>>>>>> +     return 0;
>>>>>> +}
>> [...]
>>
>> Thanks,
>> Olek

[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

Thanks,
Olek