From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id D4630ADC5 for ; Wed, 18 Feb 2015 11:26:44 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP; 18 Feb 2015 02:26:43 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,600,1418112000"; d="scan'208";a="529160370" Received: from irsmsx110.ger.corp.intel.com ([163.33.3.25]) by orsmga003.jf.intel.com with ESMTP; 18 Feb 2015 02:18:05 -0800 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.218]) by irsmsx110.ger.corp.intel.com ([169.254.15.236]) with mapi id 14.03.0195.001; Wed, 18 Feb 2015 10:26:41 +0000 From: "Iremonger, Bernard" To: Tetsuya Mukawa , Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH v8 04/14] eal/pci: Consolidate pci address comparison APIs Thread-Index: AQHQSx4FtrZxQTBnpUyeFX5T1gznCpz2MHHQ Date: Wed, 18 Feb 2015 10:26:41 +0000 Message-ID: <8CEF83825BEC744B83065625E567D7C2049EAA9F@IRSMSX108.ger.corp.intel.com> References: <1423470639-15744-2-git-send-email-mukawa@igel.co.jp> <4367974.mVXCMbsChG@xps13> <54E2DC61.9010804@igel.co.jp> <1684102.trQSmodf0H@xps13> <54E3F10A.4090303@igel.co.jp> In-Reply-To: <54E3F10A.4090303@igel.co.jp> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v8 04/14] eal/pci: Consolidate pci address comparison APIs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Feb 2015 10:26:46 -0000 > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tetsuya Mukawa > Sent: Wednesday, February 18, 2015 1:55 AM > To: Thomas Monjalon > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v8 04/14] eal/pci: Consolidate pci address= comparison APIs >=20 > On 2015/02/18 10:02, Thomas Monjalon wrote: > > 2015-02-17 15:14, Tetsuya Mukawa: > >> On 2015/02/17 9:44, Thomas Monjalon wrote: > >>> 2015-02-16 13:14, Tetsuya Mukawa: > >>>> @@ -356,13 +342,24 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *= conf) > >>>> } > >>>> else { > >>>> struct rte_pci_device *dev2 =3D NULL; > >>>> + int ret; > >>>> > >>>> TAILQ_FOREACH(dev2, &pci_device_list, next) { > >>>> - if (pci_addr_comparison(&dev->addr, &dev2->addr)) > >>>> + ret =3D eal_compare_pci_addr(&dev->addr, &dev2->addr); > >>>> + if (ret > 0) > >>>> continue; > >>>> - else { > >>>> + else if (ret < 0) { > >>>> TAILQ_INSERT_BEFORE(dev2, dev, next); > >>>> return 0; > >>>> + } else { /* already registered */ > >>>> + /* update pt_driver */ > >>>> + dev2->pt_driver =3D dev->pt_driver; > >>>> + dev2->max_vfs =3D dev->max_vfs; > >>>> + memmove(dev2->mem_resource, > >>>> + dev->mem_resource, > >>>> + sizeof(dev->mem_resource)); > >>>> + free(dev); > >>>> + return 0; > >>> Could you comment this "else part" please? > >> PCI device list is allocated when rte_eal_init() is called. At the > >> time, to fill pci device information, sysfs value is used. > >> If sysfs values written by kernel device driver will not be changed > >> by igb_uio, vfio or pci_uio_genereic, above code isn't needed. > >> But actually above values are changed or added by them. > >> > >> Here is a step to cause issue. > >> 1. Boot linux. > >> 2. Start DPDK application without any physical NIC ports. > >> - Here, some sysfs values are read, and store to pci device list. > >> 3. igb_uio starts managing a port. > >> - Here, some sysfs values are changed. > >> 4. Add a NIC port to DPDK application using hotplug functions. > >> - Here, we need to replace some values. > > I think that you are showing that something is wrongly designed in > > these EAL structures. I suggest to try cleaning this mess instead of wo= rkarounding. Hi Tetsuya, Thomas, I think that redesigning the EAL structures is beyond the scope of this pat= chset and should be undertaken as a separate task. I suspect there may be a problem in the original code when a device which = was using a kernel driver is bound to igb_uio. The igb_uio driver adds /s= ys/bus/pci/devices/0000\:05\:00.0/max_vfs. Regards, Bernard. > > > > [...] > >>>> - if (memcmp(&uio_res->pci_addr, &dev->addr, sizeof(dev->addr))) > >>>> + if (eal_compare_pci_addr(&uio_res->pci_addr, &dev->addr)) > >>> Why memcmp is not sufficient to compare PCI addresses? > >>> The only exception I see is endianness for natural sorting. > >> Here is the definition. > >> > >> struct rte_pci_addr { > >> uint16_t domain; /**< Device domain */ > >> uint8_t bus; /**< Device bus */ > >> uint8_t devid; /**< Device ID */ > >> uint8_t function; /**< Device function. */ > >> }; > >> > >> But, sizeof(struct rte_pci_addr) will be 6. > >> If rte_pci_addr is allocated in a function without bzero, last 1 byte > >> may have some value. > >> As a result, memcmp may not work. To avoid such a case, I checked > >> like above. > > OK thanks. That's the kind of information which is valuable in a commit= log. > > >=20 > Sure I will add it. >=20 > Thanks, > Tetsuya