From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id DA45CB43C for ; Wed, 18 Feb 2015 12:39:52 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP; 18 Feb 2015 03:39:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,512,1418112000"; d="scan'208";a="653756635" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by orsmga001.jf.intel.com with ESMTP; 18 Feb 2015 03:39:50 -0800 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.218]) by IRSMSX106.ger.corp.intel.com ([169.254.8.235]) with mapi id 14.03.0195.001; Wed, 18 Feb 2015 11:39:48 +0000 From: "Iremonger, Bernard" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH v8 04/14] eal/pci: Consolidate pci address comparison APIs Thread-Index: AQHQSx4FtrZxQTBnpUyeFX5T1gznCpz2MHHQgAAFwQCAABAxwA== Date: Wed, 18 Feb 2015 11:39:47 +0000 Message-ID: <8CEF83825BEC744B83065625E567D7C2049EAB13@IRSMSX108.ger.corp.intel.com> References: <1423470639-15744-2-git-send-email-mukawa@igel.co.jp> <54E3F10A.4090303@igel.co.jp> <8CEF83825BEC744B83065625E567D7C2049EAA9F@IRSMSX108.ger.corp.intel.com> <2590241.Y3C96lux3o@xps13> In-Reply-To: <2590241.Y3C96lux3o@xps13> 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 11:39:53 -0000 > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Wednesday, February 18, 2015 10:33 AM > To: Iremonger, Bernard > Cc: Tetsuya Mukawa; dev@dpdk.org; ivan.boule@6wind.com > Subject: Re: [dpdk-dev] [PATCH v8 04/14] eal/pci: Consolidate pci address= comparison APIs >=20 > Hi Bernard, >=20 > 2015-02-18 10:26, Iremonger, Bernard: > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tetsuya Mukawa > > > 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_co= nf *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 nee= ded. > > > >> 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 o= f workarounding. > > > > Hi Tetsuya, Thomas, > > I think that redesigning the EAL structures is beyond the scope of this= patchset and should be > undertaken as a separate task. >=20 > I strongly disagrees this opinion. We should never workaround design prob= lems and add more > complex/weird code. > I think that this kind of consideration is the heart of some design probl= ems we have to face today. > Please let's stop adding some code which just works without thinking the = whole design. >=20 > > I suspect there may be a problem in the original code when a device wh= ich was using a kernel driver > is bound to igb_uio. The igb_uio driver adds /sys/bus/pci/devices/0000\= :05\:00.0/max_vfs. Hi Tomas, Tetsuya, In general, I agree that we should not workaround design problems. In this case I don't think there is a problem with the rte_pci_device and p= ci_device_list structures. The "already registered" device has been replaced. It would probably be cle= aner to remove the "already registered" device from the list and then add t= he new device to the list rather than update the "already registered" devic= e. Regards, Bernard.