From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f182.google.com (mail-wr0-f182.google.com [209.85.128.182]) by dpdk.org (Postfix) with ESMTP id 2792E235 for ; Thu, 6 Jul 2017 12:00:30 +0200 (CEST) Received: by mail-wr0-f182.google.com with SMTP id 77so21787067wrb.1 for ; Thu, 06 Jul 2017 03:00:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=yiWuGfRyH9bDsX/b3npi7qiN8QC78QH71+JFgZijkRQ=; b=xexyWhnq5EmQeRM1EKutH6kjk0ardZ4WRJ371G3oAXN6UkwRk5Hi5ucMH8jBgrPxcT O/l5ig0/WEG6nEnvCAbh99SCPt1hhYV+8cq8MvnCecMRNFW3QQaWMbchhIUYNzx8gBKE qv5YeAUm4M5p2WTxc0Tr4tnxzaalr6zbRA7BbG2rq6FsozoBV1BdC7uOudEnBpfoRoEn Rsg3srpPCFC03IpshE6OhgPrGZempbU3IMX+gan6XG2C07ki1lmBwtOJNOS09vBIH0GR Oj0NEhp39qdGhJqngcgGhJCH+eNo4HBRz94gOA+rUUZxv3yhm6z0nGiROYcqzrdnr18d zpOg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=yiWuGfRyH9bDsX/b3npi7qiN8QC78QH71+JFgZijkRQ=; b=ko3HeA0n/IdQ/c5zMvjJbiD4/K5zm8hsUyV7hzQZ2fKNg7ZIwmzFusi4VriZP0PYaR nUMEtDPAzyzT3yMP9xvNkfTG5szlfSboGNzTmrSP+kt4XGlbn+28Nndf3trle75dUJOf GVcjfnHXOLqB/06k57Tcer0n0EF1OMzXneotiEk3qWKgh13Tut7O0YotvAhL6QUTXu+V auAAFsPlrbkLsXpVgdNzkzby8Vued8h4zunRggUbaTyPvcswiP/9MVNSEysmnzomSKCs r3Ax50dV3qlgba1PhJ2HsEIqgJS8lFHph6Hjw6NM0Suv762Qygtqs44Nc9lqeSBGvfzl Q+cw== X-Gm-Message-State: AKS2vOyohGPvewcp6MoM7+lWN6Ex5554cTNYbiojaXHxMUhUqBOXZ8LJ zpMitbzL6W9V2Cjx X-Received: by 10.28.52.142 with SMTP id b136mr26798899wma.48.1499335229705; Thu, 06 Jul 2017 03:00:29 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id r191sm5756491wmg.6.2017.07.06.03.00.28 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 06 Jul 2017 03:00:28 -0700 (PDT) Date: Thu, 6 Jul 2017 12:00:21 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Shreyansh Jain Cc: dev@dpdk.org Message-ID: <20170706100021.GJ11154@bidouze.vm.6wind.com> References: <39810ff7-d5fd-921b-4fad-81a8013bf1c4@nxp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <39810ff7-d5fd-921b-4fad-81a8013bf1c4@nxp.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v7 6/6] devargs: parse bus info 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: , X-List-Received-Date: Thu, 06 Jul 2017 10:00:30 -0000 On Thu, Jul 06, 2017 at 02:37:16PM +0530, Shreyansh Jain wrote: > Hello Gaetan, > > On Wednesday 05 July 2017 05:25 AM, Gaetan Rivet wrote: > >Signed-off-by: Gaetan Rivet > >--- > > lib/librte_eal/common/eal_common_devargs.c | 42 ++++++++++++++++++++++++----- > > lib/librte_eal/common/eal_common_vdev.c | 6 +++-- > > lib/librte_eal/common/include/rte_devargs.h | 3 +++ > > 3 files changed, 42 insertions(+), 9 deletions(-) > > > >diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c > >index ffa8ad9..102bd8d 100644 > >--- a/lib/librte_eal/common/eal_common_devargs.c > >+++ b/lib/librte_eal/common/eal_common_devargs.c > >@@ -78,12 +78,23 @@ rte_eal_parse_devargs_str(const char *devargs_str, > > return 0; > > } > >+static int > >+bus_name_cmp(const struct rte_bus *bus, const void *_name) > >+{ > >+ const char *name = _name; > >+ > >+ return strncmp(bus->name, name, > >+ strlen(bus->name)); > > Trivial: Any specific reason why this is split across multiple lines even > though it is less than 80 chars in total? > Not really, it's only a matter of taste. If is mostly to hightlight that we are limiting the comparison to bus->name length, by putting it on its own line. > >+} > >+ > > /* store a whitelist parameter for later parsing */ > > int > > rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str) > > { > > struct rte_devargs *devargs = NULL; > >- char *buf = NULL; > >+ struct rte_bus *bus = NULL; > >+ char *dev = NULL; > >+ const char *devname; > > int ret; > > /* use malloc instead of rte_malloc as it's called early at init */ > >@@ -94,34 +105,51 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str) > > memset(devargs, 0, sizeof(*devargs)); > > devargs->type = devtype; > >- if (rte_eal_parse_devargs_str(devargs_str, &buf, &devargs->args)) > >+ if (rte_eal_parse_devargs_str(devargs_str, &dev, &devargs->args)) > > A very basic (and possibly incorrect) question: > > here, for example "'net_pcap0,rx_pcap=input.pcap,tx_pcap=output.pcap'" was > passed to application, which would mean; > > dev = "net_pcap0" (name of the device) > devargs = "rx_pcap=input.pcap,tx_pcap=output.pcap" > > > goto fail; > >+ devname = dev; > >+ do { > >+ bus = rte_bus_find(bus, bus_name_cmp, dev); > >+ if (bus == NULL) > >+ break; > >+ devname = dev + strlen(bus->name) + 1; > > Assuming "vdev" bus: > "net_pcap0" > | > devname points here --------' (4+1) chars from start of "net_pcap0". > Is that the expectation? > Yes :) Well, actually, almost. The lines > >+ bus = rte_bus_find(bus, bus_name_cmp, dev); > >+ if (bus == NULL) > >+ break; Mean that at this point, we would already have broken out of the loop. But to continue with your example, assuming that we have a bus that is named "net_p": > Probably I am missing something here (maybe the input already has a bus > name.) > > Or, if this is not the case, then we will have to change the test > application (test_devargs.c) which passes such strings to > "rte_eal_devargs_add". > > >+ if (rte_bus_find_by_device_name(devname) == bus) > > Obviously, if my assumption is correct, this fails. Or, maybe I am > completely off the mark. > It should not be necessary to update the tests (yet). This bit of code tries to recognize bus names at the start of the declaration. However, some device names might be ambiguous and start like bus names for any reason. Device names have no specification beside not having commas within, bus names have no specification at all. Thus, we first try here to recognize a bus name within dev, but we do not stop here. We also verify afterward that the resulting device would be correct, and that its bus handler is actually the same as the bus we first recognized. In your example, the line > >+ if (rte_bus_find_by_device_name(devname) == bus) Fails, as the device is incorrect and rte_bus_find_by_device_name returns NULL as no bus is able to handle a device named > "cap0,rx_pcap=input.pcap,tx_pcap=output.pcap" Considering this, we should break from this loop with no recognized bus. As such, we enter afterward in the branch: > >+ break; > >+ devname = dev; Note here that devname is set back to the start of dev. > >+ } while (1); > >+ if (bus == NULL) { > >+ bus = rte_bus_find_by_device_name(devname); Which will try to recognize a bus from the device name ("net_pcap0"), which should succeed in recognizing vdev. It is a little contrived, but I wanted this parsing to both be flexible and perform as many checks as possible. I am developing a new bus that have a high probability of name conflicts with other device names, so I am extra careful on this side. > >+ if (bus == NULL) { > >+ fprintf(stderr, "ERROR: failed to parse bus info from device declaration\n"); > >+ goto fail; > >+ } > >+ } > >+ devargs->bus = bus; > > switch (devargs->type) { > > case RTE_DEVTYPE_WHITELISTED_PCI: > > case RTE_DEVTYPE_BLACKLISTED_PCI: > > /* try to parse pci identifier */ > >- if (eal_parse_pci_BDF(buf, &devargs->pci.addr) != 0 && > >- eal_parse_pci_DomBDF(buf, &devargs->pci.addr) != 0) > >+ if (bus->parse(devname, &devargs->pci.addr) != 0) > > goto fail; > > break; > > case RTE_DEVTYPE_VIRTUAL: > > /* save driver name */ > > ret = snprintf(devargs->virt.drv_name, > >- sizeof(devargs->virt.drv_name), "%s", buf); > >+ sizeof(devargs->virt.drv_name), "%s", devname); > > if (ret < 0 || ret >= (int)sizeof(devargs->virt.drv_name)) > > goto fail; > > break; > > } > > I think all this will change in the devargs series. > Indeed :) > >- free(buf); > >+ free(dev); > > TAILQ_INSERT_TAIL(&devargs_list, devargs, next); > > return 0; > > fail: > >- free(buf); > >+ free(dev); > > if (devargs) { > > free(devargs->args); > > free(devargs); > >diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c > >index 6ecd1b5..8fd1ef7 100644 > >--- a/lib/librte_eal/common/eal_common_vdev.c > >+++ b/lib/librte_eal/common/eal_common_vdev.c > >@@ -177,6 +177,7 @@ alloc_devargs(const char *name, const char *args) > > return NULL; > > devargs->type = RTE_DEVTYPE_VIRTUAL; > >+ devargs->bus = rte_bus_find_by_name("vdev"); > > if (args) > > devargs->args = strdup(args); > >@@ -289,12 +290,13 @@ vdev_scan(void) > > { > > struct rte_vdev_device *dev; > > struct rte_devargs *devargs; > >+ struct rte_bus *vbus; > > /* for virtual devices we scan the devargs_list populated via cmdline */ > >- > >+ vbus = rte_bus_find_by_name("vdev"); > > TAILQ_FOREACH(devargs, &devargs_list, next) { > >- if (devargs->type != RTE_DEVTYPE_VIRTUAL) > >+ if (devargs->bus != vbus) > > continue; > > dev = find_vdev(devargs->virt.drv_name); > >diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h > >index 88120a1..1f50a24 100644 > >--- a/lib/librte_eal/common/include/rte_devargs.h > >+++ b/lib/librte_eal/common/include/rte_devargs.h > >@@ -51,6 +51,7 @@ extern "C" { > > #include > > #include > > #include > >+#include > > /** > > * Type of generic device > >@@ -89,6 +90,8 @@ struct rte_devargs { > > char drv_name[32]; > > } virt; > > }; > >+ /** Bus handle for the device. */ > >+ struct rte_bus *bus; > > /** Arguments string as given by user or "" for no argument. */ > > char *args; > > }; > > > - > Shreyansh -- Gaëtan Rivet 6WIND