From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by dpdk.org (Postfix) with ESMTP id D069F2C17 for ; Thu, 7 Mar 2019 10:48:00 +0100 (CET) Received: by mail-wr1-f68.google.com with SMTP id g12so16621948wrm.5 for ; Thu, 07 Mar 2019 01:48:00 -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=b777dK5LU1MRPQnpeiXuIlraLSd8p3Yh2ACBqKJZcx4=; b=quxEUwceX+L6r10r/nMMPFzj+dJXEtbfgYGXNa/w6sJb+uevI/aQ3pKX+2Xx3fXU9I dUeurfn5V9OW/0xTqh0JOiTHlgrNzhOIVdNyfYFBuKkmfMm5IrpCECJtf0g/sftRJzmy VitP9uKops5MJIW4T+DFLh/LySCwHW+BX4qxFfgyH75qGcJLqcGpJE+ZY0hXwZh/Qhm7 SOb8v34f11crV/ca5dbwP2b0Z7ge9rGT0Gs7gAykAI78ioMYXqtVcjuZa+gdTrA5D61u D2bf+19kodOomFgoz5w4IY/DHYDXwmAFu6obR8Ye8TTe1BSuLR/j1lti9RyFHv8gnK6C m3PQ== 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=b777dK5LU1MRPQnpeiXuIlraLSd8p3Yh2ACBqKJZcx4=; b=NVveWOXbWkKOPh8CbFJyiN+n734ZYXxDd53324PobltFxmcXezDVFuLiGlAHECa2w3 nYuxPQ2y9n8j2Gt6yAwXF5Gpe+vwZ7uedMefdWkvZ6UlOF+iBms+hSCYq2+sDSPkb7eW 6x4wApewkpbnu9/udLn3BKyshBpbn8dtkKfA82yDYQAfu8SvaXDSJkZn596wCEJqWVrp 8aZmxwCC3jySjZFLj+zFiS0Zpb4sdOxZe+txjfVrOWdoL9RiKLKJ1ZRQpQOMjByYD+dP ic8cgFnWJAbOVsJd+JBifqh8QbJBHU4OpzUJ6vbetJaNwtUEIWINFJcOwkHOgMMrnHt/ 7f4Q== X-Gm-Message-State: APjAAAU8U+SYLcEIQmQKku1q5fSs7UcrbT1pJd7gcYAulvmacTI0Sfra /h0Cfpc9rMu02ay6BKd2HkJ7a288AxE= X-Google-Smtp-Source: APXvYqzoqeD5YX3IcHxFTMvDiyISaRlYjiR2sNwKK+j5OKqHyh/CF2FEs+E/ik7wu8Zy3Rf0PkjDvg== X-Received: by 2002:a5d:570a:: with SMTP id a10mr6267958wrv.85.1551952080349; Thu, 07 Mar 2019 01:48:00 -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 r63sm8178575wmr.32.2019.03.07.01.47.59 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 07 Mar 2019 01:47:59 -0800 (PST) Date: Thu, 7 Mar 2019 10:47:57 +0100 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Raslan Darawsheh Cc: Thomas Monjalon , "dev@dpdk.org" , "stephen@networkplumber.org" Message-ID: <20190307094757.56db3w74ftqkra7d@bidouze.vm.6wind.com> References: <1551779507-10857-1-git-send-email-rasland@mellanox.com> <172443910.dRk5910QrU@xps> <20190306104632.fia2r2sz5yajvkne@bidouze.vm.6wind.com> <6541291.VHOuPfexjb@xps> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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: Thu, 07 Mar 2019 09:48:00 -0000 On Thu, Mar 07, 2019 at 08:43:01AM +0000, Raslan Darawsheh wrote: > Hi, > > > -----Original Message----- > > From: Thomas Monjalon > > Sent: Wednesday, March 6, 2019 8:02 PM > > To: Gaëtan Rivet ; Raslan Darawsheh > > > > Cc: dev@dpdk.org; stephen@networkplumber.org > > Subject: Re: [dpdk-dev] [PATCH v2 3/4] net/failsafe: replace local sub-device > > with shared data > > > > 06/03/2019 11:46, Gaëtan Rivet: > > > On Tue, Mar 05, 2019 at 06:58:04PM +0100, Thomas Monjalon wrote: > > > > 05/03/2019 18:38, Gaëtan Rivet: > > > > > 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? > > > > > > > > Is the secondary process supposed to do any closing? > > > > The device management should be done only by the primary process. > > > > > > > > Note: anyway all this hotplug related code should be dropped from > > > > failsafe to be replaced by EAL hotplug management. > > > > > > > > > > I don't know, I've never used secondary process. > > > However, cursory reading the code of rte_eth_dev_close(), I don't see > > > a guard against calling it from a secondary process? > > > > Yes indeed, there is no guard. > > That's something not clear in DPDK, previously we were attaching some > > vdevs in secondary only. > > > > > Reading code like > > > > > > rte_free(dev->data->rx_queues); > > > dev->data->rx_queues = NULL; > > > > > > within makes me think the issue has been seen at least once, where > > > shared data is freed multiple times, so I guessed some secondary > > > processes were calling it. Maybe they are not meant to, but what > > > prevents them from being badly written? > > > > > > Also, given rte_dev_remove IPC call to transfer the order to the > > > primary, it seems that at least secondary processes are expected to > > > call > > > rte_dev_remove() at some point? So are they only authorized to call > > > rte_dev_remove() (to manage hotplug), but not rte_eth_dev_close()? Is > > > there a specific documentation detailing the design of secondary > > > process and the related responsibilities in the lifetime of a device? > > > How are they synching their rte_eth_devices list if they are not > > > calling rte_eth_dev_close(), ever? > > > > All these calls should be done in primary. > > The IPC mechanism calls the attach/detach in secondary at EAL level. > > The PMDs does the bridge between EAL device and ethdev port status. > > But you are right, there can be a sync issue if closing an ethdev port and not > > removing the EAL device. > > This is a generic question about deciding whether we want all ethdev ports to > > be synced in multi-process or not. > > > > In failsafe context, we are closing the EAL device and change the state of the > > sub-device accordingly. So I think there is no issue. > > > > > > > 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...). > > > > > > > > Which data do you think should be allocated per process? > > > > > > > > > > > > > > [-------- SHARED SPACE --------------] [-- PER-PROCESS --------] > > > +--------------------------------------------------------------+ > > > | +------------------+ +- rte_eth_devices[n] -+ | > > > | |rte_eth_dev_data |<---------------+ data | | PRIMARY > > > | | | +dev_priv-+ | | | > > > | | dev_private +-->| | | | | > > > | | ... | | | | | | > > > | | port_id | | | | | | > > > | | | | | | | | > > > | | | | | | | | > > > | | | | | +----------------------+ | > > > | | | | | +- rte_eth_devices[n] -+ | > > > | | | | | | | | SECONDARY > > > | | | | | | | | > > > | | | | | | | | > > > | | | | | | | | > > > | | | +---------+ | | | > > > | | |<---------------+ data | | > > > | +------------------+ +----------------------+ | > > > +--------------------------------------------------------------+ > > > > > > Here port_id is used within fail-safe to get back to rte_eth_devices[n]. > > > This disappears once a device is closed, as all shared space is zeroed. > > > > > > This means that sometimes ETH(sdev) and PORT_ID(sdev) is correct, and > > > at some point it is not anymore, once a sub-device has been closed. > > > This seems dangerous. > > > > The state of the sub-device is changed. > > I don't see any issue. > > > > > I was thinking initially that allocating a place where each sdev would > > > store their rte_eth_devices / port_id back-reference could alleviate > > > the issue, meaning that the fail-safe would not zero it on > > > sdev_close(), and it would remain valid for the lifetime of a > > > sub-device, so even when a sub-device is in DEV_PROBED state. > > > > > > But now that I think about it, it could probably be simpler: instead > > > of using (ETH(sdev)->data->port_id) for the port_id of an sdev > > > (meaning that it is dependent on the lifetime of the sdev, instead of > > > the lifetime of the failsafe), the port-id itself should be stored in > > > the sub_device structure. This structure will be available for the > > > lifetime of the failsafe, and the port_id is correct accross all processes. > > > > > > So PORT_ID(sdev) would be defined to something like (sdev->port_id), > > > and > > > ETH(sdev) would be (&rte_eth_devices[PORT_ID(sdev)]). It would remain > > > correct even once the primary has closed the sub-device. > > > > > > What do you think? Do you agree that the current state is dangerous, > > > and do you think the solution would alleviate the issue? Maybe the > > > concern is unfounded and the only issue is within fs_dev_remove(). > > > > Yes it is only seen in fs_dev_remove(). > > I discussed about your proposal with Raslan, and we agree we could change > > from sub_device.data to sub_device.port_id, it may be more future-proof. > > > > I have only one doubt: look at the macro in this patch: > > > > #define ETH(sdev) \ > > ((sdev)->data == NULL ? NULL : &rte_eth_devices[(sdev)->data- > > >port_id]) > > > > The NULL check cannot be done with a port id. > > I think it was needed to manage one case. Raslan? > > That's right since we need it for fs_tx_unsafe, to add a protection for plugged out devices during TX. Ok, thanks for your insights Thomas and Raslan. Sorry about the rambling above I needed to write down the stuff to think about it. You can use RTE_MAX_ETHPORTS as a sentinel value for port_id, this way the value is kept unsigned and there are several checks against this specific value otherwise. so ETH(sdev) could be (PORT_ID(sdev) >= RTE_MAX_ETHPORTS ? NULL : &rte_eth_devices[PORT_ID(sdev)]) -- Gaëtan Rivet 6WIND