From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id B2539A04DC; Tue, 20 Oct 2020 00:36:22 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2DB6EBC86; Tue, 20 Oct 2020 00:36:20 +0200 (CEST) Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 9087ABBA0 for ; Tue, 20 Oct 2020 00:36:17 +0200 (CEST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.nyi.internal (Postfix) with ESMTP id 3CBBE5C00FD; Mon, 19 Oct 2020 18:36:16 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Mon, 19 Oct 2020 18:36:16 -0400 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=fm2; bh= F260dPlAYT9ZwPI7Sgcd5SekXsdYQBokmFDyN7Y3A48=; b=BBpZwJi4fBzBLb7s m0fw4mnAxoLSG+JTqWwfjBirz62lTVqzYtzG6XHg+FWXNVFkRN0TKKpCh20JQZfj FNwdg3HxozCPpMxHThbCFWWR1SlD5tGCnKPUVp8rM8/qZy8Z38Qg0wgaVSYrHeZX yow/4zune7LrmRF4V8QdFqDx2vYLLDRgEalwDU6rNEAaQDrJ7Qm1HRvDlbN8Boi4 uVG4+Bk6YPfdPaiX/OGFWXzII/gw1kMfF7H80mlcw9df0TyURCQ3eIY+GgY7DLUL DjGgwcOAB1vs6tWkMwC9I8nllVLbqEhuZgCGqc42NFuFqWvj63/V6WHpXqjgI9Jp VmRgvQ== 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=fm1; bh=F260dPlAYT9ZwPI7Sgcd5SekXsdYQBokmFDyN7Y3A 48=; b=nv7w20dsEx1YERQ40xDTIbirWu1aTBSd0VzBLWvlLOT37KLv/EaqoBg6G qKc8TEFPT5Ac7IQOuHUMUvKVsitivTyBEQlcRbSxhV50epHsFXpKAvr4cWxvQS7C pUrvsjdgaU0xbtJS/cOdTRdJMjTkzVt/LAr19SZ88PXpiFTrEJq0S9pP/LKK1hLG 39ZZIWQkveiJpNwqtkjzv/XjZBtvzuPh45aTpFy5IadSssdNnO79bKFQkq7cqFW0 Z3+g8bUwtq36/H5kP39BAR/CR3yKZdrG9x+KcpMGtIZ0oyzoPRVNhgY+RDzkU7bL D4jRIsLJouaKrN0m5sbTX9n/tawcA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrjedvgdegtdcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucggtf frrghtthgvrhhnpedugefgvdefudfftdefgeelgffhueekgfffhfeujedtteeutdejueei iedvffegheenucfkphepjeejrddufeegrddvtdefrddukeegnecuvehluhhsthgvrhfuih iivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgrlhho nhdrnhgvth 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 42784306467D; Mon, 19 Oct 2020 18:36:15 -0400 (EDT) From: Thomas Monjalon To: Matan Azrad , grive@u256.net Cc: Stephen Hemminger , dev@dpdk.org, Raslan Darawsheh , Long Li Date: Tue, 20 Oct 2020 00:36:13 +0200 Message-ID: <7708623.dZStS9cY3P@thomas> In-Reply-To: <1768712.CnFUc32gz9@thomas> References: <20200819175333.19601-1-stephen@networkplumber.org> <1768712.CnFUc32gz9@thomas> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH] net/vdev_netvsc: handle removal of associated pci device 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Fixing Gaetan's address 20/10/2020 00:33, Thomas Monjalon: > Gaetan, Matan, > Please could you check? > > > 25/09/2020 22:30, Long Li: > > HI Matan, > > > > While troubleshooting a failure in DPDK on device removal when VF device briefly disappears and comes back, I notice the failsafe driver is trying repeatedly to start a sub device (after this sub device has been successfully configured, but later hot removed from the kernel). This is due to repeated alarms calling fs_dev_start(). I trace into this commit: > > > > ae80146 net/failsafe: fix removed device handling > > > > The implementation of fs_err() is interesting: > > > > +fs_err(struct sub_device *sdev, int err) > > +{ > > + /* A device removal shouldn't be reported as an error. */ > > + if (sdev->remove == 1 || err == -EIO) > > + return rte_errno = 0; > > + return err; > > +} > > > > If I change this function to: > > @@ -497,7 +497,7 @@ int failsafe_eth_new_event_callback(uint16_t port_id > > fs_err(struct sub_device *sdev, int err) > > { > > /* A device removal shouldn't be reported as an error. */ > > - if (sdev->remove == 1 || err == -EIO) > > + if (sdev->remove == 1 && err == -EIO) > > return rte_errno = 0; > > return err; > > } > > > > The hung is going away. I don't know the reason why we use a || in the if(). If a call to rte_eth_dev_start() returning EIO (as the case in fs_dev_start), the best choice would be bail out and fail this sub device. > > > > Can you please take a look? > > > > Thanks, > > Long > > > > ________________________________ > > From: Matan Azrad > > Sent: Tuesday, September 15, 2020 12:00 AM > > To: Long Li ; Stephen Hemminger > > Cc: matan@mellanox.com ; grive@u246.net ; dev@dpdk.org ; Raslan Darawsheh > > Subject: RE: [dpdk-dev] [PATCH] net/vdev_netvsc: handle removal of associated pci device > > > > Hi Li > > > > From: Long Li > > > >Subject: Re: [dpdk-dev] [PATCH] net/vdev_netvsc: handle removal of > > > >associated pci device > > > > > > > >Hi Stephen > > > > > > > >From: Stephen Hemminger: > > > >> On Sun, 6 Sep 2020 12:38:18 +0000 > > > >> Matan Azrad wrote: > > > >> > > > >> > Hi Stephen > > > >> > > > > >> > From: Stephen Hemminger: > > > >> > > The vdev_netvsc was not detecting when the associated PCI device > > > >> > > (SRIOV) was removed. Because of that it would keep feeding the > > > >> > > same > > > >> > > (removed) device to failsafe PMD which would then unsuccessfully > > > >> > > try and probe for it. > > > >> > > > > > >> > > Change to use a mark/sweep method to detect that PCI device was > > > >> > > removed, and also only tell failsafe about new PCI devices. > > > >> > > Vdev_netvsc does not have to keep stuffing the pipe with the same > > > >> > > already existing PCI device. > > > >> > > > > >> > As I know, the vdev_netvsc driver doesn't call to failsafe if the > > > >> > PCI device is > > > >> not detected by the readlink command(considered as removed)... > > > >> > Am I missing something? > > > >> > > > >> The original code is broken because ctx_yield is not cleared, it > > > >> keeps sending the same value. > > > > > > > >Looking on the code again, It looks like ctx->yield has no effect on > > > >the next pipe write, It is just used for log. > > > > > > > >After the PCI interface matching to the netvsc interface, the pipe > > > >write is triggered only if the readlink commands success to see the > > > >plugged-in PCI > > > >device: > > > >readlink /sys/class/net/[iface]/device/subsystem shows "pci" > > > >readlink /sys/class/net/[iface]/device shows the pci device ID. > > > > > > > >So, the assumption is when the above readlink failed on the interface > > > >the device is removed(plugged-out) and the fd write will not happen. > > > > > > > >The code will continue to retry probe again and again until success > > > >only for plugged-in pci device matched the netvsc device. > > > > > > Hi Matan, > > > > > > The original code keeps writing to pipe even it's the same PCI device. > > > > Yes, the vdev_netvsc writes any plugged-in device to the associated netvsc device fd. > > > > > The > > > new code writes to pipe for a new device, only once. See the following code: > > > > > > + /* Skip if this is same device already sent to failsafe */ > > > + if (strcmp(addr, ctx->yield) == 0) > > > + return 0; > > > > > > > I understand you want to optimize the pipe writing to be written only after plugged-in hot event. > > > > The current solution suffers from race: the PCI device may be plugged-out and plugged-in in short time shorter than the driver alarm delay, then the PCI device plugged-in detection will lost. > > > > My suggestion: > > Add validation to the plugged-in device probing state and that it is owned by failsafe(using ownership API) - don't write the pipe if so. > > > > Matan > > > > > > > > > This patch also saves lots of CPU since it no longer writes to pipe all the time. > > > You are correct about the code will continue to probe on a new PCI device. > > > But someone has to do it to handle hot-add. > > > > > > Thanks, > > > Long > > > > > > > > > > > > > >> It looks like device removal and add was never tested. > > > > > > > >This is basic test we have to test plug-in plug-out and it passed every > > > >day in the last years. > > > > > > > >Maybe something new and special in your setup? > > > > > > > >> If you test removal you will see that vdev_netvsc: > > > >> 1. Sends same PCI device repeatedly to failsafe (every alarm call) > > > >> This is harmless, but useless. > > > >> 2. When device is removed, keeps doing #1 > > > > >