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 3216BA04F1; Mon, 9 Dec 2019 14:36:56 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E8F1D2BAB; Mon, 9 Dec 2019 14:36:54 +0100 (CET) Received: from mail-qk1-f194.google.com (mail-qk1-f194.google.com [209.85.222.194]) by dpdk.org (Postfix) with ESMTP id E7BA223D; Mon, 9 Dec 2019 14:36:53 +0100 (CET) Received: by mail-qk1-f194.google.com with SMTP id x1so12938929qkl.12; Mon, 09 Dec 2019 05:36:53 -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=iwgMRkJyhbwmggsMYTRMMeFUkOAnx3Gaa9VWzGqbH6I=; b=Xnn/7ehUR7N0dDeRBXAi5upp+4oLRgttrXtkMvpX3txT7tnp3LtZjTLttXmWLLbtB/ 5iW97SiMt3RhDlGiZrsHRbuz79FEIxf+Z7WqA2y1qFAOKuU+QSuFvs0+RUCSGf5rOTlP Rud+y9KNk65FU6KYtc/xiOJbv0TFzOXHvLQHmprJcJCp1Knf6e/4HPmiaZJRCOW9MhNl D0EJdMfjC7WNmAGk4VTLJpdq4+Jp3DlnrJ6ixERXo3dD+YrMJLPF2Y6BAaOay6ykNTgV t05VX1U70iDoqHjDQmdND/rv3ZkSo6/QGi3QQ4RPXDRPe13YDXCU10ueRUrtpqfqEPAL rsEw== 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=iwgMRkJyhbwmggsMYTRMMeFUkOAnx3Gaa9VWzGqbH6I=; b=E39Tn9wtVcpR4+iroMpTgyUiSr9X1KEFyuCXgcJwmb4M5TtudQpIeuVSDHIdbwg1v1 LOSv4pMiBc6ZmI7p78NHnDFcKHLEUtIszZ6hKihGGn8cKSk3M7ra1Vmgm6OfjThDUhCS MvKs+Ixh9eZyzyzO36srZ73OTHFjk6cmqGXeg/tvb6vC5ig/EpW1eUZXL+QQ+fD7G6I2 JJQDgGfBTRm+qqeMF77Kh2bTUdbBDH4x2BPTgT4N6ItnCUQfsXtINFgtcxBz+7GEiE8Z 9KNudULaMHipgbx2qjodWP4pBdgIEnrjfUhbWNT0lqkHUk8WT2gzYQ4BNSpTVlfs6RVH QEpw== X-Gm-Message-State: APjAAAUxMBjtNj/nu8KqhdiqBwPEx3g4frB+h1rXeJIJUiRKZl1TgBST uc32S/A/fEobnT53riGMuxY= X-Google-Smtp-Source: APXvYqxjGN5+NNqfh3iKAEy8Z4uZMCVYe4JdVn143JLolO5SnydQcz9Z6kJ5H8Jx237DjmFZ5AMPxw== X-Received: by 2002:a37:9843:: with SMTP id a64mr27060887qke.47.1575898613203; Mon, 09 Dec 2019 05:36:53 -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 z22sm2897374qts.76.2019.12.09.05.36.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 09 Dec 2019 05:36:51 -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> From: Chas Williams <3chas3@gmail.com> Message-ID: Date: Mon, 9 Dec 2019 08:36:51 -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: 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 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. > 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.