From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com [66.111.4.26]) by dpdk.org (Postfix) with ESMTP id 201292B93 for ; Wed, 6 Mar 2019 19:02:16 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 8069F25637; Wed, 6 Mar 2019 13:02:15 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Wed, 06 Mar 2019 13:02:15 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=bgvB23rAduTkB/u/PAIjWypopbjlkYdJiowEkH1DeS0=; b=czTi/NC2A2IO zqqYaow4e6mO9Dw/qlNjeD11tpUCWSs3qdKebAhJ3uTPvJvSO+fiFFaEKIcy+fh4 Xbo1ylmTxETb2aHRhfsu8Vyiko0CA8DFI8w5Bq2xspRudi0ZM4N2LA+KbnIejogO XOLbS616UXJ4G5zfSY95+97fFoUqNX4= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=bgvB23rAduTkB/u/PAIjWypopbjlkYdJiowEkH1De S0=; b=E8H1117c2+1b4JKWvJ20FWu5cPXYW2jCGSnH/R2+Kgt+lDINf+MbvDLdP c7y56M2mpZm4my9+h+V4c06XYRdsc6OLjjbKcpKW593nqe/QwSfHFty2BkWNdLi8 B/S2bj4ovKwgNloVIk0Bikk8fJ+H2t5YqGBAmgfCBX/xIcR5dzW05X/JQW9Iq4MP lqMWDkZh8ulLHLMZMcACqSuyr4cDrzZM+R2nShfU8+ScK0KK1d90y8hNOyr88NaG 9w9Yc5+3/tk5kFkMYgmwxrXKvMVSPtrr4ureTmrift3NYr9gUw2V0s7r1gm7lYfg Ai8tv68Pa3qZRClrTq3/8ArwQ8jGw== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedutddrfeeigdegjecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthhqredttddtudenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucfkph epjeejrddufeegrddvtdefrddukeegnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhho mhgrshesmhhonhhjrghlohhnrdhnvghtnecuvehluhhsthgvrhfuihiivgeptd X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 5ADBB10341; Wed, 6 Mar 2019 13:02:13 -0500 (EST) From: Thomas Monjalon To: =?ISO-8859-1?Q?Ga=EBtan?= Rivet , Raslan Darawsheh Cc: dev@dpdk.org, "stephen@networkplumber.org" Date: Wed, 06 Mar 2019 19:02:11 +0100 Message-ID: <6541291.VHOuPfexjb@xps> In-Reply-To: <20190306104632.fia2r2sz5yajvkne@bidouze.vm.6wind.com> References: <1551779507-10857-1-git-send-email-rasland@mellanox.com> <172443910.dRk5910QrU@xps> <20190306104632.fia2r2sz5yajvkne@bidouze.vm.6wind.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="iso-8859-1" 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: Wed, 06 Mar 2019 18:02:16 -0000 06/03/2019 11:46, Ga=EBtan Rivet: > On Tue, Mar 05, 2019 at 06:58:04PM +0100, Thomas Monjalon wrote: > > 05/03/2019 18:38, Ga=EBtan Rivet: > > > What happens when a primary process closes a device before a secondar= y? > > > Is the secondary unable to stop / close its own then? Isn't there some > > > missing uninit? > >=20 > > Is the secondary process supposed to do any closing? > > The device management should be done only by the primary process. > >=20 > > Note: anyway all this hotplug related code should be dropped > > from failsafe to be replaced by EAL hotplug management. > >=20 >=20 > 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 >=20 > rte_free(dev->data->rx_queues); > dev->data->rx_queues =3D NULL; >=20 > 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? >=20 > 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...). > >=20 > > Which data do you think should be allocated per process? > >=20 > >=20 >=20 > [-------- 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 | | > | +------------------+ +----------------------+ | > +--------------------------------------------------------------+ >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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 =3D=3D 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?