From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v7 6/6] devargs: parse bus info Date: Thu, 6 Jul 2017 18:47:15 +0530 Message-ID: <2642be49-c8ad-899f-c176-9749d26dce8a@nxp.com> References: <39810ff7-d5fd-921b-4fad-81a8013bf1c4@nxp.com> <20170706100021.GJ11154@bidouze.vm.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Cc: To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= Return-path: Received: from NAM02-SN1-obe.outbound.protection.outlook.com (mail-sn1nam02on0068.outbound.protection.outlook.com [104.47.36.68]) by dpdk.org (Postfix) with ESMTP id 6F4DD1094 for ; Thu, 6 Jul 2017 15:07:53 +0200 (CEST) In-Reply-To: <20170706100021.GJ11154@bidouze.vm.6wind.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Gaetan, On Thursday 06 July 2017 03:30 PM, Gaëtan Rivet wrote: > 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. Ok. I am ok with this. > >>> +} >>> + >>> /* 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. Ok. I completely missed this logic. Thanks. > >>> + 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 > And, for this patch as well: Acked-by: Shreyansh Jain