From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FAKE_REPLY_C,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7A25CC10F14 for ; Thu, 3 Oct 2019 22:10:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 3D9CC2133F for ; Thu, 3 Oct 2019 22:10:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1570140611; bh=F6E2RvV20IDKK+8vbX46hyZ6gzaFZiM6GfmUhN5E1EM=; h=Date:From:To:Cc:Subject:In-Reply-To:List-ID:From; b=lMBsVhnh1+1r0I+Y8w0QPaxIE4/qqRaHgGm1+3obvOypX1C09eKMAd8DCWUmVT/KS Vg/lTGDvm/txftSlmzZOMwP6QdUStlBgicyEzOTrqgTzru0gh7KcVKCbMajYrmesy7 K36diKZI7Ugf7cCN8b2KLFKgggelyEyzsJwL8leE= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729534AbfJCWKK (ORCPT ); Thu, 3 Oct 2019 18:10:10 -0400 Received: from mail.kernel.org ([198.145.29.99]:48900 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729490AbfJCWKK (ORCPT ); Thu, 3 Oct 2019 18:10:10 -0400 Received: from localhost (unknown [69.71.4.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id AE8BF20867; Thu, 3 Oct 2019 22:10:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1570140609; bh=F6E2RvV20IDKK+8vbX46hyZ6gzaFZiM6GfmUhN5E1EM=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=zu8WGpXZN1sGAbtPCJ1ddNvZhX+JVpBzcXYxUiAsYO4gdcdA2rE8hV1AEdNYGcBS6 BadYC0xhyZXUUnxHU20w+IT3uX5RLAdVL7JA5yo1jKaV4LLK+ed5l6bR0zt0BmxC0+ xC+crqB4jS7C2Dcq7obhj2TPkMCSEjG11sjcZyv4= Date: Thu, 3 Oct 2019 17:10:07 -0500 From: Bjorn Helgaas To: CREGUT Pierre IMT/OLN Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, Donald Dutile , Alexander Duyck , Jakub Kicinski Subject: Re: [PATCH] PCI/IOV: update num_VFs earlier Message-ID: <20191003221007.GA209602@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <49b0ad6d-7b6f-adbd-c4a3-5f9328a7ad9d@orange.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org [+cc Don, Alex, Jakub] On Thu, Oct 03, 2019 at 11:04:45AM +0200, CREGUT Pierre IMT/OLN wrote: > Le 02/10/2019 à 01:45, Bjorn Helgaas a écrit : > > On Fri, Apr 26, 2019 at 10:11:54AM +0200, CREGUT Pierre IMT/OLN wrote: > > > I also initially thought that kobject_uevent generated the netlink event > > > but this is not the case. This is generated by the specific driver in use. > > > For the Intel i40e driver, this is the call to i40e_do_reset_safe in > > > i40e_pci_sriov_configure that sends the event. > > > It is followed by i40e_pci_sriov_enable that calls i40e_alloc_vfs that > > > finally calls the generic pci_enable_sriov function. > > I don't know anything about netlink. The script from the bugzilla > > (https://bugzilla.kernel.org/show_bug.cgi?id=202991) looks like it > > runs > > > > ip monitor dev enp9s0f2 > > > > What are the actual netlink events you see? Are they related to a > > device being removed? > > We have netlink events both when num_vfs goes from 0 to N and from N to 0. > Indeed you have to go to 0 before going to M with M != N. Right. > On an Intel card, when one goes from 0 to N, the netlink event is > sent "early". The value of num_vfs is still 0 and you get the > impression that the number of VFS has not changed. As the meaning of > those events is overloaded, you have to wait an arbitrary amount of > time until it settles (there will be no other event). There is no > such problem when it goes from N to 0 because of implementation > details but it may be different for another brand. I hadn't looked far enough. I think the "remove" netlink events are probably from the i40e_do_reset_safe() path, which eventually calls free_netdev() and put_device(). The pci_enable_sriov() path calls the driver's ->probe method, and I suspect the "add" netlink events are emitted there. > > When we change num_VFs, I think we have to disable any existing VFs > > before enabling the new num_VFs, so if you trigger on a netlink > > "remove" event, I wouldn't be surprised that reading sriov_numvfs > > would give a zero until the new VFs are enabled. > Yes but we are speaking of the event sent when num_vfs is changed from 0 to > N > > [...] > > I thought this was a good idea, but > > > > - It does break the device_lock() encapsulation a little bit: > > sriov_numvfs_store() uses device_lock(), which happens to be > > implemented as "mutex_lock(&dev->mutex)", but we really shouldn't > > rely on that implementation, and > The use of device_lock was the cheapest solution. It is true that > lock and trylock are exposed by device.h but not is_locked. To > respect the abstraction, we would have to lock the device (at least > use trylock but it means locking when we can access the value, in > that case we may just make reading num_vfs blocking ?). > > The other solution is to record the state of freshness of num_vfs > but it means a new Boolean in the pci_sriov data-structure. > > > - The netlink events are being generated via the NIC driver, and I'm > > a little hesitant about changing the PCI core to deal with timing > > issues "over there". > > NIC drivers send netlink events when their state change, but it is > the core that changes the value of num_vfs. So I would think it is > the core responsibility to make sure the exposed value makes sense > and it would be better to ignore the details of the driver > implementation. Yes, I think you're right. And I like your previous suggestion of just locking the device in the reader. I'm not enough of a sysfs expert to know if there's a good reason to avoid a lock there. Does the following look reasonable to you? commit 0940fc95da45 Author: Pierre Crégut Date: Wed Sep 11 09:27:36 2019 +0200 PCI/IOV: Serialize sysfs sriov_numvfs reads vs writes When sriov_numvfs is being updated, drivers may notify about new devices before they are reflected in sriov->num_VFs, so concurrent sysfs reads previously returned stale values. Serialize the sysfs read vs the write so the read returns the correct num_VFs value. Link: https://bugzilla.kernel.org/show_bug.cgi?id=202991 Link: https://lore.kernel.org/r/20190911072736.32091-1-pierre.cregut@orange.com Signed-off-by: Pierre Crégut Signed-off-by: Bjorn Helgaas diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index b3f972e8cfed..e77562aabbae 100644 --- a/drivers/pci/iov.c +++ b/drivers/pci/iov.c @@ -254,8 +254,14 @@ static ssize_t sriov_numvfs_show(struct device *dev, char *buf) { struct pci_dev *pdev = to_pci_dev(dev); + u16 num_vfs; + + /* Serialize vs sriov_numvfs_store() so readers see valid num_VFs */ + device_lock(&pdev->dev); + num_vfs = pdev->sriov->num_VFs; + device_lock(&pdev->dev); - return sprintf(buf, "%u\n", pdev->sriov->num_VFs); + return sprintf(buf, "%u\n", num_vfs); } /*