From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 89F1DA0C50; Sun, 1 Aug 2021 10:40:23 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0A95940143; Sun, 1 Aug 2021 10:40:23 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id EA45C40140; Sun, 1 Aug 2021 10:40:20 +0200 (CEST) Received: from [192.168.100.116] (unknown [37.139.99.76]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 65ECD7F51A; Sun, 1 Aug 2021 11:40:19 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 65ECD7F51A DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1627807220; bh=iDIpfMsD2iyaVwAvip6sew+vPpq2E34OAA9GqnInth0=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=r0wYCESbQwbmMc8qszXtIiyn1NiXdfmVfPDRLPmZfDvxjKZ9j+fg1jHKWBst5dLUH /99lTBgzt5dzah/GOx03Nmr10Kijg6TDmzyR/zYXIn/ayhPnfKpoe6jErevikg3pZU ibGjQNhlheiv0K82ojjSkMpSlIZ0tO2tCrOVKbMM= To: "Xueming(Steven) Li" , Ajit Khaparde , Somnath Kotur , John Daley , Hyong Youb Kim , Beilei Xing , Qiming Yang , Qi Zhang , Haiyue Wang , Matan Azrad , Shahaf Shuler , Slava Ovsiienko , NBU-Contact-Thomas Monjalon , Ferruh Yigit Cc: "dev@dpdk.org" , Viacheslav Galaktionov , "stable@dpdk.org" References: <20210712161747.958019-1-andrew.rybchenko@oktetlabs.ru> <498b2d48-4c04-24f5-12a7-b1da3a55fc51@oktetlabs.ru> <0164acb5-9644-4058-ee4c-8eeead751f58@oktetlabs.ru> From: Andrew Rybchenko Message-ID: <04a83f6b-8d7e-0bcf-27cf-856f311af599@oktetlabs.ru> Date: Sun, 1 Aug 2021 11:40:17 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] ethdev: fix representor port ID search by name X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 7/29/21 7:13 AM, Xueming(Steven) Li wrote: > > >> -----Original Message----- >> From: Andrew Rybchenko >> Sent: Tuesday, July 20, 2021 5:00 PM >> To: Xueming(Steven) Li ; Ajit Khaparde ; Somnath Kotur >> ; John Daley ; Hyong Youb Kim ; Beilei Xing >> ; Qiming Yang ; Qi Zhang ; Haiyue Wang >> ; Matan Azrad ; Shahaf Shuler ; Slava Ovsiienko >> ; NBU-Contact-Thomas Monjalon ; Ferruh Yigit >> Cc: dev@dpdk.org; Viacheslav Galaktionov ; stable@dpdk.org >> Subject: Re: [PATCH] ethdev: fix representor port ID search by name >> >> On 7/19/21 3:50 PM, Xueming(Steven) Li wrote: >>> >>> >>>> -----Original Message----- >>>> From: Andrew Rybchenko >>>> Sent: Monday, July 19, 2021 8:36 PM >>>> To: Xueming(Steven) Li ; Ajit Khaparde >>>> ; Somnath Kotur >>>> ; John Daley ; Hyong >>>> Youb Kim ; Beilei Xing ; >>>> Qiming Yang ; Qi Zhang ; >>>> Haiyue Wang ; Matan Azrad ; >>>> Shahaf Shuler ; Slava Ovsiienko >>>> ; NBU-Contact-Thomas Monjalon >>>> ; Ferruh Yigit >>>> Cc: dev@dpdk.org; Viacheslav Galaktionov >>>> ; stable@dpdk.org >>>> Subject: Re: [PATCH] ethdev: fix representor port ID search by name >>>> >>>> On 7/19/21 2:54 PM, Xueming(Steven) Li wrote: >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Andrew Rybchenko >>>>>> Sent: Monday, July 19, 2021 4:46 PM >>>>>> To: Xueming(Steven) Li ; Ajit Khaparde >>>>>> ; Somnath Kotur >>>>>> ; John Daley ; >>>>>> Hyong Youb Kim ; Beilei Xing >>>>>> ; Qiming Yang ; Qi >>>>>> Zhang ; Haiyue Wang ; >>>>>> Matan Azrad ; Shahaf Shuler ; >>>>>> Slava Ovsiienko ; NBU-Contact-Thomas >>>>>> Monjalon ; Ferruh Yigit >>>>>> >>>>>> Cc: dev@dpdk.org; Viacheslav Galaktionov >>>>>> ; stable@dpdk.org >>>>>> Subject: Re: [PATCH] ethdev: fix representor port ID search by name >>>>>> >>>>>> On 7/19/21 9:58 AM, Xueming(Steven) Li wrote: >>>>>>> >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Andrew Rybchenko >>>>>>>> Sent: Tuesday, July 13, 2021 12:18 AM >>>>>>>> To: Ajit Khaparde ; Somnath Kotur >>>>>>>> ; John Daley ; >>>>>>>> Hyong Youb Kim ; Beilei Xing >>>>>>>> ; Qiming Yang ; Qi >>>>>>>> Zhang ; Haiyue Wang >>>>>>>> ; Matan Azrad ; Shahaf >>>>>>>> Shuler ; Slava Ovsiienko >>>>>>>> ; NBU-Contact-Thomas Monjalon >>>>>>>> ; Ferruh Yigit ; >>>>>>>> Xueming(Steven) Li >>>>>>>> Cc: dev@dpdk.org; Viacheslav Galaktionov >>>>>>>> ; stable@dpdk.org >>>>>>>> Subject: [PATCH] ethdev: fix representor port ID search by name >>>>>>>> >>>>>>>> From: Viacheslav Galaktionov >>>>>>>> >>>>>>>> >>>>>>>> Fix representor port ID search by name if the representor itself >>>>>>>> does not provide representors info. Getting a list of >>>>>>>> representors from a representor does not make sense. Instead, a >>>>>>>> parent device >>>>>> should be used. >>>>>>>> >>>>>>>> To this end, extend the rte_eth_dev_data structure to include the port ID of the parent device for representors. >>>>>>>> >>>>>>>> Fixes: df7547a6a2cc ("ethdev: add helper function to get >>>>>>>> representor >>>>>>>> ID") >>>>>>>> Cc: stable@dpdk.org >>>>>>>> >>>>>>>> Signed-off-by: Viacheslav Galaktionov >>>>>>>> >>>>>>>> Signed-off-by: Andrew Rybchenko >>>>>>>> --- >>>>>>>> The new field is added into the hole in rte_eth_dev_data structure. >>>>>>>> The patch does not change ABI, but extra care is required since >>>>>>>> ABI check is disabled for the structure because of the libabigail >>>>>>>> bug >>>>>> [1]. >>>>>>>> >>>>>>>> Potentially it is bad for out-of-tree drivers which implement >>>>>>>> representors but do not fill in a new parert_port_id field in rte_eth_dev_data structure. Do we care? >>>>>>>> >>>>>>>> May be the patch should add lines to release notes, but I'd like to get initial feedback first. >>>>>>>> >>>>>>>> mlx5 changes should be reviwed by maintainers very carefully, since we are not sure if we patch it correctly. >>>>>>>> >>>>>>>> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=28060 >>>>>> >>>>>> [snip] >>>>>> >>>>>>>> --- a/lib/ethdev/ethdev_driver.h >>>>>>>> +++ b/lib/ethdev/ethdev_driver.h >>>>>>>> @@ -1248,8 +1248,8 @@ struct rte_eth_devargs { >>>>>>>> * For backward compatibility, if no representor info, direct >>>>>>>> * map legacy VF (no controller and pf). >>>>>>>> * >>>>>>>> - * @param ethdev >>>>>>>> - * Handle of ethdev port. >>>>>>>> + * @param parent_port_id >>>>>>>> + * Port ID of the backing device. >>>>>>>> * @param type >>>>>>>> * Representor type. >>>>>>>> * @param controller >>>>>>>> @@ -1266,7 +1266,7 @@ struct rte_eth_devargs { >>>>>>>> */ >>>>>>>> __rte_internal >>>>>>>> int >>>>>>>> -rte_eth_representor_id_get(const struct rte_eth_dev *ethdev, >>>>>>>> +rte_eth_representor_id_get(uint16_t parent_port_id, >>>>>>> >>>>>>> It make more sense to get representor info from parent port. >>>>>>> Representor is a member of switch domain, PMD owns the information >>>>>>> of the representor owner port and info of representors. This >>>>>>> change looks better, but not sure whether it valuable to introduce >>>>>>> a new >>>>>> member to the EAL data structure. >>>>>> >>>>>> IMHO, it is simply incorrect to return representors info on a >>>>>> representor itself. Representor info is an information which representors may be populated using the device. >>>>>> >>>>>> If above statement is correct, we need a way to get parent device >>>>>> by representor to do name to representor ID mapping. I see two options to do it: >>>>>> A. Dedicated field in rte_eth_dev_data as the patch does. >>>>>> B. Dedicated ethdev op (since representor knows parent port ID anyway). >>>>>> We have chosen (A) because of simplicity. >>>>> >>>>> Just recalled that representor port could be probed w/o owner PF, is a force for parent port? >>>> >>>> I thought that it is impossible and parent port is absolutely >>>> required for a representor. Could you provide an example and explain how will it work? >>> >>> In case of bonding, PF0 and PF1 become one PF port `bond0`, PCI address is PF0. >>> -a ,representor=pf[0-1]vf[0-99] // this is the syntax we proposed. >> >> Is it net/bonding or vendor-specific bonding in HW? >> If I remember correctly in the case of net/bonding we have ethdev ports for bonded devices. > > Not net/bonding pmd, it's Linux bonding, supported by hw driver. Got it. >> >>> >>> To be backward compatible, also support the following 2 devargs: >>> -a ,representor=[0-99] // probe bond0 and representor on pf0 >>> -a ,representor=[0-99] // probe representors on pf1. >>> If devargs start with PF1 devargs, no owner PF1 created as it disabled >>> in bonding. Can't create bond0(PF0) automatically here as device is located by PCI address(PF1) from devargs. >> >> So, I guess the problem is vendor-specific bonding in HW. Anyway legacy backward compatible representor spec should not require >> representors info since it worked before without it. So, it does not sound like a reason to have representors info on a representor >> itself. > > Legacy backward logic could be something like this: if PF owner port found, use it, fallback to current representor. > This won't break anything I guess, how do you think? Logically even in legacy backward compatibility PF1 VFs representors have parent port ID - PF0 which is a bond of PF0 & PF1. So, parent_port_id should be filled in. In this case eth_representor_cmp() will do rte_eth_representor_id_get(PF0-bond-id, -1, -1, VF, &id) which will return PF0 VF representor ID. Most likely it will even match and everything works, but it is still incorrect. In fact, I have another idea. Try to do: rte_eth_representor_id_get(representor-port-id, ...) first for the backward compatibility case and, if not supported, do it on parent port ID.