From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1F05DA00BE; Tue, 28 Apr 2020 15:53:02 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7A10E1D5DF; Tue, 28 Apr 2020 15:53:01 +0200 (CEST) Received: from mx0b-0016f401.pphosted.com (mx0a-0016f401.pphosted.com [67.231.148.174]) by dpdk.org (Postfix) with ESMTP id E67A71D5D0 for ; Tue, 28 Apr 2020 15:52:59 +0200 (CEST) Received: from pps.filterd (m0045849.ppops.net [127.0.0.1]) by mx0a-0016f401.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 03SDp4o8029140; Tue, 28 Apr 2020 06:52:58 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.com; h=from : to : cc : subject : date : message-id : references : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pfpt0818; bh=vafZoIJpbWn5HjxnC+RvnH6ewn/JnHwq19KS1Pa+LLg=; b=r6bCf94N8LRctFgLq8jApofQpv2T7yl9QCTBMZQGIPxXXXnnBCHSjnDQw29MfD92tkmN daYXPgIvMvI/p9YgWMB+TZtJgMCn5AKbs0+WbNZby1Ut+oJjD0jsQSZUisb8Fb2STgaI 6sYBAJtlOUT7Fy1liq6PvehhKdnS3CFakOC6Emje1eJFzPZGwF/pi46VbMaDGG1trOxh 5Z/9MOnD/dGULXFSixg7c9AavcdPEe7EeVD35Z6sR7KY6mYyTdmZuDZWuKBFcWigpAXV MBE2bJ3OoBI8lY01iTgZ0RdcYWvHIRKmz6fCkTrjk+tof9zHmsCKQK8NlpeqXMc6vFvn Lw== Received: from sc-exch03.marvell.com ([199.233.58.183]) by mx0a-0016f401.pphosted.com with ESMTP id 30mjjqcfa0-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-SHA384 bits=256 verify=NOT); Tue, 28 Apr 2020 06:52:58 -0700 Received: from DC5-EXCH02.marvell.com (10.69.176.39) by SC-EXCH03.marvell.com (10.93.176.83) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Tue, 28 Apr 2020 06:52:56 -0700 Received: from SC-EXCH04.marvell.com (10.93.176.84) by DC5-EXCH02.marvell.com (10.69.176.39) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Tue, 28 Apr 2020 06:52:55 -0700 Received: from NAM11-CO1-obe.outbound.protection.outlook.com (104.47.56.174) by SC-EXCH04.marvell.com (10.93.176.84) with Microsoft SMTP Server (TLS) id 15.0.1497.2 via Frontend Transport; Tue, 28 Apr 2020 06:52:55 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=VHgX4yg32UsW6pkUJUHhmszoOhJ1MPmFr/CVmxw/GSktJNGQnxaNZ64LNtKnCFfRX/gHq/R+r4YtlsKA3t8p85MXNGkc9nDDln4lZTsY55bSXqe/Ibv0OylODFzYgLemCRHR5IptQ3oQygq6KaWWTm/0TMJRqmwttGjfQmavjDMfMbWyh/U3E+oTVLY1ok3c+R0HkKNBrhBxij0PpNdpEYP4qJFiXNGwMtYeCRArbUavf7drKtPR7R9qI6yiQMrS1Glma8J1puNoHX9p4sownICRf+NXnved4NH5q9o5GNQumLq62bYvweKcQPkB8XBkIGlzBXRIslNY8afbOs3mJA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=vafZoIJpbWn5HjxnC+RvnH6ewn/JnHwq19KS1Pa+LLg=; b=YXIobuYUsdNDu+I1qetHnYOmtaHIjv9f+DnqwphfIM8QXSCZPeJduG3lhX0Br3pHidNwV1jj/WWE8ouJcd/SljyNZijXzAEnYH6bRUHAs44Hfps2QR3holnH9tQaN+tWIlhP9i7hBwLj9iJE11IQNeiXJJ0rM4t/fFrpIGQKGU43WF8PYg/2Cy9AQ2iXLqz3FhnbTIVfS63+47BTcS/B7EHNkrafnDwhidwOgTas8fuWVq9bMCfnEkqU76IeLb49izN+SZzXagT6jJulZHZimadTWETZEFIJTEXHOsTM3hk04hBKVrhPvozGwg0PjoRx85w5ZQut3pZ3rGzx0rh0+w== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=marvell.com; dmarc=pass action=none header.from=marvell.com; dkim=pass header.d=marvell.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.onmicrosoft.com; s=selector1-marvell-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=vafZoIJpbWn5HjxnC+RvnH6ewn/JnHwq19KS1Pa+LLg=; b=hkLZLwB5vRD/17jEVrhdwWkYSHpTf7ZxnWqPMG1z+7ahbJ1QDtCBWkUNBKYayDMnWr3dta0y+NW0RMJnGDT6Uq4lFP2vIUsGNxT0SaTC3KMYPOdG9DdOZtxZ3NJzx1WId30gBx91v1gVYAwVm1kVNK+1Qvzz9IMO9n0IMNg9ZOQ= Received: from BY5PR18MB3105.namprd18.prod.outlook.com (10.255.136.94) by BY5PR18MB3265.namprd18.prod.outlook.com (10.255.139.97) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2958.19; Tue, 28 Apr 2020 13:52:52 +0000 Received: from BY5PR18MB3105.namprd18.prod.outlook.com ([fe80::cc6:c7ae:dc40:7ddf]) by BY5PR18MB3105.namprd18.prod.outlook.com ([fe80::cc6:c7ae:dc40:7ddf%7]) with mapi id 15.20.2937.026; Tue, 28 Apr 2020 13:52:52 +0000 From: Sunil Kumar Kori To: =?iso-8859-1?Q?Ga=EBtan_Rivet?= CC: "stephen@networkplumber.org" , "david.marchand@redhat.com" , "Jerin Jacob Kollanukkaran" , "dev@dpdk.org" Thread-Topic: [EXT] Re: [dpdk-dev] [PATCH v3 1/1] bus/pci: optimise scanning with whitelist/blacklist Thread-Index: AQHWFuDHsE1AbyTvlU2Aees2cTiIn6iNWc8AgAE8N1A= Date: Tue, 28 Apr 2020 13:52:52 +0000 Message-ID: References: <20200407092856.554-1-skori@marvell.com> <20200420065554.20138-1-skori@marvell.com> <20200427184346.nrrcvcet3hndr6cn@u256.net> In-Reply-To: <20200427184346.nrrcvcet3hndr6cn@u256.net> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: u256.net; dkim=none (message not signed) header.d=none;u256.net; dmarc=none action=none header.from=marvell.com; x-originating-ip: [2401:4900:1691:6624:b91d:e77a:93d2:5503] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 3e58403f-da83-48e2-115d-08d7eb7b722d x-ms-traffictypediagnostic: BY5PR18MB3265: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:3276; x-forefront-prvs: 0387D64A71 x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BY5PR18MB3105.namprd18.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(4636009)(346002)(376002)(39860400002)(396003)(366004)(136003)(52536014)(66574012)(76116006)(5660300002)(66946007)(66556008)(54906003)(64756008)(316002)(66476007)(66446008)(2906002)(7696005)(33656002)(8676002)(186003)(8936002)(6506007)(9686003)(4326008)(81156014)(71200400001)(86362001)(6916009)(55016002)(478600001); DIR:OUT; SFP:1101; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: i2kkhpuQLoC35RK/XqrkbFgTfWAnIgxPqxQQQXTmpS7aCFr0ivlMAoP682ibDcf+wBw1vm9Sb/ia/nE0MOXs+BaITd/wpiDwFWp9WxMOReciSmrb6D8nZX05za9m2QW3z/Ka66LiVRFzxKvTVNSOM285BFWsJxJk3HzzLAs1OcXI6BgABwPQIQL+cJzWzqo7lLreCfF89yTdKoljy0F5BHH/RRPuGiZgsnNl9zbTmBa0r4QiFaCtPvyZ4WK9G2iLZZS2aXCvZ/9X3LpD63OIa+f84Vo32g+Krc2tL5oE6LQQnjBXvInsHWETxAaxuatHBN9ZkYSOSCvkJy4jwAZPmE3a9xXq7h8rC3Wdl3NJgEUeetp3TZ/0r9rgXTNnO34MloSsb0cGeyz2awfro+aNfBsxodBQk5WvVH/srnnmKb7cKvISQrLPuRwKGhMQcX5S x-ms-exchange-antispam-messagedata: dAyAJJUBn+B6xyBtXS/XVfCAXymdXjj1x7cpv1r9aNKBxCNZflvIo+5CO1NVG9f9X7p/7PNtf1+ok23ymN88GZO4Gv2iLnDMEIRFr82wXJllOYggvmgnVchhB25K1J6wKInfGBPf20KxnpTWZLfkZicdeoPBXYAUUBZI9/KQWAaKACAXQW9pmXvOcUhBbU5sUoppHGhSJkQXe3+d9k6vWYzPmBKYPl6lQxrEPHjAa/MpvnlnAPkyWgfu4wVge4/+JALQuhn7Ui+fk+MSMO9CJwWDaofzX9YBKZ2BAbdcnUMgBXl/dNT3mKkoSE9tadP7B9d8N8FRHBQc/T1q9lF+XmlA8qas9pcOvVvdvsiu3jFWM0DjDcXeqcp3L1y0ocABzqPf++zAnZhmh0I4SSIrpyk+rsv+rYI0k3swWmUInM2qVghlyrlTCOAywA95qy2EHmGXXGL7hC/RuCQ2DutresXamM5Bxc/EDLuWf43PEJZNdPq4iRsyFgwfYP9q9OMY6ZJuc3VUFZstJeIobet6MF9FoxbW3KLurw29GfO9Bk+jwS1PNJmgoVhJivKKdbpC+Iy8RFT5QjMv1BOgCDRLz5bZ/3h+NYu0cvYe9DxfAwMQnXpd6qxb0WjCo7mQhj/ZzNg9+9nOCSfra14Q2MYPQCkZ5ydtoQuIhEpnxHd8CSJwgpDjIqKPa4goC0E9EQnW503ehqF/V6sh+lrBHkczWAfOeqgb4sRPgnm7dO5RFXrRuWH9pZhdFEqwAPWTibHKAIVS0w4b/OBEs/rKHwXpQLB4yvUsVbBbVjHA12IyxYUQfd1BnaGIY+89n1/jAHFzdEgjasnno02ATkTKMDWT1ljae1KSqhVHkUdN0eRzut0H5128vXo9T4XHKq9/pobG Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 3e58403f-da83-48e2-115d-08d7eb7b722d X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Apr 2020 13:52:52.3376 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: c3LOKW8beu9alcrtdJW0h/i/y7O6/rDdHnqnY1mgmoTZgZh+wSHucQVIiLucYKgadV5WDsuRPxtwKRdKuMu5mA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR18MB3265 X-OriginatorOrg: marvell.com X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138, 18.0.676 definitions=2020-04-28_09:2020-04-28, 2020-04-28 signatures=0 Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v3 1/1] bus/pci: optimise scanning with whitelist/blacklist X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" >-----Original Message----- >From: Ga=EBtan Rivet >Sent: Tuesday, April 28, 2020 12:14 AM >To: Sunil Kumar Kori >Cc: stephen@networkplumber.org; david.marchand@redhat.com; Jerin Jacob >Kollanukkaran ; dev@dpdk.org >Subject: [EXT] Re: [dpdk-dev] [PATCH v3 1/1] bus/pci: optimise scanning wi= th >whitelist/blacklist > >External Email > >---------------------------------------------------------------------- >Hello Sunil, > >As it seems that this patch does not overly alarm people using the PCI >bus, I have a few comments on this version. Sending those a little >earlier will allow you to change as needed before proceeding with your >tests. > >On 20/04/20 12:25 +0530, Sunil Kumar Kori wrote: >> rte_bus_scan API scans all the available PCI devices irrespective of whi= te >> or black listing parameters then further devices are probed based on whi= te >> or black listing parameters. So unnecessary CPU cycles are wasted during >> rte_pci_scan. >> >> For Octeontx2 platform with core frequency 2.4 Ghz, rte_bus_scan >consumes >> around 26ms to scan around 90 PCI devices but all may not be used by the >> application. So for the application which uses 2 NICs, rte_bus_scan >> consumes few microseconds and rest time is saved with this patch. >> >> Patch restricts devices to be scanned as per below mentioned conditions: >> - All devices will be scanned if no parameters are passed. >> - Only white listed devices will be scanned if white list is available. >> - All devices, except black listed, will be scanned if black list is >> available. >> >> Signed-off-by: Sunil Kumar Kori >> --- >> v3: >> - remove __rte_experimental from private function. >> - remove entry from map file too. >> v2: >> - Added function to validate ignorance of device based on PCI address. >> - Marked device validation function as experimental. >> >> drivers/bus/pci/bsd/pci.c | 13 ++++++++++++- >> drivers/bus/pci/linux/pci.c | 3 +++ >> drivers/bus/pci/pci_common.c | 34 >++++++++++++++++++++++++++++++++++ >> drivers/bus/pci/private.h | 11 +++++++++++ >> 4 files changed, 60 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c >> index ebbfeb13a..c8d954751 100644 >> --- a/drivers/bus/pci/bsd/pci.c >> +++ b/drivers/bus/pci/bsd/pci.c >> @@ -338,6 +338,7 @@ rte_pci_scan(void) >> .match_buf_len =3D sizeof(matches), >> .matches =3D &matches[0], >> }; >> + struct rte_pci_addr pci_addr; >> >> /* for debug purposes, PCI can be disabled */ >> if (!rte_eal_has_pci()) >> @@ -357,9 +358,19 @@ rte_pci_scan(void) >> goto error; >> } >> >> - for (i =3D 0; i < conf_io.num_matches; i++) >> + for (i =3D 0; i < conf_io.num_matches; i++) { >> + pci_addr.domain =3D matches[i].pc_sel.pc_domain; >> + pci_addr.bus =3D matches[i].pc_sel.pc_bus; >> + pci_addr.devid =3D matches[i].pc_sel.pc_dev; >> + pci_addr.function =3D matches[i].pc_sel.pc_func; >> + >> + /* Check that device should be ignored or not */ > >This comment is unnecessary, the function name should be sufficient to >describe the check done. > Ack >> + if (pci_addr_ignore_device(&pci_addr)) >> + continue; >> + > >As this function is almost a copy of pci_ignore_device(), with a twist >on the addr, I think the name pci_ignore_device_addr() would be more >helpful. > >I think in general however, that exposed symbols, even internals, >should be prefixed with rte_. It was (almost) ok for >pci_ignore_device() to forego the namespace as it is a static function >that will be mangled on export, but that won't be the case for your >function. > >Please add rte_ prefix. > Ack >> if (pci_scan_one(fd, &matches[i]) < 0) >> goto error; >> + } >> >> dev_count +=3D conf_io.num_matches; >> } while(conf_io.status =3D=3D PCI_GETCONF_MORE_DEVS); >> diff --git a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c >> index 71b0a3053..92bdad826 100644 >> --- a/drivers/bus/pci/linux/pci.c >> +++ b/drivers/bus/pci/linux/pci.c >> @@ -487,6 +487,9 @@ rte_pci_scan(void) >> if (parse_pci_addr_format(e->d_name, sizeof(e->d_name), >&addr) !=3D 0) >> continue; >> >> + if (pci_addr_ignore_device(&addr)) >> + continue; >> + >> snprintf(dirname, sizeof(dirname), "%s/%s", >> rte_pci_get_sysfs_path(), e->d_name); >> >> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c >> index 3f5542076..a350a1993 100644 >> --- a/drivers/bus/pci/pci_common.c >> +++ b/drivers/bus/pci/pci_common.c >> @@ -589,6 +589,40 @@ pci_dma_unmap(struct rte_device *dev, void >*addr, uint64_t iova, size_t len) >> return -1; >> } >> >> +static struct rte_devargs * >> +pci_addr_devargs_lookup(const struct rte_pci_addr *pci_addr) >> +{ >> + struct rte_devargs *devargs; >> + struct rte_pci_addr addr; >> + >> + RTE_EAL_DEVARGS_FOREACH("pci", devargs) { >> + devargs->bus->parse(devargs->name, &addr); > >Why not use rte_pci_addr_parse directly there? The bus->parse() API, >while stable, is one-level of indirection removed from what's done, >it's simpler for the reader to see the intent by using the proper function= . > >Return value should be checked. If the devargs name is not parseable, >there are other issues at hand (memory corruption), we should skip the >device as well or crash, but not proceed with comparison. > Ack >> + if (!rte_pci_addr_cmp(pci_addr, &addr)) >> + return devargs; >> + } >> + return NULL; >> +} >> + >> +bool >> +pci_addr_ignore_device(const struct rte_pci_addr *pci_addr) >> +{ >> + struct rte_devargs *devargs =3D pci_addr_devargs_lookup(pci_addr); >> + >> + switch (rte_pci_bus.bus.conf.scan_mode) { >> + case RTE_BUS_SCAN_WHITELIST: >> + if (devargs && devargs->policy =3D=3D RTE_DEV_WHITELISTED) >> + return false; >> + break; >> + case RTE_BUS_SCAN_UNDEFINED: >> + case RTE_BUS_SCAN_BLACKLIST: >> + if (devargs =3D=3D NULL || >> + devargs->policy !=3D RTE_DEV_BLACKLISTED) >> + return false; >> + break; >> + } >> + return true; >> +} >> + >> static bool >> pci_ignore_device(const struct rte_pci_device *dev) >> { > >The logic seems ok to me. > >However, the logic is the same as the one in rte_pci_probe(). During >probe, any device on the bus would have already been vetted during scan. >It should be ok to probe all existing rte_pci_device. > >The same argument can be made for rte_pci_get_iommu_class() then, no >need to use pci_ignore_device(). It is done after the scan() so it >should be ok. > >And if pci_ignore_device() can be completely removed, then you should >rename your function from rte_pci_ignore_device_addr() to >rte_pci_ignore_device() altogether. > Ack >> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h >> index a205d4d9f..75af786f7 100644 >> --- a/drivers/bus/pci/private.h >> +++ b/drivers/bus/pci/private.h >> @@ -42,6 +42,17 @@ int rte_pci_scan(void); >> void >> pci_name_set(struct rte_pci_device *dev); >> >> +/** >> + * Validate whether a device with given pci address should be ignored o= r >not. >> + * >> + * @param pci_addr >> + * PCI address of device to be validated >> + * @return >> + * 1: if device is to be ignored, >> + * 0: if device is to be scanned, >> + */ >> +bool pci_addr_ignore_device(const struct rte_pci_addr *pci_addr); >> + >> /** >> * Add a PCI device to the PCI Bus (append to PCI Device list). This fu= nction >> * also updates the bus references of the PCI Device (and the generic d= evice >> -- >> 2.17.1 > >Best regards, >-- >Ga=EBtan