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 AD49D4318A; Tue, 17 Oct 2023 10:27:28 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9053D40279; Tue, 17 Oct 2023 10:27:28 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id A8E1640269 for ; Tue, 17 Oct 2023 10:27:26 +0200 (CEST) Received: from kwepemm000004.china.huawei.com (unknown [172.30.72.57]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4S8n8T20bFzrTM4; Tue, 17 Oct 2023 16:24:41 +0800 (CST) Received: from [10.67.121.59] (10.67.121.59) by kwepemm000004.china.huawei.com (7.193.23.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.31; Tue, 17 Oct 2023 16:27:22 +0800 Message-ID: <287187f5-3e58-d64d-b521-8fa2f4f6f2f2@huawei.com> Date: Tue, 17 Oct 2023 16:27:22 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH v3 1/8] ethdev: add member notification for bonding port To: Long Wu , Chaoyong He , "dev@dpdk.org" , Ferruh Yigit CC: oss-drivers , James Hershaw References: <20231007013440.1309422-1-chaoyong.he@corigine.com> <20231008015041.1551165-1-chaoyong.he@corigine.com> <20231008015041.1551165-2-chaoyong.he@corigine.com> From: "lihuisong (C)" In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To kwepemm000004.china.huawei.com (7.193.23.18) X-CFilter-Loop: Reflected 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 在 2023/10/9 11:11, Long Wu 写道: >> Hi Chaoyong, >> >> some comments as below. >> >> >> 在 2023/10/8 9:50, Chaoyong He 写道: >>> From: Long Wu >>> >>> Bonding PMD does not let member ports know the bonding port's >>> information, like how many member ports the bonding port has, what >>> mode the bonding port is in and so on. >>> >>> Add the notification interface for bonding port to let member port >>> know it is added to a bonding port and what the bonding port's >>> configuration is. If so the member ports have chance to achieve its >>> bond-flow-offlod or other private bonding functions. >> "its bond-flow-offlod or other private bonding functions" >> I wonder that what PMDs can do with this. >> Can you give an example in PMD to help others review? > After this patch series, I will send out nfp PMD code about "its bond-flow-offlod or other private bonding functions ". > I can explain here: > "bond-flow" means the flow rule's destination port is a bonding port. > Now DPDK can use bonding port as the source port in a flow rule and member ports can offload this flow. > But member ports cannot offload a flow that its destination port is a bonding port. > Because the member ports don't know the bonding port. (Of course, some PMDs has its self-function to let member ports know the bonding port but it doesn't a general "DPDK" way). > After this "notify patch", DPDK can do "bond-flow-offload", also "other private bonding functions"(like hardware balance policy, primary port changing and so on) can work. I think what you said is more like "bonding offload", right? It seems that you cannot do it just based on current these API in this series. You probably have other works to upstream for like "bond-flow" and "other private bonding functions". > >>> Signed-off-by: Long Wu >>> Reviewed-by: James Hershaw >>> Reviewed-by: Chaoyong He >>> --- >>> drivers/net/bonding/eth_bond_private.h | 1 + >>> drivers/net/bonding/rte_eth_bond.h | 46 ++++++++++++++++ >>> drivers/net/bonding/rte_eth_bond_api.c | 73 >> ++++++++++++++++++++++++++ >>> drivers/net/bonding/rte_eth_bond_pmd.c | 27 ++++++++-- >>> drivers/net/bonding/version.map | 3 ++ >>> lib/ethdev/ethdev_driver.h | 18 +++++++ >>> 6 files changed, 165 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/net/bonding/eth_bond_private.h >>> b/drivers/net/bonding/eth_bond_private.h >>> index e688894210..f69e85c199 100644 >>> --- a/drivers/net/bonding/eth_bond_private.h >>> +++ b/drivers/net/bonding/eth_bond_private.h >>> @@ -180,6 +180,7 @@ struct bond_dev_private { >>> uint8_t member_update_idx; >>> >>> bool kvargs_processing_is_done; >>> + bool notify_member; /**< Enable member notification of bonding port. >>> +*/ >>> >>> uint32_t candidate_max_rx_pktlen; >>> uint32_t max_rx_pktlen; >>> diff --git a/drivers/net/bonding/rte_eth_bond.h >>> b/drivers/net/bonding/rte_eth_bond.h >>> index f10165f2c6..737beca446 100644 >>> --- a/drivers/net/bonding/rte_eth_bond.h >>> +++ b/drivers/net/bonding/rte_eth_bond.h >>> @@ -351,6 +351,52 @@ rte_eth_bond_link_up_prop_delay_set(uint16_t >> bonding_port_id, >>> int >>> rte_eth_bond_link_up_prop_delay_get(uint16_t bonding_port_id); >>> >>> +/** >>> + * Set the flag of whether bonding port notifies member ports. >>> + * >>> + * @param bonding_port_id >>> + * Port ID of bonding device. >>> + * @param notify_member >>> + * Flag of whether bonding port notifies member ports. >>> + * >>> + * @return >>> + * 0 on success, negative value otherwise. >>> + */ >>> +__rte_experimental >>> +int >>> +rte_eth_bond_notify_member_flag_set(uint16_t bonding_port_id, bool >>> +notify_member); >> s/notify_membe/notify in input? >> because function name reveals the meaning already. > Thank you, you are right. > >>> + >>> +/** >>> + * Get the flag of whether bonding port notifies member ports. >>> + * >>> + * @param bonding_port_id >>> + * Port ID of bonding device. >>> + * @param notify_member >>> + * Flag of whether bonding port notifies member ports. >>> + * >>> + * @return >>> + * 0 on success, negative value otherwise. >>> + */ >>> +__rte_experimental >>> +int >>> +rte_eth_bond_notify_member_flag_get(uint16_t bonding_port_id, bool >>> +*notify_member); >>> + >>> +/** >>> + * Notify the member ports of bonding port's information. >>> + * >>> + * This interface is called in the following functions: >>> + * - bond_ethdev_lsc_event_callback() >>> + * - bond_ethdev_configure() >> Is this interface used just in these cases? >> If so I don't think it should be a API in eth_dev_op. > Sorry I'm a bit confused. > Do you mean "rte_eth_bond_notify_members" this interface? This interface is called in 3 functions. User can also call this API, right? How should the user use it? Is it when update bonding port information? I feel like to know what the time of the notice is. > >>> ... >>> + struct rte_eth_dev *member_dev[RTE_MAX_ETHPORTS]; >>> + >>> + if (valid_bonding_port_id(bonding_port_id) != 0) >>> + return -EINVAL; >>> + >>> + bond_dev = &rte_eth_devices[bonding_port_id]; >>> + internals = bond_dev->data->dev_private; >>> + >>> + for (i = 0; i < internals->member_count; i++) { >>> + member_port_id = internals->members[i].port_id; >>> + member_dev[i] = &rte_eth_devices[member_port_id]; >>> + if (*member_dev[i]->dev_ops->bond_notify_member == NULL) >>> + return -ENOTSUP; >>> + } >> Do we need all member ports support bond_notify_member api? >> If allow user to select one member port to notify? This might be more general. >> In addition, this action here is inconsistent with above handle(do not >> notify member if doesn't supportbond_notify_member API). > Yes, we do not need all member ports support this API. > I just want to have a stricter restriction on this feature before, but I think your suggestion is better. anyway, we need to have a clearly comments about this. > >>> ...