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 10BD3A0547; Tue, 30 Mar 2021 14:15:52 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EAE39140DE6; Tue, 30 Mar 2021 14:15:51 +0200 (CEST) Received: from mellanox.co.il (mail-il-dmz.mellanox.com [193.47.165.129]) by mails.dpdk.org (Postfix) with ESMTP id 0DB9B140DE4 for ; Tue, 30 Mar 2021 14:15:50 +0200 (CEST) Received: from Internal Mail-Server by MTLPINE1 (envelope-from xuemingl@nvidia.com) with SMTP; 30 Mar 2021 15:15:45 +0300 Received: from nvidia.com (pegasus05.mtr.labs.mlnx [10.210.16.100]) by labmailer.mlnx (8.13.8/8.13.8) with ESMTP id 12UCFXhb032518; Tue, 30 Mar 2021 15:15:45 +0300 From: Xueming Li To: Thomas Monjalon , Gaetan Rivet Cc: dev@dpdk.org, xuemingl@nvidia.com, Asaf Penso , Wenzhuo Lu , Beilei Xing , Bernard Iremonger , Gaetan Rivet , Anatoly Burakov , Dmitry Kozlyuk , Narcisa Ana Maria Vasile , Dmitry Malloy , Pallavi Kadam , Ray Kinsella , Neil Horman , Ferruh Yigit , Andrew Rybchenko Date: Tue, 30 Mar 2021 12:15:16 +0000 Message-Id: <1617106521-25931-2-git-send-email-xuemingl@nvidia.com> X-Mailer: git-send-email 1.8.3.1 In-Reply-To: <1617106521-25931-1-git-send-email-xuemingl@nvidia.com> References: <1617106521-25931-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 v3 1/5] devargs: unify 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 scratch buffer to data field, introduces rte_devargs_reset() function to wrap the memory clean up logic. Signed-off-by: Xueming Li --- app/test-pmd/config.c | 3 +- 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 | 34 +++++++++++--------- 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 | 8 ++--- 13 files changed, 59 insertions(+), 46 deletions(-) diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index ef0b9784d0..d774610419 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_reset(&da); } void diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index 96d2e0fcec..d4be23f8f8 100644 --- a/app/test-pmd/testpmd.c +++ b/app/test-pmd/testpmd.c @@ -3015,8 +3015,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; } @@ -3025,6 +3023,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_reset(&da); return; } port_flow_flush(port_id); @@ -3034,6 +3033,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_reset(&da); return; } @@ -3042,6 +3042,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_reset(&da); } void diff --git a/drivers/bus/vdev/vdev.c b/drivers/bus/vdev/vdev.c index 9a673347ae..d075409942 100644 --- a/drivers/bus/vdev/vdev.c +++ b/drivers/bus/vdev/vdev.c @@ -245,13 +245,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_reset(devargs); free(devargs); return NULL; } @@ -305,7 +306,7 @@ insert_vdev(const char *name, const char *args, return 0; fail: - free(devargs->args); + rte_devargs_reset(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..b203e02d9a 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_reset(&sdev->devargs); } } diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c index b9fc508673..cb4a2abc02 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_reset(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..48fd329583 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_reset(&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_reset(&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..148a23830a 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_reset(da); return ret; } @@ -586,7 +584,8 @@ rte_dev_iterator_init(struct rte_dev_iterator *it, it->bus_str = NULL; it->cls_str = NULL; - devargs.data = dev_str; + /* Setting data field implies no malloc in parsing. */ + devargs.data = (void *)(intptr_t)dev_str; if (rte_devargs_layers_parse(&devargs, dev_str)) goto get_out; diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c index fcf3d9a3cc..48f85ee9c0 100644 --- a/lib/librte_eal/common/eal_common_devargs.c +++ b/lib/librte_eal/common/eal_common_devargs.c @@ -150,7 +150,7 @@ rte_devargs_layers_parse(struct rte_devargs *devargs, * their parsing afterward. */ if (devargs->data != devstr) { - char *s = (void *)(intptr_t)(devargs->data); + char *s = devargs->data; while ((s = strchr(s, '/'))) { *s = '\0'; @@ -219,13 +219,14 @@ 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; return 0; } @@ -260,6 +261,16 @@ rte_devargs_parsef(struct rte_devargs *da, const char *format, ...) return ret; } +void +rte_devargs_reset(struct rte_devargs *da) +{ + if (da == NULL) + return; + if (da->data) + free(da->data); + da->data = NULL; +} + int rte_devargs_insert(struct rte_devargs **da) { @@ -276,15 +287,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_reset(listed_da); + *listed_da = **da; /* replace provided devargs with found one */ free(*da); *da = listed_da; @@ -326,7 +330,7 @@ rte_devargs_add(enum rte_devtype devtype, const char *devargs_str) fail: if (devargs) { - free(devargs->args); + rte_devargs_reset(devargs); free(devargs); } @@ -346,7 +350,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_reset(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..ae6010e8f8 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_reset(&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_reset(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..134b44a887 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; + /**< Raw string including bus, class and driver arguments. */ }; /** @@ -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_reset(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 474cf123fa..53933b3269 100644 --- a/lib/librte_eal/rte_eal_exports.def +++ b/lib/librte_eal/rte_eal_exports.def @@ -30,6 +30,7 @@ EXPORTS rte_devargs_parse rte_devargs_parsef rte_devargs_remove + rte_devargs_reset rte_devargs_type_count rte_dump_physmem_layout rte_dump_stack diff --git a/lib/librte_eal/version.map b/lib/librte_eal/version.map index 48a2b55d57..38c2aea5be 100644 --- a/lib/librte_eal/version.map +++ b/lib/librte_eal/version.map @@ -415,6 +415,7 @@ EXPERIMENTAL { rte_thread_tls_value_set; # added in 21.05 + rte_devargs_reset; rte_version_minor; rte_version_month; rte_version_prefix; diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 3059aa55b3..e11a95558f 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) { @@ -278,13 +277,14 @@ rte_eth_iterator_init(struct rte_dev_iterator *iter, const char *devargs_str) end: iter->cls = rte_class_find_by_name("eth"); + rte_devargs_reset(&devargs); return 0; error: if (ret == -ENOTSUP) RTE_ETHDEV_LOG(ERR, "Bus %s does not support iterating.\n", iter->bus->name); - free(devargs.args); + rte_devargs_reset(&devargs); free(bus_str); free(cls_str); return ret; -- 2.25.1