From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by dpdk.org (Postfix) with ESMTP id 085162BE9 for ; Tue, 5 Mar 2019 18:38:29 +0100 (CET) Received: by mail-wr1-f65.google.com with SMTP id g12so10414176wrm.5 for ; Tue, 05 Mar 2019 09:38:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=vwGn91osICnelIUHIxguZfG5D0CTCaq3HxgOXZnINWM=; b=rS2wJV7/wG9epP5SONlG+cUqll/zFA7saeVVSZLqP1XnPP917u5h1O0zNL/FsODm1f 9GnqPZOoWXf7MWxtQrU/7+5ylxpO6mznlcU6JHloPxS72/d8PJQXYrS1rMWDU7PN24FF j11DglLiSWiAFrdyD+O9tJSq5rLxYzJs4xHwY8yD4OW3OdKiClXtBS19B3WWd4J+2nrH VASLlQob6XJ9lKqpAa8YSJdvu8Xfbvqx7Iy4ji2rAXCN7MERTWkYNDddp0ZEDN4/SssF aBX+o/mdOIxaCarQTro2vP5qvlATLnMhFUnHupQ2NpOztSyMl884X45U/fiB9GYEF5mv +6DA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=vwGn91osICnelIUHIxguZfG5D0CTCaq3HxgOXZnINWM=; b=smzOqpyBrchtNsj0DiP82vG+DG314IO+NzB4P/GHnY0zDzMPd5X2oqv19lcBsVL9E0 Tj/A2TzX9xcAiliLCnyDrPC/Wfx+hhp2GQOnq0Bkk/D8qS2sVtsXvYJcQZrX/rXt6Kq1 U0FnlKOC+79N5y5P9lUTGP/gWZc+nWPqa2PnT8PxEp/XVeotTYNpx2CS7LgNurxu470n bA+oy8H6ASmY/zxAHihruRNN5vfpJn6s/Ad8oB1JYvnaYXzGXHQHCpcA5fMOeqDP3jN1 IgyjRf4eALTfp9laSAd7WnqJlWnNba3K2uwrLkNWVMaoeaIDpH/AXVPcCho4uRiptD7h OvpQ== X-Gm-Message-State: APjAAAWnDtlsTB35amksybFoZNcGRRo0wvydV0ERRtmIIBQa+9u4WAJ8 BtueYNFxLnzvolhtqLPwGP7trg== X-Google-Smtp-Source: APXvYqwHRuxy3dW3aU2cK+rofBtG9q4D1UsyX8zyoH2ZZYbPuZlFDuvDjmYiNOQkPLZEWt750k+SXw== X-Received: by 2002:adf:9cc3:: with SMTP id h3mr19806wre.47.1551807508573; Tue, 05 Mar 2019 09:38:28 -0800 (PST) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id t2sm148606wmi.37.2019.03.05.09.38.27 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 05 Mar 2019 09:38:27 -0800 (PST) Date: Tue, 5 Mar 2019 18:38:25 +0100 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Raslan Darawsheh Cc: "dev@dpdk.org" , Thomas Monjalon , "stephen@networkplumber.org" Message-ID: <20190305173825.2ho27gqfx7f72xqb@bidouze.vm.6wind.com> References: <1551779507-10857-1-git-send-email-rasland@mellanox.com> <1551779507-10857-3-git-send-email-rasland@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1551779507-10857-3-git-send-email-rasland@mellanox.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v2 3/4] net/failsafe: replace local sub-device with shared data 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: , X-List-Received-Date: Tue, 05 Mar 2019 17:38:29 -0000 On Tue, Mar 05, 2019 at 09:52:05AM +0000, Raslan Darawsheh wrote: > In multiprocess context, the pointer to sub-device is shared between > processes. Previously, it was a pointer to per process eth_dev so > it's needed to replace this dependency. > > Signed-off-by: Thomas Monjalon > Signed-off-by: Raslan Darawsheh > --- > v2: - moved comment in fs_sdev about subs to this commit > - added parenthesis around macro arguments. > --- > drivers/net/failsafe/failsafe_eal.c | 2 +- > drivers/net/failsafe/failsafe_ether.c | 7 ++++--- > drivers/net/failsafe/failsafe_private.h | 13 ++++++++----- > 3 files changed, 13 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c > index 56d1669..6fac4b6 100644 > --- a/drivers/net/failsafe/failsafe_eal.c > +++ b/drivers/net/failsafe/failsafe_eal.c > @@ -112,7 +112,7 @@ fs_bus_init(struct rte_eth_dev *dev) > continue; > } > } > - ETH(sdev) = &rte_eth_devices[pid]; > + sdev->data = rte_eth_devices[pid].data; > SUB_ID(sdev) = i; > sdev->fs_port_id = dev->data->port_id; > sdev->dev = ETH(sdev)->device; > diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c > index d5b1488..e1fff59 100644 > --- a/drivers/net/failsafe/failsafe_ether.c > +++ b/drivers/net/failsafe/failsafe_ether.c > @@ -267,18 +267,19 @@ static void > fs_dev_remove(struct sub_device *sdev) > { > int ret; > + struct rte_eth_dev *edev = ETH(sdev); I'd have added that above the "int ret;". (inverse christmas tree and all that.) > > if (sdev == NULL) > return; > switch (sdev->state) { > case DEV_STARTED: > failsafe_rx_intr_uninstall_subdevice(sdev); > - rte_eth_dev_stop(PORT_ID(sdev)); > + rte_eth_dev_stop(edev->data->port_id); > sdev->state = DEV_ACTIVE; > /* fallthrough */ > case DEV_ACTIVE: > failsafe_eth_dev_unregister_callbacks(sdev); > - rte_eth_dev_close(PORT_ID(sdev)); > + rte_eth_dev_close(edev->data->port_id); Ok I see. I missed that during the first reading, the private_data is zeroed on dev_close(), so ETH(sdev) becomes invalid here. What happens when a primary process closes a device before a secondary? Is the secondary unable to stop / close its own then? Isn't there some missing uninit? This seems dangerous to me. Why not instead allocating a per-process slab of memory that would hold the relevant references and outlive the shared data (a per-process rte_eth_dev private data...). > sdev->state = DEV_PROBED; > /* fallthrough */ > case DEV_PROBED: > @@ -287,7 +288,7 @@ fs_dev_remove(struct sub_device *sdev) > ERROR("Bus detach failed for sub_device %u", > SUB_ID(sdev)); > } else { > - rte_eth_dev_release_port(ETH(sdev)); > + rte_eth_dev_release_port(edev); > } > sdev->state = DEV_PARSED; > /* fallthrough */ > diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h > index 84e847f..1e2ad2d 100644 > --- a/drivers/net/failsafe/failsafe_private.h > +++ b/drivers/net/failsafe/failsafe_private.h > @@ -100,13 +100,16 @@ struct fs_stats { > uint64_t timestamp; > }; > > +/* > + * Allocated in shared memory. > + */ > struct sub_device { > /* Exhaustive DPDK device description */ > struct sub_device *next; > struct rte_devargs devargs; > - struct rte_bus *bus; > - struct rte_device *dev; > - struct rte_eth_dev *edev; > + struct rte_bus *bus; /* per process. */ > + struct rte_device *dev; /* per process. */ > + struct rte_eth_dev_data *data; /* shared between processes */ > uint8_t sid; > /* Device state machine */ > enum dev_state state; > @@ -139,7 +142,7 @@ struct fs_priv { > * subs[0] is the preferred device > * any other is just another slave > */ > - struct sub_device *subs; > + struct sub_device *subs; /* shared between processes */ > uint8_t subs_head; /* if head == tail, no subs */ > uint8_t subs_tail; /* first invalid */ > uint8_t subs_tx; /* current emitting device */ > @@ -254,7 +257,7 @@ extern int failsafe_mac_from_arg; > > /* sdev: (struct sub_device *) */ > #define ETH(sdev) \ > - ((sdev)->edev) > + ((sdev)->data == NULL ? NULL : &rte_eth_devices[(sdev)->data->port_id]) > > /* sdev: (struct sub_device *) */ > #define PORT_ID(sdev) \ > -- > 2.7.4 > -- Gaëtan Rivet 6WIND