From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f177.google.com (mail-pd0-f177.google.com [209.85.192.177]) by dpdk.org (Postfix) with ESMTP id 720C8B674 for ; Tue, 17 Feb 2015 07:15:00 +0100 (CET) Received: by pdjz10 with SMTP id z10so41449428pdj.12 for ; Mon, 16 Feb 2015 22:14:59 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=/J7L0HxHWAsy6EfW+5LpN1DxGkVwKeCRMydb2lYki20=; b=DgzYzfnKOJnEilutmaluSWgQG9ijvfze2tUVp6KW//aWOl0ejtRSfGS/vgYKOVw+jZ G9clDXUBTPPWe2hRy+8df+OBOTwfEfKo/M0oV+UNKbTA3AMLQaR6CmyPucW+BFJqkPan tp853AoMJ+NFxoJj2XMlma9VIDQKNBeo9zjtkzcBwUs9saJpNcHBXokMXtknEzl1owYv OzadtAXL2i17vIPpahQYdS8pKNR5N+e+xEo8bQSvIiF1DPMxPXUNWmeukXkYJ9Dug0sW 2/r1R/iWLjqPAWePEmgR91xwwoBMozTWVrLPluGaho2ZrB49PHhpI/fp3/oqlY6eFGwH 7EaA== X-Gm-Message-State: ALoCoQnkt7Wf3wnx30b46pk4Pd3zdykl7+Ku95IZRx1U9NPY6hxVfQBRy4LN5Cubpd1CPaQYa9Po X-Received: by 10.66.66.238 with SMTP id i14mr46495898pat.27.1424153699843; Mon, 16 Feb 2015 22:14:59 -0800 (PST) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by mx.google.com with ESMTPSA id v2sm16493295pdo.69.2015.02.16.22.14.58 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Feb 2015 22:14:59 -0800 (PST) Message-ID: <54E2DC61.9010804@igel.co.jp> Date: Tue, 17 Feb 2015 15:14:57 +0900 From: Tetsuya Mukawa User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Thomas Monjalon References: <1423470639-15744-2-git-send-email-mukawa@igel.co.jp> <1424060073-23484-1-git-send-email-mukawa@igel.co.jp> <1424060073-23484-5-git-send-email-mukawa@igel.co.jp> <4367974.mVXCMbsChG@xps13> In-Reply-To: <4367974.mVXCMbsChG@xps13> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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: Tue, 17 Feb 2015 06:15:00 -0000 On 2015/02/17 9:44, Thomas Monjalon wrote: > 2015-02-16 13:14, Tetsuya Mukawa: >> This patch replaces pci_addr_comparison() and memcmp() of pci addresse= s by >> eal_compare_pci_addr(). >> >> v8: >> - Fix pci_scan_one() to update sysfs values. >> (Thanks to Qiu, Michael and Iremonger, Bernard) >> v5: >> - Fix pci_scan_one to handle pt_driver correctly. >> v4: >> - Fix calculation method of eal_compare_pci_addr(). >> - Add parameter checking. >> >> Signed-off-by: Tetsuya Mukawa >> --- >> lib/librte_eal/bsdapp/eal/eal_pci.c | 29 ++++++++++++----------= ---- >> lib/librte_eal/common/eal_common_pci.c | 2 +- >> lib/librte_eal/common/include/rte_pci.h | 34 ++++++++++++++++++++++= +++++++++ >> lib/librte_eal/linuxapp/eal/eal_pci.c | 30 +++++++++++++---------= ----- >> lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 2 +- >> 5 files changed, 63 insertions(+), 34 deletions(-) >> >> diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c b/lib/librte_eal/bsda= pp/eal/eal_pci.c >> index 74ecce7..7dbdccd 100644 >> --- a/lib/librte_eal/bsdapp/eal/eal_pci.c >> +++ b/lib/librte_eal/bsdapp/eal/eal_pci.c >> @@ -270,20 +270,6 @@ pci_uio_map_resource(struct rte_pci_device *dev) >> return (0); >> } >> =20 >> -/* Compare two PCI device addresses. */ >> -static int >> -pci_addr_comparison(struct rte_pci_addr *addr, struct rte_pci_addr *a= ddr2) >> -{ >> - uint64_t dev_addr =3D (addr->domain << 24) + (addr->bus << 16) + (ad= dr->devid << 8) + addr->function; >> - uint64_t dev_addr2 =3D (addr2->domain << 24) + (addr2->bus << 16) + = (addr2->devid << 8) + addr2->function; >> - >> - if (dev_addr > dev_addr2) >> - return 1; >> - else >> - return 0; >> -} >> - >> - >> /* Scan one pci sysfs entry, and fill the devices list from it. */ >> static int >> pci_scan_one(int dev_pci_fd, struct pci_conf *conf) >> @@ -356,13 +342,24 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *co= nf) >> } >> else { >> struct rte_pci_device *dev2 =3D NULL; >> + int ret; >> =20 >> 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. > [...] >> --- a/lib/librte_eal/common/include/rte_pci.h >> +++ b/lib/librte_eal/common/include/rte_pci.h >> @@ -269,6 +269,40 @@ eal_parse_pci_DomBDF(const char *input, struct rt= e_pci_addr *dev_addr) >> } >> #undef GET_PCIADDR_FIELD >> =20 >> +/* Compare two PCI device addresses. */ >> +/** >> + * Utility function to compare two PCI device addresses. >> + * >> + * @param addr >> + * The PCI Bus-Device-Function address to compare >> + * @param addr2 >> + * The PCI Bus-Device-Function address to compare >> + * @return >> + * 0 on equal PCI address. >> + * Positive on addr is greater than addr2. >> + * Negative on addr is less than addr2, or error. >> + */ >> +static inline int >> +eal_compare_pci_addr(struct rte_pci_addr *addr, struct rte_pci_addr *= addr2) > I think that this function should be prefixed with rte_ Sure, I will. > [...] >> /* skip this element if it doesn't match our PCI address */ >> - 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. >> continue; >> =20 >> for (i =3D 0; i !=3D uio_res->nb_maps; i++) { >>