From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753358AbcLIWmg (ORCPT ); Fri, 9 Dec 2016 17:42:36 -0500 Received: from mail-cys01nam02on0115.outbound.protection.outlook.com ([104.47.37.115]:45024 "EHLO NAM02-CY1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753085AbcLIWmd (ORCPT ); Fri, 9 Dec 2016 17:42:33 -0500 X-Greylist: delayed 2921 seconds by postgrey-1.27 at vger.kernel.org; Fri, 09 Dec 2016 17:42:33 EST From: Haiyang Zhang To: Stephen Hemminger , Greg KH CC: KY Srinivasan , "olaf@aepfle.de" , "linux-kernel@vger.kernel.org" , "bjorn.helgaas@gmail.com" , "apw@canonical.com" , "devel@linuxdriverproject.org" , "leann.ogasawara@canonical.com" , "jasowang@redhat.com" Subject: RE: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial numbers Thread-Topic: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial numbers Thread-Index: AQHSUR21+bJgO3U+U0emkkp2CWFPCaD+NO0AgACI2oCAAHx4AIAAtV0AgAAbvVA= Date: Fri, 9 Dec 2016 20:09:49 +0000 Message-ID: References: <1481185988-30383-1-git-send-email-kys@exchange.microsoft.com> <1481186023-30429-1-git-send-email-kys@exchange.microsoft.com> <1481186023-30429-3-git-send-email-kys@exchange.microsoft.com> <20161208155604.GA4601@kroah.com> <20161209073122.GB1513@kroah.com> <20161209102030.1f550c5a@xeon-e3> In-Reply-To: <20161209102030.1f550c5a@xeon-e3> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=haiyangz@microsoft.com; x-originating-ip: [72.74.33.140] x-ms-office365-filtering-correlation-id: 4055d76a-a8b0-4e90-54a8-08d4206f543b x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001);SRVR:MWHPR03MB2496; x-microsoft-exchange-diagnostics: 1;MWHPR03MB2496;7:7dLUVTbvEGTv8oWuqzTPuhNl8N51nV42ITJZtn3723OAbEHDdsX9QmlnuiPqiXr6vMjmzKga59Y7Tiy+jGKBkNI7Utckk7BMAGStl9wMR6AB4OPRY/yuXW0d4pvtUnaMNaS13/g16xJQnNOeCMcljQ8EHKpr7rt9XZkDgF8n8vFVksLY5Ey0WtFgelw+X1dTG+/zGrOnaj/7Uk3qWXLyKXVMaiO9YVKmcwl6czyvCI6TShqmtfz9u9gTzfx4VFeBuMabCQrXgpbVTC2yWMomOLZ+YrL+Koaz/u1MYXJViFERmvUJCbaLp6cVMjox4y+zVjRIxdDt/5assAsOdA9ELK13f/Mc5WRPvjjPPrqXLqaCUKQKm3SzZ7Cj5WBW9FN/mwhgyyOT7h7/89NXeE93fbNefSBU/NK6nC8wMs7b44HVQ9yK5iB9qZVv7ME7tmUADGxLbu+UIbvjW0oBs2d8ix0s6IQw8l4CTi6aS5xGeBA= x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(9452136761055)(140211028294663)(198206253151910); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(61425038)(6040375)(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046)(6055026)(61426038)(61427038)(6041248)(20161123562025)(20161123560025)(20161123564025)(20161123555025)(6072148)(6047074);SRVR:MWHPR03MB2496;BCL:0;PCL:0;RULEID:;SRVR:MWHPR03MB2496; x-forefront-prvs: 015114592F x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(7916002)(39850400002)(39410400002)(39860400002)(39450400003)(39840400002)(40224003)(60444003)(13464003)(377454003)(24454002)(189002)(199003)(7736002)(99286002)(7696004)(8990500004)(105586002)(74316002)(76176999)(66066001)(106116001)(106356001)(39060400001)(54356999)(50986999)(77096006)(81156014)(101416001)(68736007)(81166006)(6506006)(38730400001)(305945005)(8936002)(97736004)(5001770100001)(86612001)(33656002)(9686002)(8676002)(76576001)(575784001)(86362001)(6116002)(189998001)(2906002)(4326007)(3846002)(229853002)(92566002)(2950100002)(3660700001)(3280700002)(5005710100001)(10290500002)(5660300001)(102836003)(10090500001)(122556002)(6436002)(93886004)(2900100001);DIR:OUT;SFP:1102;SCL:1;SRVR:MWHPR03MB2496;H:BLUPR03MB1412.namprd03.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Dec 2016 20:09:49.1153 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR03MB2496 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uB9Mgm54027140 > -----Original Message----- > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Friday, December 9, 2016 1:21 PM > To: Greg KH > Cc: KY Srinivasan ; olaf@aepfle.de; Haiyang Zhang > ; linux-kernel@vger.kernel.org; > bjorn.helgaas@gmail.com; apw@canonical.com; devel@linuxdriverproject.org; > leann.ogasawara@canonical.com; jasowang@redhat.com > Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on > serial numbers > > On Fri, 9 Dec 2016 08:31:22 +0100 > Greg KH wrote: > > > On Fri, Dec 09, 2016 at 12:05:53AM +0000, KY Srinivasan wrote: > > > > > > > > > > -----Original Message----- > > > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > > > Sent: Thursday, December 8, 2016 7:56 AM > > > > To: KY Srinivasan > > > > Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org; > > > > olaf@aepfle.de; apw@canonical.com; vkuznets@redhat.com; > > > > jasowang@redhat.com; leann.ogasawara@canonical.com; > > > > bjorn.helgaas@gmail.com; Haiyang Zhang > > > > Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on > serial > > > > numbers > > > > > > > > On Thu, Dec 08, 2016 at 12:33:43AM -0800, > kys@exchange.microsoft.com > > > > wrote: > > > > > From: Haiyang Zhang > > > > > > > > > > We currently use MAC address to match VF and synthetic NICs. > Hyper-V > > > > > provides a serial number to both devices for this purpose. This > patch > > > > > implements the matching based on VF serial numbers. This is the > way > > > > > specified by the protocol and more reliable. > > > > > > > > > > Signed-off-by: Haiyang Zhang > > > > > Signed-off-by: K. Y. Srinivasan > > > > > --- > > > > > drivers/net/hyperv/netvsc_drv.c | 55 > > > > ++++++++++++++++++++++++++++++++++++--- > > > > > 1 files changed, 51 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/net/hyperv/netvsc_drv.c > > > > b/drivers/net/hyperv/netvsc_drv.c > > > > > index 9522763..c5778cf 100644 > > > > > --- a/drivers/net/hyperv/netvsc_drv.c > > > > > +++ b/drivers/net/hyperv/netvsc_drv.c > > > > > @@ -1165,9 +1165,10 @@ static void netvsc_free_netdev(struct > > > > net_device *netdev) > > > > > free_netdev(netdev); > > > > > } > > > > > > > > > > -static struct net_device *get_netvsc_bymac(const u8 *mac) > > > > > +static struct net_device *get_netvsc_byvfser(u32 vfser) > > > > > { > > > > > struct net_device *dev; > > > > > + struct net_device_context *ndev_ctx; > > > > > > > > > > ASSERT_RTNL(); > > > > > > > > > > @@ -1175,7 +1176,8 @@ static void netvsc_free_netdev(struct > net_device > > > > *netdev) > > > > > if (dev->netdev_ops != &device_ops) > > > > > continue; /* not a netvsc device */ > > > > > > > > > > - if (ether_addr_equal(mac, dev->perm_addr)) > > > > > + ndev_ctx = netdev_priv(dev); > > > > > + if (ndev_ctx->vf_serial == vfser) > > > > > return dev; > > > > > } > > > > > > > > > > @@ -1205,21 +1207,66 @@ static void netvsc_free_netdev(struct > > > > net_device *netdev) > > > > > return NULL; > > > > > } > > > > > > > > > > +static u32 netvsc_get_vfser(struct net_device *vf_netdev) > > > > > +{ > > > > > + struct device *dev; > > > > > + struct hv_device *hdev; > > > > > + struct hv_pcibus_device *hbus = NULL; > > > > > + struct list_head *iter; > > > > > + struct hv_pci_dev *hpdev; > > > > > + unsigned long flags; > > > > > + u32 vfser = 0; > > > > > + u32 count = 0; > > > > > + > > > > > + for (dev = &vf_netdev->dev; dev; dev = dev->parent) { > > > > > > > > You are going to walk the whole device tree backwards? That's > crazy. > > > > And foolish. And racy and broken (what happens if the tree > changes > > > > while you do this?) Where is the lock being grabbed while this > happens? > > > > What about reference counts? Do you see other drivers ever doing > this > > > > (if you do, point them out and I'll go yell at them too...) > > > > > > Greg, > > > > > > We are registering for netdev events. Coming into this function, the > caller > > > guarantees that the list of netdevs does not change - we assert this > on entry: > > > ASSERT_RTNL(). We are only walking up the device tree for the > netdevs whose > > > state change is being notified to us - the device tree being walked > here is limited to > > > netdevs under question. > > > > But a netdev is a child of some type of "real" device, and you are now > > walking the tree of all devices up to the "root" parent device, which > > means you will hit PCI bridges, USB controllers, and all sorts of fun > > things if you are a child of those types of devices. > > > > And can't you tell if the netdev for this event, really is "your" > > netdev? Or are you getting called this for "all" netdevs? Sorry, I > > don't know this api, any pointers to it would be appreciated. > > > > > We have a reference to the device and we know the device is not > going away. Is it not > > > safe to dereference the parent pointer - after all the child has > taken a reference on > > > the parent as part of device_add() call. > > > > It might be, and might not be. There's a reason you don't see this > > pattern anywhere in the kernel because of this... > > > > > > > + if (!dev_is_vmbus(dev)) > > > > > + continue; > > > > > > > > Ick. > > > > > > > > Why isn't your parent pointer a vmbus device all the time? How > could > > > > you get burried down in the device hierarchy when you are the > driver for > > > > a specific bus type in the first place? How could this function > ever be > > > > called for a device that is NOT of this type? > > > > > > We get notified when state changes on any of the netdev devices in > the system. > > > Not all netdevs in the system belong to vmbus. Consider for instance > the > > > emulated NIC that can be configured. This is an emulated PCI NIC. We > are only > > > interested in netdevs that correspond to the VF instance that we are > interested in. > > > > Can you "know" this is your netdev by some other way than having to > walk > > the device tree? Name? local device type? Something else? This > seems > > like an odd api in that everyone would have to do gyrations like this > in > > order to determine if the netdev is "theirs" or not... > > The scenario is SR-IOV on Hyper-V. In the case of VF device, the host > hands the > guest OS a PCI device for the virtual function device. The VF device is > placed > on a special synthetic PCI bus (ie not part of the other buses on the > system). > The VF device also has a synthetic network interface (netvsc) which > lives > on VMBUS. This code is about managing the interaction between the two. > > The association between VF and synthetic NIC is done in response to the > VF network device being registered. Initial version was based on MAC > address > which is the same. Later refinement used permanent MAC address to > avoid bugs if MAC address changed. This version is to use serial number > instead which is safer than MAC address. > > The code to walk up/down maybe not be needed to find serial number. > Perhaps a more direct single set of conditions is possible? > > Something like: > > In pci-hyperv.c > > u32 hv_pcifront_get_serial(struct pci_bus *bus, unsigned int devfn) > { > struct hv_pcibus_device *hbus > = container_of(bus->sysdata, > struct hv_pcibus_device, sysdata); > struct hf_pci_dev *hpdev; > u32 serial; > > hpdev = get_pcichild_wslot(hbus, devfn_to_wslot(pdev->devfn)); > if (!hpdev) > return 0; > > serial = hpdev->devs.ser; > put_pcichild(hpdev, hv_pcidev_ref_by_slot); > return serial; > } > > In netvsc_drv.c > > static u32 netvsc_get_vfser(struct net_device *vf_netdev) > { > struct device *dev = vf_netdev->dev.parent; > struct pci_dev *pdev; > u32 wslot; > > if (!dev || !dev_is_pci(dev)) > return 0; > > pdev = container_of(dev, struct pci_device, dev); > > return hv_pcifront_get_serial(pdev->bus, pdev->devfn); > } > > > > > > P.S: it would be good to be able to get win_slot out through sysfs as > well for systemd/udev. Stephen, Thanks for suggestion. Actually, in my earlier implementation of this feature (VF serial based matching), I thought about export a function from vPCI driver, then calling it from netvsc. So I don't need to move structs between headers... But, it creates a dependency of netvsc on vPCI driver's symbol. So, even if on a VM without SRIOV, we have to load vPCI driver, which we don't want. Also, hv_vpci device is 3 parent layers above the vf_netdevice: Here is the VF drv hierarchy -- Should we assume it's always 3 parents above vf_netdevice, or search for it? [ 368.185259] HZINFO:NETDEV_REGISTER: [ 368.185261] HZINFO: dev:ffff88007c10d518, bus: (null), busName:(null), drvName:(null) [ 368.185262] HZINFO: dev:ffff88007c10c0a0, bus:ffffffff81ce4b60, busName:pci, drvName:ixgbevf [ 368.185263] HZINFO: dev:ffff8800355c0000, bus: (null), busName:(null), drvName:(null) [ 368.185264] HZINFO: dev:ffff8800355c5428, bus:ffffffffa0008160, busName:vmbus, drvName:hv_pci [ 368.185264] HZINFO: dev:ffff88007c49e268, bus:ffffffff81ce9800, busName:acpi, drvName:vmbus [ 368.185265] HZINFO: dev:ffff88007c48ea68, bus:ffffffff81ce9800, busName:acpi, drvName:(null) [ 368.185266] HZINFO: dev:ffff88007c48aa68, bus:ffffffff81ce9800, busName:acpi, drvName:(null) [ 368.185266] HZINFO: dev:ffff88007c48a268, bus:ffffffff81ce9800, busName:acpi, drvName:(null) [ 368.185267] HZINFO: dev:ffff88007c489a68, bus:ffffffff81ce9800, busName:acpi, drvName:(null) Thanks, - Haiyang