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 9A676A04DC; Tue, 20 Oct 2020 00:33:19 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E0327BC86; Tue, 20 Oct 2020 00:33:17 +0200 (CEST) Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 62AC2BC84 for ; Tue, 20 Oct 2020 00:33:15 +0200 (CEST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.nyi.internal (Postfix) with ESMTP id 036C65C0078; Mon, 19 Oct 2020 18:33:14 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute2.internal (MEProxy); Mon, 19 Oct 2020 18:33:14 -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= 15216WQgrWOzfZZQjkf05Q/GZyKYWpz4jTUH4h3H26c=; b=AjyKS08fBIf9XY3D c29QMhNilMrY45ibUTJQbrq0G+L0Rb/TZH+3b/hH4otHBxoZ9lo91n3TVPM3hXNE IcTdPcUdmn+JuMGaxOGb7Je3hc+ZKqWOPx4uy5atFhkGNkevtL1R4tBNEZYPVrey u/lLYWVOaFC3M3S17UQUTYgrr4IGPx2oRusDCBlModpfsWJ+9zsGXd0EPUIGw9JQ b82U4Vu7gKdAcie8Aa15NXiimUQxdORJgnYtheSHcYacdxeZJxIQaYYx/+LFtazy LNtHQI/IIUbTIh373wijhnby1U3eEFi0f6gjg0caPJSimDj5H+fEJBTtLgzuPi7O 6F0CXQ== 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=15216WQgrWOzfZZQjkf05Q/GZyKYWpz4jTUH4h3H2 6c=; b=Sm91yRouaEBRN1Bz4vSygovLE7+JBK2R6AGgeBRHmuPqvXBkxn/qPVLCn ypH8NkEBzkAw0EWZBpntpJVU9zlZ3odGr3MFnKvgAwOrIdf0g5qnaggPAdBPW7ei mM+AEdHOt0z8QWko+hsqDKGpxvAV+8/Y6WsbQxkeaYS0BnPj0elN8vSZMBdqr0Dh CTg9Rh4esoSPmPp6OBZi15gwlX7Ikra0rN41jlMvTuYChePnO/59y05FT7Kd5LVY YMScrp2IH6PYLNAhDQCZCxeSa9LCFiC+D0W7H+K1+B11NaksqIjrtTvEdnLPaPX5 OIh0xlzL+yiZjicIlRIQpHhV9wHmw== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrjedvgdeflecutefuodetggdotefrodftvf 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 5C885328005E; Mon, 19 Oct 2020 18:33:12 -0400 (EDT) From: Thomas Monjalon To: Matan Azrad , "grive@u246.net" Cc: Stephen Hemminger , dev@dpdk.org, Raslan Darawsheh , Long Li Date: Tue, 20 Oct 2020 00:33:10 +0200 Message-ID: <1768712.CnFUc32gz9@thomas> In-Reply-To: References: <20200819175333.19601-1-stephen@networkplumber.org> 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" 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 >