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 78E7BA04F1; Sun, 8 Dec 2019 16:44:35 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 775E01BF8C; Sun, 8 Dec 2019 16:44:34 +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 7AE4B1BF89; Sun, 8 Dec 2019 16:44:33 +0100 (CET) Received: by mail-qt1-f196.google.com with SMTP id k11so12910382qtm.3; Sun, 08 Dec 2019 07:44:33 -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=1T0/Vnp6PUTVfi6dHPAfLySMtW2dRf81dJABlUkQk8k=; b=Ca25eevB3n4i+zthkpDdUnnDU4QS326P1kJAl7Q9H+t6UNwOmN/Q28yAkAYjOCgzP1 6s31F/tFTNXGlkKU3K7a6muIFMTc8a2l+xDLPfFLITpY13LjBrFLbeMUvgy9p9tf/Ina +r+oRYVqkjDpef9iP9LSle6GN0dcWbtZ7n9AxXMdpNrvcBXpkypBcOHBI2JWIe5qntdu fdUQI6E8Gv6evAWpeGoaiVh0nCcgTghwBUAgb24oK377MqpETDf0brv6cBXcVSQoCIJM 9TYrl1HwnnR8Hu4psMNTfL+M/DU0ONFAlaYF2CKMNV/wHUS6tDOZzB/U2jvs3wABPFBt 3uIw== 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=1T0/Vnp6PUTVfi6dHPAfLySMtW2dRf81dJABlUkQk8k=; b=YYWqvB2VgB0NPYclmaLMfLmt1ju9FZCw0MNkI+/Es+amLHJaIM2wL91PCNRQYB2aXC KB7gr9xvoR+zpJhSa2iGDrCtXyoUhAf1enrPkW98RJn4x8hjT25DetpfXJIJIYs7BPha bzaKJwRQPJv9sqj9t3irskZZXS6A3zNo5NKFd8sz4IF/t0sRb/HGMzchUCtGQQ9X5Bl3 vcDrHse8lf99bX0FBz50KinnO2h/zJpx4/cbsWsCdL91SGfjhYUWC8QpuVkIoT9eCnuQ DJsfEoalWBs2dwBU53zL9rH5rGzCH1o04/K8BTqqux2CBVaGGOyIhDZTWlLOJJyWtza0 mp7w== X-Gm-Message-State: APjAAAXd4JrrYrILX2R41b7VXT7UCgBNdtYafiwH0rGOzN5kBiHv0ga6 YW78hDnFYJcbsI7AZk0iv+g= X-Google-Smtp-Source: APXvYqwxDW6h57+JnL19Rt7Zk5sxAqEw+eXaAeRXoAYAXfB8TGM7m7LDGUwczMTFreXuxwuDdFpDyQ== X-Received: by 2002:ac8:2aad:: with SMTP id b42mr22025929qta.166.1575819872729; Sun, 08 Dec 2019 07:44:32 -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 o7sm4297153qkd.119.2019.12.08.07.44.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 08 Dec 2019 07:44:32 -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> From: Chas Williams <3chas3@gmail.com> Message-ID: <656592a0-bc73-51d0-7ecc-996d027e7d42@gmail.com> Date: Sun, 8 Dec 2019 10:44:31 -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: <79c1f419-524c-a4d2-c97f-39789afe4d8a@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 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.