From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f176.google.com (mail-wr0-f176.google.com [209.85.128.176]) by dpdk.org (Postfix) with ESMTP id 7B4987D0B for ; Fri, 18 Aug 2017 11:53:01 +0200 (CEST) Received: by mail-wr0-f176.google.com with SMTP id 5so41887330wrz.5 for ; Fri, 18 Aug 2017 02:53:01 -0700 (PDT) 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=bFuCb2zaa/FJuvHL+W4OVd6V9hrAux/BuHzqeruYwEI=; b=Dt+isu5gne2iYXktFJtu5nbN6tbzVzOuxoFM9Qp9+/JZUJlqT/dBgRTqggmP/1eUQD ywE5ZzSMu3NHOyi6LsZ6HGUsQgXfYtFC16ANodwCer+52gFV8SeP7SU3Rh621kZnv3/i ojm6j0Uuz4mi+DALyzdER3UB/VcroBa6nQsW97p7I54hhmU+0eAwWwY6x1LVQwjfzg38 Jc4GVp01RxaccNddX+mVqqp5+g/gT4SRTGMxBw8GBMbuxQbvWNO/GYtB07UPLN8BhGxj 45xlAqIms5gwDK0lNarcIDH16aCkVZ/A65TcQK3xmkMr7ILWKW+ua8L/JkVu7S0m4ou6 yJIg== 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=bFuCb2zaa/FJuvHL+W4OVd6V9hrAux/BuHzqeruYwEI=; b=i6pfIVFnz+8qbOo+9QG4fAWl9sfNF5QRK2Uo9cfXajQnx+z9R9cvFAnEf1Q+GUtiNP bv/eUEbJ18oXeJUnxx/d+JgPR1HdX5wz3acvgwDIIKAZOuoHgfkdyUrxRK/cQsuMuiOG PRoBvQVBLW77LQChI5/Fp3nVUs3NXEkf85VSB6WP2nVpxfk1brvTCoIMSOxaziAqsMkW 9aWl/88oDw1rePNGPkJqzw3Oyt2i7eE3YJHHr5xjexH2UHDrjF+X2g8sUlbV7cJ+7I+x gAqr/gUCb29n6jJgNLgp3NgbnanFoc6qlB8tSOEC4Bv8/bPl7yvV0gIYu1AymREjaDQb 3EBA== X-Gm-Message-State: AHYfb5hwAYFEj22oD6tC0qzIdent6B13Sn/SASQErkUugQAbZuOeN6sG +h4YBkwLCSbHEFXR X-Received: by 10.28.105.5 with SMTP id e5mr1069839wmc.42.1503049980947; Fri, 18 Aug 2017 02:53:00 -0700 (PDT) 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 m127sm1336499wmm.46.2017.08.18.02.52.59 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 18 Aug 2017 02:52:59 -0700 (PDT) Date: Fri, 18 Aug 2017 11:52:50 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Shahaf Shuler Cc: Thomas Monjalon , "dev@dpdk.org" , "stable@dpdk.org" Message-ID: <20170818095250.GT8124@bidouze.vm.6wind.com> References: <20170816114308.165850-1-shahafs@mellanox.com> <20170816124130.GL8124@bidouze.vm.6wind.com> <20170816152607.GO8124@bidouze.vm.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-stable] [PATCH] ethdev: fix device state on close X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Aug 2017 09:53:01 -0000 On Thu, Aug 17, 2017 at 06:04:27AM +0000, Shahaf Shuler wrote: > Wednesday, August 16, 2017 6:26 PM, Gaëtan Rivet: > > > Even though it is reasonable for driver to call the > > rte_eth_dev_port_release, I still think the ethdev layer should protect from > > such bad behavior from the application side. > > > It is more robust than counting on the different PMD to implement such > > release. > > > > > > > The ethdev layer does so in the rte_eth_dev_detach function, which is > > currently the only way to detach a device from the ethdev level. > > > > `rte_eth_dev_detach` setting the device state seems to me to be a crutch > > against badly implemented drivers. If I was nitpicky I would actually remove it > > and ideally everyone should enforce the call of rte_eth_dev_release_port > > from device removal functions when reviewing PMD implementations. > > > > The hotplug API is available to applications and the only way to have > > consistent devices states after calling rte_eal_hotplug_remove is to have > > drivers using a hook in the ethdev layer allowing to clean-up resources upon > > release. While it is trivial in its current state, such entry-point in the ethdev > > layer will be useful and should be kept and enforced IMO. > > > > I'm thinking about the 16-bit portid scheduled for v17.11, which implies an > > arbitrary number of port available. This would imply a dynamic allocation of > > rte_eth_devices, which would *need* such release hook to be available. > > Well the API should not be designed from conjectures or speculations of > > course, but I think it should be useful and there is no reason to remove it. > > > > Going further, I find it dangerous to have two points where the port is > > semantically released (device state set to UNUSED). If the API of the port > > release changes, we now have two points where we need to enforce the > > changes. While trivial concerning an enum, it could become more complex / > > dangerous if this veered toward memory management. > > Those are valid concerns, and you convinced me the RTE_ETH_DEV_UNUSED cannot be set after device close. > > I still think the ethdev layer missing protection against driver calls (other than detach) following a device close. The API not allows, but the ethdev should enforce it. > Considering some driver memset to 0 their control structs after device close, with no protection such bad calls might lead to segfault, which is not what we want even under bad behavior. > > One could suggest to protect it inside the driver by replacing the vtable of the driver ops, however it will introduce a lot of duplicated code which can easily be avoided if ethdev would not pass such > Calls after device close. > > So, how about adding another state which will indicate the device is closed, cannot be used, yet still attached? > It's feasible. It might be useful. Adding finer device states seems logical to me, I made those explicit in the fail-safe. However, in the fail-safe I had the need to make automatic the handling of devices, meaning that these device states had to be properly abstracted and parseable. Adding finer device states in ethdev: Pros: - Easier to follow API and to spot usage mistakes. - Increasing the rigidity of the API might make apparent mistakes from PMD implementations. Helps PMD maintainers. Cons: - Some of those "mistakes" might be due to hardware-specific limitations that cannot be bypassed. I'm thinking about the flow-isolation implementation in MLX4 that had specific requirements about when it could be enabled, that would actually be forbidden by this proposal as it had to be done before rte_eth_dev_configure. - Handling special cases could quickly become a mess of edge-cases with workarounds here and there. Making the API evolve would imply untangling this mess at times. I'm considering there a full-blown device state implementation. I know you are only proposing adding an intermediate device state allowing to protect the user from using a closed device. But this proposal should be examined on the direction it would lead us into. It might be simpler for example to introduce a flag "dev_closed" in rte_eth_dev_data (along dev_started) for example, and checking only on this in the relevant functions. I don't have a strong opinion about the finer-grained device states. I only wanted to lay out what I saw as possible longer-term effects of choosing this solution and a possible alternative. -- Gaëtan Rivet 6WIND