Message-ID: <85172e19-5344-bc30-e6a4-aa39dba3b50b@intel.com>
Date:   Wed, 15 Feb 2023 17:56:07 +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 2/3] net/mlx5e: Add helper for
 encap_info_equal for tunnels with options
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-3-gavinl@nvidia.com>
 <611a9a70-0f6e-cba5-dcb3-3412e6e9956f@intel.com>
 <40a616a5-f350-2ac1-eda1-7e4c777ed487@nvidia.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <40a616a5-f350-2ac1-eda1-7e4c777ed487@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?RHR0SStsZ3RQQ29mWnIzbW5qS1V4V0ZaQUdLdVdwNkR1ZHNKZGFkOCtHZWdo?=
 =?utf-8?B?RWJBUHRFcjMxd1BBaUxnblVMZGNEcGZPU3ljbm9pNHlDS3dXM2ozVnh6cHNN?=
 =?utf-8?B?WnFDajlCU3JHQUgwdUVpQUlzRGlmcnc5V1RTNmt5SEJiUVkwUHh3MmVLd2ht?=
 =?utf-8?B?RUM3M0t5YXFpZndQb2lEL3BnTlNsNVgzOVplR2wvam94QnZpdXJob0hveUFH?=
 =?utf-8?B?Sy9vbTNwTHZaRVNmS1V0VTI3Tk5kMkVQQlpkZWY0ZDBYK3JyQWVFMkIyMDNH?=
 =?utf-8?B?ZUVQTXRIM0N5azcrSStOcE50VjNnMFY4OW5IZ0E4bjNqbHpKRDZ5Ty9iVFRQ?=
 =?utf-8?B?R0pocmNHcTRMSUFkOEZCbGhqc0J4THlXeVRCZkZLa01XMTZUWmpmZTdaZEhD?=
 =?utf-8?B?TWlHRjUyRmhwUk5aYyt0MW53SDl0UjEvR2ZVcm52N1IwdWs3YkxGd3M3MWo2?=
 =?utf-8?B?VEhjQUF3a0toOVVIbW1FWCtXNHNjUXFweXdHeGJkaStKYlMveUpYVmJIUXBn?=
 =?utf-8?B?TnBjTjNBa3FyTy9vWm1RdEd0SnFVV0lmWHp6a2pFVHAzbWppNEtLTWFldmZV?=
 =?utf-8?B?QTIwL2xqeldTdFFITHBXK2V1Yzg3YkFxRzM5WU5oWkxDTDVnQ2pDV3QwWXUz?=
 =?utf-8?B?OEs2WjlMelJkQnpSMFlOVllRMldVbjNMckhGc0hHaDNnUzJDVitoSTh4ZHk4?=
 =?utf-8?B?MTBTV20xM0tZd2hLV1p1TjVuaVY0UTA5ay8zdUppdi92L012UHcydUF3eFBB?=
 =?utf-8?B?Qmlqem8vWnlXY2FuMVZKNndvY1NLV0s2WmV2RlQ5WmFTaExzNW1PQlRpVnRv?=
 =?utf-8?B?QzBvQms1dFdseGtuSzlKRHBZUm5KRllpSXBKdWVxUVBWb08zbVN4NkdEVnZT?=
 =?utf-8?B?M1hVOEVhMW0xYkwrU1FnSTdVVkJQb3ZFdFRZeWJYZThrQlpSZ0lHb1RkNE13?=
 =?utf-8?B?YXJwSWlrYXJmdDRVYzNTUWFlNUFyTC9ERHJMTFZJS00wRW5PL3dwZFRsbExQ?=
 =?utf-8?B?ZmcwRmx5WjVhamVCZWhoK1pncjlwZUdPNWxkUjNEQlA0U2hQTkJNK2NBenhP?=
 =?utf-8?B?Z0RhWlo1U2NKd2hZSFZicThMeVA4QkQrNXFYUk9Bd0dTMXJ3NXNnV1NZbjNi?=
 =?utf-8?B?dnlMMUVGV0NoekVhdE1uZ1l0OTYraStrSnhiTWdYUHR5VTIzais3OVlLb09s?=
 =?utf-8?B?QnVmcnBTdFU3OVVhUnp0SE0vYm5xTElFcC9kcEswT2ErSWhLbFZuQURjUmdx?=
 =?utf-8?B?NWc3OGQ1dU8zQyt2dWxCd2xwWkZuZE53bitsT0Q0RzFGYTgwREt0M1BZU1Mr?=
 =?utf-8?B?RjFoVmpkZGxGK2t5ZXk2RFZ1QW91dHM0bk1RaUhKWVd5NzJCaTBpWHJXanFO?=
 =?utf-8?B?Rk5JcDVlcDNZZTdhTFd5WVFtNXVWd2lNV2RRRzRRbDJvVkpCd2dneXd5YTFO?=
 =?utf-8?B?OEo3ZHZvV2g3R0VDU3ZJVXpSY05oQkZJQzhsYzVEUktaRklFR2tZcHMxQ2h4?=
 =?utf-8?B?c01IclJaYzVlbW9TUU1tSjczYytLNUlKMW5rNUhWNVhpbE10eXo2WU5ZYUht?=
 =?utf-8?B?dHVnN2Y4NFJwQkQyTXBqQ0kremRVSEJ2c2Rjb21YRXI1Z0xRYVFBTUJKNk12?=
 =?utf-8?B?bjlBQ0dDK0tzbVphOWJBeVJiWDlYVjNXQlBPYitJNnl2Y2JEVGdmU1UxQU9j?=
 =?utf-8?B?Y1p1QWMwcTVTRFNQZkxadGVOY0NudUowRWs2ZEt3Um5jYVBvQ0JxZnovM0dX?=
 =?utf-8?B?b2lJLytVdmluR0lPd041TllqcFd4MlUwNnpDcWVpcEwxVGhqQ0p6R3c4N0FV?=
 =?utf-8?B?TjlDQUErTFZFMlhUNElyWEZDdnk4aGNtZ29ZMHFoTUdUWlhYbzhlSnM2NVha?=
 =?utf-8?B?WlZvN0syQmdaN0hCOFZwVzd4TjgyalJnZEF5OE1qRkxhUjIvN04wbE5aalpB?=
 =?utf-8?B?M2phV2hUcWNxYkk2WUVLVjlHZkRnM21Gc3NxYXdDNFBvZVZjS0hsSjNOVlY5?=
 =?utf-8?B?cW5nM1JtMFFRbGtsbmZyMGdma1lKMVZ3TVZEMFhDVFB1b1g5QlNTMHZUZ1pi?=
 =?utf-8?B?OStjUGNBWXQwYnQ3RDBXN0thZVAyaXk5dUw2U3d1UUdkMDdPcDlPTEpycVR4?=
 =?utf-8?B?VXNJMXM3bVNlSFhZTG9TTEhVbFdyOS85UlJzQXNhUVNoaFVON05yeHkrSkFQ?=
 =?utf-8?Q?MEUEDlJh/37i/LI72P7AVJA=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bdfed791-db1b-4bd7-d3ca-08db0f75b8ad
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Feb 2023 16:57:28.9546
 (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: DLRVQRvkC/ou/v56p9nWZ9Hd3If1h7graLmPMLhnePkLqi6r/RkDPny7PtZIBJ+7OtKWQbO/5tRR9oSXax9uIwSFi61f0zlXK7RbrnezHD4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7213
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, 15 Feb 2023 10:54:12 +0800

> 
> On 2/14/2023 11:01 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:36 +0200

[...]

>>> +     if (a_has_opts != b_has_opts)
>>> +             return false;
>>> +
>>> +     /* options stored in memory next to ip_tunnel_info struct */
>>> +     a_info = container_of(a->ip_tun_key, struct ip_tunnel_info, key);
>>> +     b_info = container_of(b->ip_tun_key, struct ip_tunnel_info, key);
>>> +
>>> +     return a_info->options_len == b_info->options_len &&
>>> +             memcmp(a_info + 1, b_info + 1, a_info->options_len) == 0;
>> 1. memcmp() is not aligned to the first expr (off-by-one to the right).
> Options start from "info + 1", see ip_tunnel_info_opts and will use it
> here to replace the "info+1".

Nah, I mean the following. Your code:

	return a_info->options_len == b_info->options_len &&
		memcmp(a_info + 1, b_info + 1, ...

should be:

	return a_info->options_len == b_info->options_len &&
	       memcmp(a_info + 1, b_info + 1, ...

7 spaces instead of a tab to have it aligned to the prev line.

>> 2. `!expr` is preferred over `expr == 0`.
> ACK
>>
>>> +}
>>> +
>>>   static int cmp_decap_info(struct mlx5e_decap_key *a,
>>>                          struct mlx5e_decap_key *b)
>>>   {
>> [...]
>>
>> Thanks,
>> Olek
Thanks,
Olek