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 3C5BFA0C55; Mon, 6 Sep 2021 18:16:22 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A1B8F410F0; Mon, 6 Sep 2021 18:16:21 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 5C4DC410ED for ; Mon, 6 Sep 2021 18:16:20 +0200 (CEST) Received: by shelob.oktetlabs.ru (Postfix, from userid 122) id 126A17F609; Mon, 6 Sep 2021 19:16:20 +0300 (MSK) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on shelob.oktetlabs.ru X-Spam-Level: X-Spam-Status: No, score=0.8 required=5.0 tests=ALL_TRUSTED, DKIM_ADSP_DISCARD, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from oktetlabs.ru (calendar.oktetlabs.ru [192.168.34.50]) by shelob.oktetlabs.ru (Postfix) with ESMTP id 6FB127F502; Mon, 6 Sep 2021 19:16:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 6FB127F502 Authentication-Results: shelob.oktetlabs.ru/6FB127F502; dkim=none; dkim-atps=neutral MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 06 Sep 2021 19:16:12 +0300 From: Viacheslav Galaktionov To: Ferruh Yigit Cc: Andrew Rybchenko , Ajit Khaparde , Somnath Kotur , John Daley , Hyong Youb Kim , Beilei Xing , Qiming Yang , Qi Zhang , Haiyue Wang , Matan Azrad , Shahaf Shuler , Viacheslav Ovsiienko , Thomas Monjalon , dev@dpdk.org, declan.doherty@intel.com In-Reply-To: References: <20210712161747.958019-1-andrew.rybchenko@oktetlabs.ru> <20210831160625.3463129-1-andrew.rybchenko@oktetlabs.ru> Message-ID: X-Sender: viacheslav.galaktionov@oktetlabs.ru User-Agent: Roundcube Webmail/1.3.16 Subject: Re: [dpdk-dev] [PATCH v4] 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 2021-09-01 17:55, Ferruh Yigit wrote: > On 8/31/2021 5:06 PM, Andrew Rybchenko wrote: >> From: Viacheslav Galaktionov >> >> Getting a list of representors from a representor does not make sense. >> Instead, a parent device should be used. >> > > Which code is getting list of the representors? > > As far as I can see impacted APIs are: > 'rte_eth_representor_id_get()' > 'rte_eth_representor_info_get()' > > Which are now getting 'parent_port_id' as argument, instead of > representro port id. > > 'rte_eth_representor_info_get()' is using 'representor_info_get()' > dev_ops, > which is only implemented by 'mlx5', so is this problem only valid for > 'mlx5' > and can it be solved within PMD implementation? It's not an mlx5-specific problem, it's going to affect sfc as well once it starts supporting representors. But that doesn't really matter as it's more about the usage of representors in general, not specific to any PMD's internals. Since representors are created through some device (which is probed and assigned a port ID), it makes sense to query the list of representors from the same device. [snip] >> index edf96de2dc..72fefa59c2 100644 >> --- a/lib/ethdev/rte_ethdev_core.h >> +++ b/lib/ethdev/rte_ethdev_core.h >> @@ -185,6 +185,12 @@ struct rte_eth_dev_data { >> /**< Switch-specific identifier. >> * Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags. >> */ >> + uint16_t parent_port_id; > > Why 'parent'? Isn't this for the port that port representator > represents, does > it called 'parent'? Above that device mentioned as 'backing device' a > few times, > so would something like 'peer_port_id' better? This is true, the naming here is confusing and should be changed. "parent_port_id" doesn't point at the represented entity, but at the device that was used to create this representor. We call it the backing device, so using "backer_port_id" sounds appropriate, what do you think? >> + /**< Port ID of the backing device. >> + * This device will be used to query representor >> + * info and calculate representor IDs. >> + * Valid if RTE_ETH_DEV_REPRESENTOR in dev_flags. >> + */ >> > > > Overall I am not feeling good about adding representor port related > information > withing the device data struct. > I wonder if this information can be kept in the device private data. > > Or, it is hard to explain but can we use something like inheritance, a > representor specific dev_data derived from original dev_data. We can > store > dev_data pointers in 'struct rte_eth_dev' but can cast it to > representor > specific dev_data when type is representor. > > struct rte_eth_dev_data_rep > struct rte_eth_dev_data > > > This brings lots of complexity though, specially in allocating/freeing > this > struct, not sure if it worth to the effort. This information is usually kept in the device private data as well, but we need to use it from the generic code to redirect the representor info requests to the appropriate ports. Using "inheritance" is a good suggestion, but it does bring a lot of complexity, as you've said, and we're not sure if the result is worth the effort. We can also avoid storing this port ID in the device data by creating a special callback that PMDs would use to return it. However, this also brings complexity and this information is static anyway, so having a separate callback might be a little too much. What we're doing here just seems to be the simplest option.