Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <224af610-5bf2-9740-b698-190453c52d69@nvidia.com>
Date:   Mon, 19 Jun 2023 17:15:43 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v2 for-next] RDMA/core: Get IB width and speed from netdev
To:     Junxian Huang <huangjunxian6@hisilicon.com>,
        Leon Romanovsky <leon@kernel.org>
Cc:     jgg@nvidia.com, linux-rdma@vger.kernel.org, linuxarm@huawei.com,
        linux-kernel@vger.kernel.org
References: <20230603063833.541682-1-huangjunxian6@hisilicon.com>
 <20230611174605.GG12152@unreal>
 <1b11b9e9-a729-0d61-52e3-6bcf132ca356@hisilicon.com>
Content-Language: en-US
From:   Mark Zhang <markzhang@nvidia.com>
In-Reply-To: <1b11b9e9-a729-0d61-52e3-6bcf132ca356@hisilicon.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZmNqMGtGSjNuaTJ3UFlLZHZpd0pRZ0hYejFvS1I0d0d5RStWeEFNZU5JSFV2?=
 =?utf-8?B?OXVwZzFLcUlhb1RranpLRElER2pDV1IxYmtVZmxHVGR1OThMbU9STHoxdTNG?=
 =?utf-8?B?Nkg5LzBWeENRZnRwQ3NIVTh1QVBWd3VBSzMwVUJqNWJMQlZLK2h3TlZ0WFdy?=
 =?utf-8?B?bUhPQkJVOThHalpzTkJWamRXUFB2cTFvRk9ZQW5vR0JQMWlIZit3enBKbXJS?=
 =?utf-8?B?VEl0OElQRklyWmdYcGIwekpzZUkyWm84cHlpU3h4MDgrOXNqQ2pYaWJUeXZV?=
 =?utf-8?B?UEI1NEhZa21TMVgrU0pXRUZTU3BadkM3ams0SFJzZWwzRDJjR25EdGhOZ0sz?=
 =?utf-8?B?eE1kYnZhY3dVWFpuK0dScXF6cFEyTkZKR1RCb1lWK2NUY01QNUlKWVR3Y0hm?=
 =?utf-8?B?c29qR0RRRDhkT1NycjZqMjVUbmhlM0FQZ0pBeEZCTWxzN2tOc094TUZSVkhS?=
 =?utf-8?B?OEZHaDBQci9uTE56TVQrbE42MnV6WjZNcHMvNGFCNHRKdG02ZXlYQnE0MEI4?=
 =?utf-8?B?c1QxQUx4TE94RVZ4Yk9OZm9wTEh5bkJTMWhQai9xUFFmTjl3bjBrMEMzK2lR?=
 =?utf-8?B?MVJNbTQvbkg2a1ladFRpTFVnYml3bHhOMXdianNweERKOFkzT3U1RnhwSWsr?=
 =?utf-8?B?RGppTHlyZ2t5clhiY3pxb3FidG1MbFdEYTMwMi9nMlhhYXFQK2xPbm0zZUtC?=
 =?utf-8?B?MDhma3FwYm9PUDBRSitTRGl2NVJTR3ViZG1RVFVzdStGYVpzZ0pzRDVFUE1l?=
 =?utf-8?B?K1gwaG5VM1Fwc0NZYjhiOEZncE5GcldCMkR2eXpTRm5nOFI1aWFQU1Z5Snpl?=
 =?utf-8?B?WWFtMjZpdDlXVU9yb3JKQkhWUENJTXI3bTZBVFRlQk8wQnQ1WldoVTFFTUUv?=
 =?utf-8?B?ZmRPbGRuOUEzYldJcENxSnY5WHFFeFd1aTdPRDJ4YjVraWlHNXJhVlcwS0JT?=
 =?utf-8?B?TC9LSzVPYzRycllJOEgvUGp0eVVHdXFnWXMzWS9HaUJQaGFiazhlV0NRUTd4?=
 =?utf-8?B?cWowcG1PN0dVOTFFVlh6TENSNHVCV3pkeDc1SFloNVZ2a0d5ZHRmWitzNXhl?=
 =?utf-8?B?TTlseTRaTlRpYnNxcUZjMDlMR0YyS1d0MkJkc0JZZlExaFpYWXEvcnhIMktV?=
 =?utf-8?B?cG12Rk5mVVBMREFPaXRhSk5xOG9ZclYxMHpmZXEyNi9DbVV4L2V2TjQ5Q1ln?=
 =?utf-8?B?amF0dFl4bHo3Y3hZbEVCSFFvQjl4Q1owUCtZbjE0eEwyeVF6dHAxSVJlUDRW?=
 =?utf-8?B?WVEyU0NXWU9GZ2VQR1N5a0tLR2VNaDdlYlZybm1XbGMzLzB0SjlvMmxnajNw?=
 =?utf-8?B?NzMvUjhsc3luQjFLOUE3UXlPOFh5NmJDckVIL3RKV09FMjNyRGJ4QkJrSkoy?=
 =?utf-8?B?eUhhYTdRR011T0NhRmZERzNGYUFFM3QvM1AvWXVxclJsSU9Kc215ZVliZGl4?=
 =?utf-8?B?b0NROFNJeDlKS3F5YmZza005bS9laXpvZjRFaHpjQlo1ck1yK3JHb3RHZS91?=
 =?utf-8?B?TU1WVkMzR3Jvb0FsSWlUeFdpUVRkeE5UNnZOdHRLeGhVcEFnc25hUThmaDNV?=
 =?utf-8?B?SGNPb2thVjdlRkpxNHVaa0Nyd3ByQllYWCtockxTaG1KMm1HR0NnVzFHWnRT?=
 =?utf-8?B?bUdFOWRjZklDcnRIVmU3RkN0TUlBbEN0SmZSSm04Q2cvR0ZDemw5eGhOTncx?=
 =?utf-8?B?Rk4zVXZXUmszUjNEZ2ZoanJ1eVFiYy9tc0hVL3R5UVZNZWZDdWw0UTJSWlZQ?=
 =?utf-8?B?WUJoWXZ2OGpscHp0aWpoUWZWeVdWRmxZaFR3cmVSRzZZN0dlSHZGTHp6UGty?=
 =?utf-8?B?cXp6bVpOeDJUdzJzV0JEZzJ1NmVKWXYvSlErbXVVVW5veUVyRnVuVzNqWkdN?=
 =?utf-8?B?ejU5VitqTmhyYTd3SzcwcTlsV0kvMUQ1UzZnZkpNcjNIMWNadHpsNDZWSVZX?=
 =?utf-8?B?QXBHMVRGVGV1U2lHc0tIZnhWWW9BaGlhME1QS2h3aHFnUmsxKzNxQzVTNkhO?=
 =?utf-8?B?WldGNjhXZk1WaWhqZlJ6TWZTOEt0TFRabThydW9KN2VJZm9TUzViSk5lbGtU?=
 =?utf-8?B?Z2hOTkxZWHhPOW9lZ3VLc3JqejNCZUZ2VlUyU1Y0akcrVnJCRjVxMXYrcW5s?=
 =?utf-8?Q?pX87Ojch0w5abYp9iAPf+yKlK?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1c5820f0-606e-4415-c614-08db70a5c926
X-MS-Exchange-CrossTenant-AuthSource: MWHPR12MB1373.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jun 2023 09:15:55.8738
 (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: a7qzW/RTKwJKdvnDlg9E70jO4I2B/TqIr6st9BuPC8h4KG5duNb9/6DYOVm01jZY7n7FXkBob+u5oAUsQmbvWw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR12MB5366
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 6/19/2023 2:20 PM, Junxian Huang wrote:
> 
> 
> On 2023/6/12 1:46, Leon Romanovsky wrote:
>> On Sat, Jun 03, 2023 at 02:38:33PM +0800, Junxian Huang wrote:
>>> From: Haoyue Xu <xuhaoyue1@hisilicon.com>
>>>
>>> Previously, there was no way to query the number of lanes for a network
>>> card, so the same netdev_speed would result in a fixed pair of width and
>>> speed. As network card specifications become more diverse, such fixed
>>> mode is no longer suitable, so a method is needed to obtain the correct
>>> width and speed based on the number of lanes.
>>
>> I'm sorry but I didn't understand the problem statement. Can you please
>> provide an example of configuration that will give different results
>> before this patch and after?
>>
> 
> I'll give examples with 20G and 200G netdevs respectively.
> 
> 20G:
> Before this patch, regardless of the actual number of lanes, the width and
> speed displayed in ibv_devinfo would be always fixed:
> 	active_width: 4X
> 	active_speed: 5 Gbps
> After this patch, there will be different combinations of width and speed
> according to the number of lanes. For example, for a 20G netdev whose number
> of lanes is 2, the width and speed displayed in ibv_devinfo will be:
> 	active_width: 2X
> 	active_speed: 10 Gbps
> 
> 200G:
> Before this patch, netdevs with netdev_speed more than 40G cannot get a right
> width and speed in ibv_devinfo. Only the default result would be displayed:
> 	active_width: 4X
> 	active_speed: 25 Gbps
> After this patch, taking an example with 4 lanes, the displayed results will be:
> 	active_width: 4X
> 	active_speed: 50 Gbps

Can we use ib_query_port() instead of __ethtool_get_link_ksettings()?
   width = attr.active_width;
   speed = ib_speed_enum_to_int(attr.active_speed) *
	  ib_width_enum_to_int(attr.active_width);


>>>
>>> This patch retrieves netdev lanes and speed from net_device and
>>> translates them to IB width and speed. Also, add a generic function
>>> to translating netdev speed to IB speed.
>>>
>>> Signed-off-by: Haoyue Xu <xuhaoyue1@hisilicon.com>
>>> Signed-off-by: Luoyouming <luoyouming@huawei.com>
>>> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
>>> ---
>>>   drivers/infiniband/core/verbs.c | 19 +++++++++++++++++--
>>>   include/rdma/ib_verbs.h         | 26 ++++++++++++++++++++++++++
>>>   2 files changed, 43 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
>>> index b99b3cc283b6..35f1b670600a 100644
>>> --- a/drivers/infiniband/core/verbs.c
>>> +++ b/drivers/infiniband/core/verbs.c
>>> @@ -1880,11 +1880,19 @@ int ib_modify_qp_with_udata(struct ib_qp *ib_qp, struct ib_qp_attr *attr,
>>>   }
>>>   EXPORT_SYMBOL(ib_modify_qp_with_udata);
>>>   
>>> +static void ib_get_width_and_speed(u32 netdev_speed, u32 lanes,
>>> +				   u16 *speed, u8 *width)
>>> +{
>>> +	*width = ib_int_to_ib_width(lanes);
>>> +	*speed = ib_eth_to_ib_speed(netdev_speed / lanes);
>>> +}
>>> +
>>>   int ib_get_eth_speed(struct ib_device *dev, u32 port_num, u16 *speed, u8 *width)
>>>   {
>>>   	int rc;
>>>   	u32 netdev_speed;
>>>   	struct net_device *netdev;
>>> +	bool cap_link_lanes_supported;
>>>   	struct ethtool_link_ksettings lksettings;
>>>   
>>>   	if (rdma_port_get_link_layer(dev, port_num) != IB_LINK_LAYER_ETHERNET)
>>> @@ -1896,16 +1904,23 @@ int ib_get_eth_speed(struct ib_device *dev, u32 port_num, u16 *speed, u8 *width)
>>>   
>>>   	rtnl_lock();
>>>   	rc = __ethtool_get_link_ksettings(netdev, &lksettings);
>>> +	cap_link_lanes_supported = netdev->ethtool_ops->cap_link_lanes_supported;
>>>   	rtnl_unlock();
>>>   
>>>   	dev_put(netdev);
>>>   
>>>   	if (!rc && lksettings.base.speed != (u32)SPEED_UNKNOWN) {
>>>   		netdev_speed = lksettings.base.speed;
>>> +		if (cap_link_lanes_supported && lksettings.lanes) {
>>
>> According to the documentation cap_link_lanes_supported defines if
>> number of lanes can be supplied by user and I would expect from
>> __ethtool_get_link_ksettings() to get right numbers after it was
>> changed.
>>
>> Thanks
>>
> 
> I'm sorry but I didn't quite understand. Do you mean the critical section of
> rtnl_lock() here should be expanded to make sure getting the right number of
> lanes?
> 
> Junxian
> 
>>> +			ib_get_width_and_speed(netdev_speed, lksettings.lanes,
>>> +					       speed, width);
>>> +			return 0;
>>> +		}
>>>   	} else {
>>>   		netdev_speed = SPEED_1000;
>>> -		pr_warn("%s speed is unknown, defaulting to %u\n", netdev->name,
>>> -			netdev_speed);
>>> +		if (rc)
>>> +			pr_warn("%s speed is unknown, defaulting to %u\n",
>>> +				netdev->name, netdev_speed);
>>>   	}
>>>   
>>>   	if (netdev_speed <= SPEED_1000) {
>>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>>> index 1e7774ac808f..7dc926ec7fee 100644
>>> --- a/include/rdma/ib_verbs.h
>>> +++ b/include/rdma/ib_verbs.h
>>> @@ -552,6 +552,18 @@ static inline int ib_width_enum_to_int(enum ib_port_width width)
>>>   	}
>>>   }
>>>   
>>> +static inline int ib_int_to_ib_width(u32 lanes)
>>> +{
>>> +	switch (lanes) {
>>> +	case 1: return IB_WIDTH_1X;
>>> +	case 2: return IB_WIDTH_2X;
>>> +	case 4: return IB_WIDTH_4X;
>>> +	case 8: return IB_WIDTH_8X;
>>> +	case 12: return IB_WIDTH_12X;
>>> +	default: return IB_WIDTH_1X;
>>> +	}
>>> +}
>>> +
>>>   enum ib_port_speed {
>>>   	IB_SPEED_SDR	= 1,
>>>   	IB_SPEED_DDR	= 2,
>>> @@ -563,6 +575,20 @@ enum ib_port_speed {
>>>   	IB_SPEED_NDR	= 128,
>>>   };
>>>   
>>> +static inline int ib_eth_to_ib_speed(u32 speed)
>>> +{
>>> +	switch (speed) {
>>> +	case SPEED_2500: return IB_SPEED_SDR;
>>> +	case SPEED_5000: return IB_SPEED_DDR;
>>> +	case SPEED_10000: return IB_SPEED_FDR10;
>>> +	case SPEED_14000: return IB_SPEED_FDR;
>>> +	case SPEED_25000: return IB_SPEED_EDR;
>>> +	case SPEED_50000: return IB_SPEED_HDR;
>>> +	case SPEED_100000: return IB_SPEED_NDR;
>>> +	default: return IB_SPEED_SDR;
>>> +	}
>>> +}
>>> +
>>>   enum ib_stat_flag {
>>>   	IB_STAT_FLAG_OPTIONAL = 1 << 0,
>>>   };
>>> -- 
>>> 2.30.0
>>>