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 54E61A0C51; Thu, 10 Jun 2021 08:29:08 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D89DA4067C; Thu, 10 Jun 2021 08:29:07 +0200 (CEST) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id 664094003C for ; Thu, 10 Jun 2021 08:29:06 +0200 (CEST) Received: from dggeme765-chm.china.huawei.com (unknown [172.30.72.57]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4G0v7t54L2z1BG9Z; Thu, 10 Jun 2021 14:24:10 +0800 (CST) Received: from [127.0.0.1] (10.69.27.114) by dggeme765-chm.china.huawei.com (10.3.19.111) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2176.2; Thu, 10 Jun 2021 14:29:03 +0800 To: Andrew Rybchenko , "Ananyev, Konstantin" , "dev@dpdk.org" References: <1618571071-5927-1-git-send-email-tangchengchang@huawei.com> <1619171202-28486-1-git-send-email-tangchengchang@huawei.com> <1619171202-28486-3-git-send-email-tangchengchang@huawei.com> <4b1e8435-c25c-b490-c196-9aebdee5733a@huawei.com> <4fa26208-464d-e255-e5e7-21d4e0160bab@oktetlabs.ru> CC: "linuxarm@huawei.com" , "chas3@att.com" , "humin29@huawei.com" , "Yigit, Ferruh" From: Chengchang Tang Message-ID: Date: Thu, 10 Jun 2021 14:29:03 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <4fa26208-464d-e255-e5e7-21d4e0160bab@oktetlabs.ru> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.69.27.114] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To dggeme765-chm.china.huawei.com (10.3.19.111) X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH 2/2] net/bonding: support configuring Tx offloading for bonding 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" Hi, Andrew and Ananyev On 2021/6/9 17:37, Andrew Rybchenko wrote: > On 6/9/21 12:11 PM, Ananyev, Konstantin wrote: >> >>> >>> >>> On 2021/6/8 17:49, Andrew Rybchenko wrote: >>>> "for bonding" is redundant in the summary since it is already >>>> "net/bonding" >>>> >>>> On 4/23/21 12:46 PM, Chengchang Tang wrote: >>>>> Currently, the TX offloading of the bonding device will not take effect by >>>> >>>> TX -> Tx >>>> >>>>> using dev_configure. Because the related configuration will not be >>>>> delivered to the slave devices in this way. >>>> >>>> I think it is a major problem that Tx offloads are actually >>>> ignored. It should be a patches with "Fixes:" which addresses >>>> it. >>>> >>>>> The Tx offloading capability of the bonding device is the intersection of >>>>> the capability of all slave devices. Based on this, the following functions >>>>> are added to the bonding driver: >>>>> 1. If a Tx offloading is within the capability of the bonding device (i.e. >>>>> all the slave devices support this Tx offloading), the enabling status of >>>>> the offloading of all slave devices depends on the configuration of the >>>>> bonding device. >>>>> >>>>> 2. For the Tx offloading that is not within the Tx offloading capability >>>>> of the bonding device, the enabling status of the offloading on the slave >>>>> devices is irrelevant to the bonding device configuration. And it depends >>>>> on the original configuration of the slave devices. >>>>> >>>>> Signed-off-by: Chengchang Tang >>>>> --- >>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 13 +++++++++++++ >>>>> 1 file changed, 13 insertions(+) >>>>> >>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c >>>>> index 84af348..9922657 100644 >>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c >>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c >>>>> @@ -1712,6 +1712,8 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev, >>>>> struct rte_flow_error flow_error; >>>>> >>>>> struct bond_dev_private *internals = bonded_eth_dev->data->dev_private; >>>>> + uint64_t tx_offload_cap = internals->tx_offload_capa; >>>>> + uint64_t tx_offload; >>>>> >>>>> /* Stop slave */ >>>>> errval = rte_eth_dev_stop(slave_eth_dev->data->port_id); >>>>> @@ -1759,6 +1761,17 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev, >>>>> slave_eth_dev->data->dev_conf.rxmode.offloads &= >>>>> ~DEV_RX_OFFLOAD_JUMBO_FRAME; >>>>> >>>>> + while (tx_offload_cap != 0) { >>>>> + tx_offload = 1ULL << __builtin_ctzll(tx_offload_cap); >>>>> + if (bonded_eth_dev->data->dev_conf.txmode.offloads & tx_offload) >>>>> + slave_eth_dev->data->dev_conf.txmode.offloads |= >>>>> + tx_offload; >>>>> + else >>>>> + slave_eth_dev->data->dev_conf.txmode.offloads &= >>>>> + ~tx_offload; >>>>> + tx_offload_cap &= ~tx_offload; >>>>> + } >>>>> + >>>> >>>> Frankly speaking I don't understand why it is that complicated. >>>> ethdev rejects of unsupported Tx offloads. So, can't we simply: >>>> slave_eth_dev->data->dev_conf.txmode.offloads = >>>> bonded_eth_dev->data->dev_conf.txmode.offloads; >>>> >>> >>> Using such a complicated method is to increase the flexibility of the slave devices, >>> allowing the Tx offloading of the slave devices to be incompletely consistent with >>> the bond device. If some offloading can be turned on without bond device awareness, >>> they can be retained in this case. >> >> >> Not sure how that can that happen... > > +1 > > @Chengchang could you provide an example how it could happen. > For example: device 1 capability: VLAN_INSERT | MBUF_FAST_FREE device 2 capability: VLAN_INSERT And the capability of bonded device will be VLAN_INSERT. So, we can only set VLAN_INSERT for the bonded device. So what if we want to enable MBUF_FAST_FREE in device 1 to improve performance? For the application, as long as it can guarantee the condition of MBUF ref_cnt = 1, then it can run normally if MBUF_FAST_FREE is turned on. In my logic, if device 1 has been configured with MBUF_FAST_FREE, and then added to the bonded device as a slave. The MBUF_FAST_FREE will be reserved. >> From my understanding tx_offload for bond device has to be intersection of tx_offloads >> of all slaves, no? Otherwise bond device might be misconfigured. >> Anyway for that code snippet above, wouldn't the same be achived by: >> slave_eth_dev->data->dev_conf.txmode.offloads &= internals->tx_offload_capa & bonded_eth_dev->data->dev_conf.txmode.offloads; >> ? > I think it will not achieved my purpose in the scenario I mentioned above. > . >