From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4784DA04F1; Mon, 9 Dec 2019 15:45:05 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 48BD82C17; Mon, 9 Dec 2019 15:45:04 +0100 (CET) Received: from mail-qt1-f196.google.com (mail-qt1-f196.google.com [209.85.160.196]) by dpdk.org (Postfix) with ESMTP id 178022C12; Mon, 9 Dec 2019 15:45:02 +0100 (CET) Received: by mail-qt1-f196.google.com with SMTP id 38so15692135qtb.13; Mon, 09 Dec 2019 06:45:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=BmjX2h65Dh9GJrM7JyUC9mE4HNVKQS5ZnhPPeHMTBg0=; b=IJgj3htojPk+JopTx/QcKL3wym3OadUXUkKGB9FN70eBTnmCocBrU1Qx25lqiWUXey oBxqU6+Lzdp5v+TS6LwQTn8thxWZMQNdGd0PaMFZwgxyyNCNQHDlQf2segBtjSYrjdD1 Fq6QXf12EuPyvBv9tZ8Tw1ul8jo3Pw+TtpUDvyphjRlM4sliJ3jCzUBuvQG1g2mPapxH rmwF5t5wM/8xPVfNr/98TfWdxDV+klEUgzjfzTdQDGKNzDT6dJSwaNnZWdyauq6ZFJ/K 41B+Ysq7xn+aEqrqaLgrE1tG0a0wqvlix/cgK7wlqC/SB9ys+aKPGBwEvRTQELvgzRwn wi+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=BmjX2h65Dh9GJrM7JyUC9mE4HNVKQS5ZnhPPeHMTBg0=; b=I7FpZG7rVhluFfnjJMhUksr/v5ZM1yPEQYfaJqTeXbyvPMFsm9OAurgP2q+paDOeOx QKiQCLlvI/qCdmoO8ZpgkHdiQo6pSo43ogKqzW6d3yV44RI8qgUzdG2swReyPC9hu/Kj AVkJ4g6NZ1L2nwtqxli9UVcfrsTALkWaUETEQ4BgC95iWzpevHsV4VvEwl2+3tQLLEIX G3NT7oy1zUTlvEzZZ7+9xTseD06LEaWl2TP7feStlU0LVO+5Cg46lCWOQX9Fwp/Zv9+u G2i/dokGKYl6EMsqUJN19VNRBFzDLIe1Yh+MaN/Ttb1lIq0zZnTH2LuKzDRG05iDKtB2 t72g== X-Gm-Message-State: APjAAAXR0x35bPWLcM/qai3ud0YqRvr5yw2q7msTTptwajNBrGwKhNVC uSzfEVhpI/iLwZyjvetU6v4= X-Google-Smtp-Source: APXvYqx+8kyh9ukVLVtX66TXtxsEMZ3H+ZkBfRMeyxx945HpGX9jZX7TtvlOGIROlCKdqpNVRkH4lQ== X-Received: by 2002:ac8:a83:: with SMTP id d3mr25722855qti.228.1575902701368; Mon, 09 Dec 2019 06:45:01 -0800 (PST) Received: from [192.168.1.10] (pool-96-255-60-31.washdc.fios.verizon.net. [96.255.60.31]) by smtp.gmail.com with ESMTPSA id k50sm10230314qtc.90.2019.12.09.06.45.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 09 Dec 2019 06:45:00 -0800 (PST) To: Andrew Rybchenko , Ferruh Yigit , Chas Williams Cc: dev@dpdk.org, stable@dpdk.org, Declan Doherty References: <1574154221-8628-1-git-send-email-arybchenko@solarflare.com> <873a52fd-7108-5428-1ce1-1bb82c023bb0@intel.com> <79c1f419-524c-a4d2-c97f-39789afe4d8a@solarflare.com> <656592a0-bc73-51d0-7ecc-996d027e7d42@gmail.com> <345a511f-34bc-96f9-83af-9550b188d940@solarflare.com> From: Chas Williams <3chas3@gmail.com> Message-ID: <7dbe92d9-268c-c4b5-096d-f0aec02bd872@gmail.com> Date: Mon, 9 Dec 2019 09:45:00 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <345a511f-34bc-96f9-83af-9550b188d940@solarflare.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH] net/bonding: do not inherit slave device configuration X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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 12/9/19 9:31 AM, Andrew Rybchenko wrote: > On 12/9/19 4:36 PM, Chas Williams wrote: >> >> >> On 12/9/19 2:16 AM, Andrew Rybchenko wrote: >>> On 12/8/19 6:44 PM, Chas Williams wrote: >>>> On 2019-11-19 07:40, Andrew Rybchenko wrote: >>>>> On 11/19/19 3:18 PM, Ferruh Yigit wrote: >>>>>> On 11/19/2019 9:03 AM, Andrew Rybchenko wrote: >>>>>>> Bonding device should control bonded devices configuration. >>>>>>> >>>>>>> Also avoid usage of slave's data->dev_conf. >>>>>>> >>>>>>> Fixes: 2efb58cbab6e ("bond: new link bonding library") >>>>>>> Cc: stable@dpdk.org >>>>>>> >>>>>>> Signed-off-by: Andrew Rybchenko >>>>>>> --- >>>>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 24 >> ++++++++++++------------ >>>>>>> 1 file changed, 12 insertions(+), 12 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c >>>> b/drivers/net/bonding/rte_eth_bond_pmd.c >>>>>>> index 707a0f3cdd..4f0e83205d 100644 >>>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c >>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c >>>>>>> @@ -1679,6 +1679,7 @@ int >>>>>>> slave_configure(struct rte_eth_dev *bonded_eth_dev, >>>>>>> struct rte_eth_dev *slave_eth_dev) >>>>>>> { >>>>>>> + struct rte_eth_conf dev_conf; >>>>>>> struct bond_rx_queue *bd_rx_q; >>>>>>> struct bond_tx_queue *bd_tx_q; >>>>>>> uint16_t nb_rx_queues; >>>>>>> @@ -1693,34 +1694,34 @@ slave_configure(struct rte_eth_dev >>>> *bonded_eth_dev, >>>>>>> /* Stop slave */ >>>>>>> rte_eth_dev_stop(slave_eth_dev->data->port_id); >>>>>>> >>>>>>> + memset(&dev_conf, 0, sizeof(dev_conf)); >>>>>>> + >>>>>>> /* Enable interrupts on slave device if supported */ >>>>>>> if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC) >>>>>>> - slave_eth_dev->data->dev_conf.intr_conf.lsc = 1; >>>>>>> + dev_conf.intr_conf.lsc = 1; >>>>>> I assume the original intention is making incremental changes to the >>>> existing >>>>>> slave configuration, if so we should copy the >>>> 'slave_eth_dev->data->dev_conf' to >>>>>> 'dev_conf' before start updating it. >>>>> >>>>> The problem is that I don't understand how incremental changes >>>>> happen. It simply looks wrong or I don't understand something. >>>>> It looks like it is the only place in bonding where slave configuration >>>>> is done. >>>>> >>>> >>>> I understand your confusion. Yes, it certainly looks like >>>> slave_configure() is doing things wrong by directly modifying the >> slave's >>>> data->dev_conf. If rte_eth_dev_configure() fails, the changes made do >>>> get rolled back and become visible anyway despite the device having >>>> failed to meet that configuration. rte_eth_dev_configure() handles the >>>> rollback, but can't do anything in this case because it doesn't know >>>> the device was directly modified. >>>> >>>> You should make a copy of the dev_conf instead of starting from scratch. >>>> There are other capabilities in there that bonding doesn't care about >>>> but the application might. >>> >>> May application configure slave device directly (e.g. before >>> adding in bond) and bonding should respect it? >> >> That's not the issue here. dev_conf contains rx_offload_capa, >> tx_offload_capa et al. You can't just reset those to 0. Those are set >> by the driver's PMD and the application, whether bonding or otherwise, >> needs to be able to examine those. >> >>> Are there usecases behind? >>> Of course, if an application configures both slaves directly >>> and via bonding device, it could understand the configuration, >>> but it looks very error-prone and over-complicated. >>> Wouldn't it be better if bonding device configuration is >>> passed to slaves? >> >> Bonding is a layer on top of the existing ports. It doesn't control >> everything aspect of the bonded interfaces though. Bonding doesn't care >> about the particular offloads a PMD may or may not support. That's an >> application issue. For instance, your application may not be able >> to support scatter/gather. That's not really bonding's concern. > > OK, I see now. > > As I understand it requires from applications to be bonding- > aware. I.e. an application which works with a provided DPDK > port cannot accept bonding port since bonding port differs > a lot and requires application to configure slave ports first > to enable offloads etc. IMHO it is a drawback and limitation. I don't think you can hide all the messy bits of the bonding PMD by adding more API. If your application can already correctly operate with a particular port, then the bonding PMD doesn't do anything new. The bonding PMD could be viewed as the set of the common capabilities of the slaves. If your application can't use the port without bonding, adding bonding as an additional layer won't help. Most applications will need some awareness of bonding to know that they need to ignore what the bonding presents as capabilities because the capabilities of the bonding PMD are really just those of the individual ports which the application should have already vetted. >>> May be the reason behind is that net/bonding does not forward >>> configuration to slaves except RSS configuration right now. >> >> Bonding does try to forward some configuration to the slaves, atleast >> where it makes sense. If you change the bonding MTU for instance, this >> is forwarded to the slaves. It doesn't make sense to change the >> bonding interface MTU without also changing the slave MTUs. >> >>> Is the behaviour documented anywhere? >> >> Not that I am aware of. >> >>> Of course, any changes in the area would be behaviour change >>> which should be documented in release notes at least or >>> even go through deprecation process. >> >> You current patch does propose a signficiant change because it clears >> any existing configuration on the slave PMDs. If you simply copy the >> slave's dev_conf first, you can avoid making a change. > > Yes, it is pretty clear now. I'm just trying to understand > theory of how it is expected to work right now. Many > thanks for explanations. > > I'll update the patch. > > Does it make sense to address discuss above limitation > in the future? For some features, possibly. One example of this issue is how to handle reception of the LACPDUs. Some PMDs can only support promiscuous mode, others are smarter and could potentially just receive the particular multicast group. Is this something the application should configure? Is this something that the bonding PMD should setup? If bonding does it, does the user need fine-grained control?