From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by dpdk.org (Postfix) with ESMTP id 096DF239 for ; Wed, 6 Mar 2019 11:46:36 +0100 (CET) Received: by mail-wr1-f67.google.com with SMTP id d17so12817655wre.10 for ; Wed, 06 Mar 2019 02:46:36 -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=xbM1lJp6jO9uJQYUNResyPYyfq8GaLhvgeZy8TRSkX4=; b=OBdTK8FNF+kGSA2DR5oeRmDePwG17BzuOgGsNfDany2MG3erKzRHXMoCpsPtpFC4RS lko79AySHdYjDWqqMJySMx4KATYB116vOiIUyDce4Nr3eQiz+UWH4Z9L3jufUlSknbZz PJUh/5bZSHWCo6bJFV8bnQEO9U5H5LaI/dhnkmBIy2XiPcjphBbih/kRrjW10qCnRHDB Jdi60GMsIwth6o7lEXkEh3cmChoQtWXjTujUwE6DhBtef/7I4msK/pogFFuSlEkTOmkm TtC/FXKmG0WP+jgyJYaht/hvrLVuSOrDXMKtHbw1JrYg+6Xs+OYVqkn2L8ii/C1MrpSK mBxQ== 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=xbM1lJp6jO9uJQYUNResyPYyfq8GaLhvgeZy8TRSkX4=; b=LuJEXPlWdXGoFmDHVGhwZv8rwVTiVuTtF5PAyte3SPZotw0t7uYB1wLmkxAgxzXdWF KBxuqyUJgzMF/GxyhXSra4Sl49qUZ1R1vgQdCVkD3XcjCJ+Hppk3DgvewmtzP8Lrt/kC j4UNr61Zhv+Y50xcdBGxrzLHuj/4PU48EnBohpasUgIlCjiPkrxte+O7o8/VrHWkOpsq oVCW4dFnW3ZHrK6VZGtjkGKyMeMFWTT1rUDqw7wunvAVwwOeBB8Ajdii0VF5T22TsdUS njVB5U6QXH5sTAEJmf6jE9ZVOsvPV689bekhbE1v9MJZGSXb/4nJrlm0Kl7WvubIW6tA +J8w== X-Gm-Message-State: APjAAAVLWrOrqRjwDo87XajhZXZ/Mh/S28uv4oBIv6W4xScbRCQJyUPu dAy8oBRyFlSqKRaM07MtPD2XAA== X-Google-Smtp-Source: APXvYqyb7zZOqIxbGSLtibtFskpboKF+aIdeFe0orI5V3iI8k6J/erCtJ6rnCPnJZ5KzhAnDijWkYw== X-Received: by 2002:a5d:6542:: with SMTP id z2mr2420891wrv.237.1551869195423; Wed, 06 Mar 2019 02:46:35 -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 c1sm3883205wrw.7.2019.03.06.02.46.34 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 06 Mar 2019 02:46:34 -0800 (PST) Date: Wed, 6 Mar 2019 11:46:32 +0100 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Thomas Monjalon Cc: Raslan Darawsheh , "dev@dpdk.org" , "stephen@networkplumber.org" Message-ID: <20190306104632.fia2r2sz5yajvkne@bidouze.vm.6wind.com> References: <1551779507-10857-1-git-send-email-rasland@mellanox.com> <1551779507-10857-3-git-send-email-rasland@mellanox.com> <20190305173825.2ho27gqfx7f72xqb@bidouze.vm.6wind.com> <172443910.dRk5910QrU@xps> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <172443910.dRk5910QrU@xps> 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: Wed, 06 Mar 2019 10:46:36 -0000 On Tue, Mar 05, 2019 at 06:58:04PM +0100, Thomas Monjalon wrote: > Hi, > > 05/03/2019 18:38, Gaëtan Rivet: > > > fs_dev_remove(struct sub_device *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. > > I don't follow you. What do you mean with this comment? > PORT_ID(sdev) uses ETH(sdev). ETH(sdev) will now point to `&rte_eth_devices[(sdev)->data->port_id]`, so data->port_id will be zeroed on sdev close. So once the sub-device has been closed, calling rte_eth_dev_release_port(ETH(sdev)) is not possible anymore, justifying the change (taking the reference to ETH(sdev) first, then using it after shared data has been overwritten). > > 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? 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? > > 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. 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(). -- Gaëtan Rivet 6WIND