[v2,2/5] devargs: refactor scratch buffer storage

Message ID 1610983002-7630-3-git-send-email-xuemingl@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series eal: enable global device syntax |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Xueming Li Jan. 18, 2021, 3:16 p.m. UTC
  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 <xuemingl@nvidia.com>
---
 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(-)
  

Comments

Thomas Monjalon March 18, 2021, 9:14 a.m. UTC | #1
18/01/2021 16:16, Xueming Li:
> 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 <xuemingl@nvidia.com>
> ---
> --- 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. */

Adding a field changes the size of the struct, which is an ABI break.
We need to plan this change for DPDK 21.11.
Let's think what can be done in the meantime.
  

Patch

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;