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 149BDA0C44; Mon, 14 Jun 2021 16:13:44 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 82F914067A; Mon, 14 Jun 2021 16:13:43 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 8F77B4003F for ; Mon, 14 Jun 2021 16:13:41 +0200 (CEST) Received: from [192.168.1.71] (unknown [188.170.85.171]) (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 BDCF87F5B3; Mon, 14 Jun 2021 17:13:40 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru BDCF87F5B3 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1623680021; bh=DZlZT2wPDAJKJdCXVJRkn+f8PlW87XGR7ME0uDPqolY=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=gd1+KFUdjyLKcw8q2esRN+Ls+dHsXLR5fuIYfUaQT01BXdZBNzuw2+eslUazTJQ6B zWcg7aBuk8lpx1E6OcxBfIzVMapHECk6j2tn2kllgFwhqNBo12v5XNCZHv/94CUDfb 8eWkhNoyoVPje9nbQpOByk+pxLHJUzagez1vQg2I= To: "Ananyev, Konstantin" , Chengchang Tang , "dev@dpdk.org" Cc: "linuxarm@huawei.com" , "chas3@att.com" , "humin29@huawei.com" , "Yigit, Ferruh" 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> From: Andrew Rybchenko Message-ID: <4042d357-85e7-6e48-746a-02563132d4d2@oktetlabs.ru> Date: Mon, 14 Jun 2021 17:13:40 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.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 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" On 6/14/21 2:05 PM, Ananyev, Konstantin wrote: > > >> 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. > > So your intention is to allow slave device silently overrule master tx_offload settings? > If so, I don't think it is a good idea - sounds like potentially bogus and error prone approach. +1 > Second thing - I still don't see how the code above can help you with it. > From what I read in your code - you clear tx_offload bits that are not not supported by the master. +1 >> >>>> 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. >> >>> . >>> >