From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?B?UmFmYcWCIEtvemlr?= Subject: Re: [PATCH v2 3/4] eal: enable WC during resources mapping Date: Fri, 29 Jun 2018 12:28:48 +0200 Message-ID: References: <1530191753-18689-1-git-send-email-rk@semihalf.com> <1530191753-18689-4-git-send-email-rk@semihalf.com> <2cb4219e-b354-0983-e56a-c9a14336b596@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org, Marcin Wojtas , =?UTF-8?Q?Micha=C5=82_Krawczyk?= , "Tzalik, Guy" , "Schmeilin, Evgeny" , "Matushevsky, Alexander" , "Chauskin, Igor" , Thomas Monjalon To: Ferruh Yigit Return-path: Received: from mail-lj1-f195.google.com (mail-lj1-f195.google.com [209.85.208.195]) by dpdk.org (Postfix) with ESMTP id D35171B4DE for ; Fri, 29 Jun 2018 12:28:49 +0200 (CEST) Received: by mail-lj1-f195.google.com with SMTP id h2-v6so3669764ljb.10 for ; Fri, 29 Jun 2018 03:28:49 -0700 (PDT) In-Reply-To: <2cb4219e-b354-0983-e56a-c9a14336b596@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 2018-06-29 11:05 GMT+02:00 Ferruh Yigit : > On 6/29/2018 9:58 AM, Rafa=C5=82 Kozik wrote: >> 2018-06-28 16:50 GMT+02:00 Ferruh Yigit : >>> On 6/28/2018 2:15 PM, Rafal Kozik wrote: >>>> Write combining (WC) increases NIC performance by making better >>>> utilization of PCI bus, but cannot be used by all PMDs. >>>> >>>> It will be enabled only if RTE_PCI_DRV_WC_ACTIVATE will be set in >>>> drivers flags. For proper work also igb_uio driver must be loaded with >>>> wc_activate set to 1. >>>> >>>> When mapping PCI resources, firstly try to us WC. >>>> If it is not supported it will fallback to normal mode. >>>> >>>> Signed-off-by: Rafal Kozik >>>> Acked-by: Bruce Richardson >>>> --- >>>> drivers/bus/pci/linux/pci_uio.c | 41 +++++++++++++++++++++++++++++---= --------- >>>> drivers/bus/pci/rte_bus_pci.h | 2 ++ >>>> 2 files changed, 31 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/p= ci_uio.c >>>> index d423e4b..e3947c2 100644 >>>> --- a/drivers/bus/pci/linux/pci_uio.c >>>> +++ b/drivers/bus/pci/linux/pci_uio.c >>>> @@ -282,22 +282,19 @@ int >>>> pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx= , >>>> struct mapped_pci_resource *uio_res, int map_idx) >>>> { >>>> - int fd; >>>> + int fd =3D -1; >>>> char devname[PATH_MAX]; >>>> void *mapaddr; >>>> struct rte_pci_addr *loc; >>>> struct pci_map *maps; >>>> + int wc_activate =3D 0; >>>> + >>>> + if (dev->driver !=3D NULL) >>>> + wc_activate =3D dev->driver->drv_flags & RTE_PCI_DRV_WC_= ACTIVATE; >>>> >>>> loc =3D &dev->addr; >>>> maps =3D uio_res->maps; >>>> >>>> - /* update devname for mmap */ >>>> - snprintf(devname, sizeof(devname), >>>> - "%s/" PCI_PRI_FMT "/resource%d", >>>> - rte_pci_get_sysfs_path(), >>>> - loc->domain, loc->bus, loc->devid, >>>> - loc->function, res_idx); >>>> - >>>> /* allocate memory to keep path */ >>>> maps[map_idx].path =3D rte_malloc(NULL, strlen(devname) + 1, 0); >>>> if (maps[map_idx].path =3D=3D NULL) { >>>> @@ -309,11 +306,31 @@ pci_uio_map_resource_by_index(struct rte_pci_dev= ice *dev, int res_idx, >>>> /* >>>> * open resource file, to mmap it >>>> */ >>>> - fd =3D open(devname, O_RDWR); >>>> - if (fd < 0) { >>>> - RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", >>>> + if (wc_activate) { >>>> + /* update devname for mmap */ >>>> + snprintf(devname, sizeof(devname), >>>> + "%s/" PCI_PRI_FMT "/resource%d_wc", >>>> + rte_pci_get_sysfs_path(), >>>> + loc->domain, loc->bus, loc->devid, >>>> + loc->function, res_idx); >>>> + >>>> + fd =3D open(devname, O_RDWR); >>> >>> What do you think adding an error log here? If opening resource$_wc fai= ls and >>> fallback to resource# file, there won't be any way for user to know abo= ut it. >>> >> >> This error will be misleading for two reasons: >> * even if open return success, it could silently fall-back to non >> prefetchable mode, >> * NICs could have multiple BARs, but not all support WC. I such case >> fails will be desirable. > > OK, perhaps not error log to prevent mislead, but what do you think "info= " level > log, to notify the user that write combined version in not used. > In new revision of patch set I add logging of device name. For every resources it provides information if mapped with or without WC. It looks like: EAL: /sys/bus/pci/devices/0000:00:06.0/resource0 mapped EAL: /sys/bus/pci/devices/0000:00:06.0/resource2_wc mapped EAL: /sys/bus/pci/devices/0000:00:06.0/resource4 mapped >> >>>> + } >>>> + >>>> + if (!wc_activate || fd < 0) { >>>> + snprintf(devname, sizeof(devname), >>>> + "%s/" PCI_PRI_FMT "/resource%d", >>>> + rte_pci_get_sysfs_path(), >>>> + loc->domain, loc->bus, loc->devid, >>>> + loc->function, res_idx); >>>> + >>>> + /* then try to map resource file */ >>>> + fd =3D open(devname, O_RDWR); >>>> + if (fd < 0) { >>>> + RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", >>>> devname, strerror(errno)); >>>> - goto error; >>>> + goto error; >>>> + } >>>> } >>>> >>>> /* try mapping somewhere close to the end of hugepages */ >>>> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_p= ci.h >>>> index 458e6d0..828acc5 100644 >>>> --- a/drivers/bus/pci/rte_bus_pci.h >>>> +++ b/drivers/bus/pci/rte_bus_pci.h >>>> @@ -135,6 +135,8 @@ struct rte_pci_bus { >>>> >>>> /** Device needs PCI BAR mapping (done with either IGB_UIO or VFIO) *= / >>>> #define RTE_PCI_DRV_NEED_MAPPING 0x0001 >>>> +/** Device needs PCI BAR mapping with enabled write combining (wc) */ >>>> +#define RTE_PCI_DRV_WC_ACTIVATE 0x0002 >>>> /** Device driver supports link state interrupt */ >>>> #define RTE_PCI_DRV_INTR_LSC 0x0008 >>>> /** Device driver supports device removal interrupt */ >>>> >>> >