From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id DCA8EA0A03; Mon, 18 Jan 2021 16:17:05 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5E9D6140F37; Mon, 18 Jan 2021 16:16:55 +0100 (CET) Received: from mellanox.co.il (mail-il-dmz.mellanox.com [193.47.165.129]) by mails.dpdk.org (Postfix) with ESMTP id AD0A7140F2C for ; Mon, 18 Jan 2021 16:16:50 +0100 (CET) Received: from Internal Mail-Server by MTLPINE1 (envelope-from xuemingl@nvidia.com) with SMTP; 18 Jan 2021 17:16:49 +0200 Received: from nvidia.com (pegasus05.mtr.labs.mlnx [10.210.16.100]) by labmailer.mlnx (8.13.8/8.13.8) with ESMTP id 10IFGmBn002238; Mon, 18 Jan 2021 17:16:48 +0200 From: Xueming Li To: Thomas Monjalon , Ferruh Yigit , Andrew Rybchenko , Olivier Matz Cc: dev@dpdk.org, Viacheslav Ovsiienko , xuemingl@nvidia.com, Asaf Penso Date: Mon, 18 Jan 2021 15:16:39 +0000 Message-Id: <1610983002-7630-3-git-send-email-xuemingl@nvidia.com> X-Mailer: git-send-email 1.8.3.1 In-Reply-To: <1610983002-7630-1-git-send-email-xuemingl@nvidia.com> References: <1610983002-7630-1-git-send-email-xuemingl@nvidia.com> In-Reply-To: <1608304614-13908-2-git-send-email-xuemingl@nvidia.com> References: <1608304614-13908-2-git-send-email-xuemingl@nvidia.com> Subject: [dpdk-dev] [PATCH v2 2/5] devargs: refactor scratch buffer storage X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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" In current design, legacy parser rte_devargs_parse() saved scratch buffer to devargs.args while new parser rte_devargs_layers_parse() saved to devargs.data. Code using devargs had to know the difference and cleaned up memory accordingly - error prone. This patch unifies data the dedicate scratch buffer, introduces rte_devargs_free() function to wrap the memory memory clean up. Signed-off-by: Xueming Li --- app/test-pmd/config.c | 7 ++-- app/test-pmd/testpmd.c | 5 ++- drivers/bus/vdev/vdev.c | 9 ++-- drivers/net/failsafe/failsafe_args.c | 3 +- drivers/net/failsafe/failsafe_eal.c | 2 +- examples/multi_process/hotplug_mp/commands.c | 6 +-- lib/librte_eal/common/eal_common_dev.c | 9 ++-- lib/librte_eal/common/eal_common_devargs.c | 43 +++++++++++--------- lib/librte_eal/common/hotplug_mp.c | 6 +-- lib/librte_eal/include/rte_devargs.h | 18 ++++++-- lib/librte_eal/rte_eal_exports.def | 1 + lib/librte_eal/version.map | 1 + lib/librte_ethdev/rte_ethdev.c | 7 ++-- 13 files changed, 64 insertions(+), 53 deletions(-) diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index 3f6c8642b1..21bdece399 100644 --- a/app/test-pmd/config.c +++ b/app/test-pmd/config.c @@ -509,8 +509,6 @@ device_infos_display(const char *identifier) if (rte_devargs_parsef(&da, "%s", identifier)) { printf("cannot parse identifier\n"); - if (da.args) - free(da.args); return; } @@ -558,6 +556,7 @@ device_infos_display(const char *identifier) } } }; + rte_devargs_free(&da); } void @@ -602,8 +601,8 @@ port_infos_display(portid_t port_id) else printf("\nFirmware-version: %s", "not available"); - if (dev_info.device->devargs && dev_info.device->devargs->args) - printf("\nDevargs: %s", dev_info.device->devargs->args); + if (dev_info.device->devargs && dev_info.device->devargs->src) + printf("\nDevargs: %s", dev_info.device->devargs->src); printf("\nConnect to socket: %u", port->socket_id); if (port_numa[port_id] != NUMA_NO_CONFIG) { diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index 2b60f6c5d3..ea27779bfd 100644 --- a/app/test-pmd/testpmd.c +++ b/app/test-pmd/testpmd.c @@ -2987,8 +2987,6 @@ detach_devargs(char *identifier) memset(&da, 0, sizeof(da)); if (rte_devargs_parsef(&da, "%s", identifier)) { printf("cannot parse identifier\n"); - if (da.args) - free(da.args); return; } @@ -2997,6 +2995,7 @@ detach_devargs(char *identifier) if (ports[port_id].port_status != RTE_PORT_STOPPED) { printf("Port %u not stopped\n", port_id); rte_eth_iterator_cleanup(&iterator); + rte_devargs_free(&da); return; } port_flow_flush(port_id); @@ -3006,6 +3005,7 @@ detach_devargs(char *identifier) if (rte_eal_hotplug_remove(da.bus->name, da.name) != 0) { TESTPMD_LOG(ERR, "Failed to detach device %s(%s)\n", da.name, da.bus->name); + rte_devargs_free(&da); return; } @@ -3014,6 +3014,7 @@ detach_devargs(char *identifier) printf("Device %s is detached\n", identifier); printf("Now total ports is %d\n", nb_ports); printf("Done\n"); + rte_devargs_free(&da); } void diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c index acfd78828f..012326d809 100644 --- a/drivers/bus/vdev/vdev.c +++ b/drivers/bus/vdev/vdev.c @@ -236,13 +236,14 @@ alloc_devargs(const char *name, const char *args) devargs->bus = &rte_vdev_bus; if (args) - devargs->args = strdup(args); + devargs->data = strdup(args); else - devargs->args = strdup(""); + devargs->data = strdup(""); + devargs->args = devargs->data; ret = strlcpy(devargs->name, name, sizeof(devargs->name)); if (ret < 0 || ret >= (int)sizeof(devargs->name)) { - free(devargs->args); + rte_devargs_free(devargs); free(devargs); return NULL; } @@ -296,7 +297,7 @@ insert_vdev(const char *name, const char *args, return 0; fail: - free(devargs->args); + rte_devargs_free(devargs); free(devargs); free(dev); return ret; diff --git a/drivers/net/failsafe/failsafe_args.c b/drivers/net/failsafe/failsafe_args.c index 707490b94c..52fdcb977f 100644 --- a/drivers/net/failsafe/failsafe_args.c +++ b/drivers/net/failsafe/failsafe_args.c @@ -451,8 +451,7 @@ failsafe_args_free(struct rte_eth_dev *dev) sdev->cmdline = NULL; free(sdev->fd_str); sdev->fd_str = NULL; - free(sdev->devargs.args); - sdev->devargs.args = NULL; + rte_devargs_free(&sdev->devargs); } } diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c index b9fc508673..3a4d8c835a 100644 --- a/drivers/net/failsafe/failsafe_eal.c +++ b/drivers/net/failsafe/failsafe_eal.c @@ -79,7 +79,7 @@ fs_bus_init(struct rte_eth_dev *dev) rte_eth_devices[pid].device->devargs; /* Take control of probed device. */ - free(da->args); + rte_devargs_free(da); memset(da, 0, sizeof(*da)); if (probed_da != NULL) snprintf(devstr, sizeof(devstr), "%s,%s", diff --git a/examples/multi_process/hotplug_mp/commands.c b/examples/multi_process/hotplug_mp/commands.c index a8a39d07f7..e593cad56c 100644 --- a/examples/multi_process/hotplug_mp/commands.c +++ b/examples/multi_process/hotplug_mp/commands.c @@ -121,8 +121,6 @@ static void cmd_dev_attach_parsed(void *parsed_result, if (rte_devargs_parsef(&da, "%s", res->devargs)) { cmdline_printf(cl, "cannot parse devargs\n"); - if (da.args) - free(da.args); return; } @@ -131,6 +129,7 @@ static void cmd_dev_attach_parsed(void *parsed_result, else cmdline_printf(cl, "failed to attached device %s\n", da.name); + rte_devargs_free(&da); } cmdline_parse_token_string_t cmd_dev_attach_attach = @@ -168,8 +167,6 @@ static void cmd_dev_detach_parsed(void *parsed_result, if (rte_devargs_parsef(&da, "%s", res->devargs)) { cmdline_printf(cl, "cannot parse devargs\n"); - if (da.args) - free(da.args); return; } @@ -180,6 +177,7 @@ static void cmd_dev_detach_parsed(void *parsed_result, else cmdline_printf(cl, "failed to dettach device %s\n", da.name); + rte_devargs_free(&da); } cmdline_parse_token_string_t cmd_dev_detach_detach = diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c index 8a3bd3100a..4b4d589f64 100644 --- a/lib/librte_eal/common/eal_common_dev.c +++ b/lib/librte_eal/common/eal_common_dev.c @@ -185,10 +185,8 @@ local_dev_probe(const char *devargs, struct rte_device **new_dev) return ret; err_devarg: - if (rte_devargs_remove(da) != 0) { - free(da->args); - free(da); - } + if (rte_devargs_remove(da) != 0) + rte_devargs_free(da); return ret; } @@ -586,7 +584,7 @@ rte_dev_iterator_init(struct rte_dev_iterator *it, it->bus_str = NULL; it->cls_str = NULL; - devargs.data = dev_str; + devargs.data = (void *)(intptr_t)dev_str; if (rte_devargs_layers_parse(&devargs, dev_str)) goto get_out; @@ -619,6 +617,7 @@ rte_dev_iterator_init(struct rte_dev_iterator *it, it->device = NULL; it->class_device = NULL; get_out: + rte_devargs_free(&devargs); return -rte_errno; } diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c index c3969ff158..9c7a7de30e 100644 --- a/lib/librte_eal/common/eal_common_devargs.c +++ b/lib/librte_eal/common/eal_common_devargs.c @@ -144,13 +144,14 @@ rte_devargs_layers_parse(struct rte_devargs *devargs, devargs->drv_str = layers[2].str; devargs->bus = bus; devargs->cls = cls; + devargs->src = devstr; /* If we own the data, clean up a bit * the several layers string, to ease * their parsing afterward. */ if (devargs->data != devstr) { - char *s = (void *)(intptr_t)(devargs->data); + char *s = devargs->data; while ((s = strchr(s, '/'))) { *s = '\0'; @@ -164,12 +165,8 @@ rte_devargs_layers_parse(struct rte_devargs *devargs, rte_kvargs_free(layers[i].kvlist); } if (ret != 0) { - if (devargs->data && devargs->data != devstr) { - /* Free duplicated data. */ - free(devargs->data); - devargs->data = NULL; - } rte_errno = -ret; + rte_devargs_free(devargs); } return ret; } @@ -225,13 +222,17 @@ rte_devargs_parse(struct rte_devargs *da, const char *dev) da->bus = bus; /* Parse eventual device arguments */ if (devname[i] == ',') - da->args = strdup(&devname[i + 1]); + da->data = strdup(&devname[i + 1]); else - da->args = strdup(""); - if (da->args == NULL) { + da->data = strdup(""); + if (da->data == NULL) { RTE_LOG(ERR, EAL, "not enough memory to parse arguments\n"); return -ENOMEM; } + da->drv_str = da->data; + + da->src = dev; + return 0; } @@ -266,6 +267,15 @@ rte_devargs_parsef(struct rte_devargs *da, const char *format, ...) return ret; } +void +rte_devargs_free(struct rte_devargs *da) +{ + if (da && da->data && da->data != da->src) + free(da->data); + da->data = NULL; + da->src = NULL; +} + int rte_devargs_insert(struct rte_devargs **da) { @@ -282,15 +292,8 @@ rte_devargs_insert(struct rte_devargs **da) if (strcmp(listed_da->bus->name, (*da)->bus->name) == 0 && strcmp(listed_da->name, (*da)->name) == 0) { /* device already in devargs list, must be updated */ - listed_da->type = (*da)->type; - listed_da->policy = (*da)->policy; - free(listed_da->args); - listed_da->args = (*da)->args; - listed_da->bus = (*da)->bus; - listed_da->cls = (*da)->cls; - listed_da->bus_str = (*da)->bus_str; - listed_da->cls_str = (*da)->cls_str; - listed_da->data = (*da)->data; + rte_devargs_free(listed_da); + *listed_da = **da; /* replace provided devargs with found one */ free(*da); *da = listed_da; @@ -332,7 +335,7 @@ rte_devargs_add(enum rte_devtype devtype, const char *devargs_str) fail: if (devargs) { - free(devargs->args); + rte_devargs_free(devargs); free(devargs); } @@ -352,7 +355,7 @@ rte_devargs_remove(struct rte_devargs *devargs) if (strcmp(d->bus->name, devargs->bus->name) == 0 && strcmp(d->name, devargs->name) == 0) { TAILQ_REMOVE(&devargs_list, d, next); - free(d->args); + rte_devargs_free(d); free(d); return 0; } diff --git a/lib/librte_eal/common/hotplug_mp.c b/lib/librte_eal/common/hotplug_mp.c index ee791903b3..13f2a427cf 100644 --- a/lib/librte_eal/common/hotplug_mp.c +++ b/lib/librte_eal/common/hotplug_mp.c @@ -95,6 +95,7 @@ __handle_secondary_request(void *param) tmp_req = *req; + memset(&da, 0, sizeof(da)); if (req->t == EAL_DEV_REQ_TYPE_ATTACH) { ret = local_dev_probe(req->devargs, &dev); if (ret != 0) { @@ -118,8 +119,6 @@ __handle_secondary_request(void *param) ret = rte_devargs_parse(&da, req->devargs); if (ret != 0) goto finish; - free(da.args); /* we don't need those */ - da.args = NULL; ret = eal_dev_hotplug_request_to_secondary(&tmp_req); if (ret != 0) { @@ -176,6 +175,7 @@ __handle_secondary_request(void *param) if (ret) RTE_LOG(ERR, EAL, "failed to send response to secondary\n"); + rte_devargs_free(&da); free(bundle->peer); free(bundle); } @@ -283,7 +283,7 @@ static void __handle_primary_request(void *param) ret = local_dev_remove(dev); quit: - free(da->args); + rte_devargs_free(da); free(da); break; default: diff --git a/lib/librte_eal/include/rte_devargs.h b/lib/librte_eal/include/rte_devargs.h index 296f19324f..4a917a266b 100644 --- a/lib/librte_eal/include/rte_devargs.h +++ b/lib/librte_eal/include/rte_devargs.h @@ -60,16 +60,16 @@ struct rte_devargs { /** Name of the device. */ char name[RTE_DEV_NAME_MAX_LEN]; RTE_STD_C11 - union { - /** Arguments string as given by user or "" for no argument. */ - char *args; + union { /**< driver-related part of device string. */ + const char *args; /**< legacy name. */ const char *drv_str; }; struct rte_bus *bus; /**< bus handle. */ struct rte_class *cls; /**< class handle. */ const char *bus_str; /**< bus-related part of device string. */ const char *cls_str; /**< class-related part of device string. */ - const char *data; /**< Device string storage. */ + char *data; /**< Scratch buffer. */ + const char *src; /**< Arguments given by user. */ }; /** @@ -145,6 +145,16 @@ rte_devargs_parsef(struct rte_devargs *da, const char *format, ...) __rte_format_printf(2, 0); +/** + * Free resources in devargs. + * + * @param da + * The devargs structure holding the device information. + */ +__rte_experimental +void +rte_devargs_free(struct rte_devargs *da); + /** * Insert an rte_devargs in the global list. * diff --git a/lib/librte_eal/rte_eal_exports.def b/lib/librte_eal/rte_eal_exports.def index fe27bffe45..6fb1aaf8a8 100644 --- a/lib/librte_eal/rte_eal_exports.def +++ b/lib/librte_eal/rte_eal_exports.def @@ -29,6 +29,7 @@ EXPORTS rte_devargs_next rte_devargs_parse rte_devargs_parsef + rte_devargs_free rte_devargs_remove rte_devargs_type_count rte_dump_physmem_layout diff --git a/lib/librte_eal/version.map b/lib/librte_eal/version.map index b1db7ec795..ef388a30a1 100644 --- a/lib/librte_eal/version.map +++ b/lib/librte_eal/version.map @@ -409,6 +409,7 @@ EXPERIMENTAL { rte_thread_tls_key_delete; rte_thread_tls_value_get; rte_thread_tls_value_set; + rte_devargs_free; }; INTERNAL { diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 17ddacc78d..325e7693eb 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -193,13 +193,14 @@ int rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str) { int ret; - struct rte_devargs devargs = {.args = NULL}; + struct rte_devargs devargs; const char *bus_param_key; char *bus_str = NULL; char *cls_str = NULL; int str_size; memset(iter, 0, sizeof(*iter)); + memset(&devargs, 0, sizeof(devargs)); /* * The devargs string may use various syntaxes: @@ -244,8 +245,6 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str) goto error; } iter->cls_str = cls_str; - free(devargs.args); /* allocated by rte_devargs_parse() */ - devargs.args = NULL; iter->bus = devargs.bus; if (iter->bus->dev_iterate == NULL) { @@ -284,7 +283,7 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str) if (ret == -ENOTSUP) RTE_ETHDEV_LOG(ERR, "Bus %s does not support iterating.\n", iter->bus->name); - free(devargs.args); + rte_devargs_free(&devargs); free(bus_str); free(cls_str); return ret; -- 2.25.1