Re: [dpdk-dev] [PATCH 05/15 v3] ring: Convert to use of PMD_REGISTER_DRIVER and fix linking

["De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman
> Sent: Thursday, April 17, 2014 4:17 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 05/15 v3] ring: Convert to use of
> PMD_REGISTER_DRIVER and fix linking
> 
> convert the ring driver to use the PMD_REGISTER_DRIVER macro and fix up
> the
> Makefile so that its linkage is only done if we are building static libraries.
> This means that the test applications now have no reference to the ring
> library
> when building DSO's and must specify its use on the command line with the -
> d
> option.  Static linking will still initalize the driver automatically.
> 
> Note that the ring driver was also written in such a way that it violated some
> general layering principles, several functions were contained in the pmd
> which
> were being called by example from the test application in the app/test
> directory.  Specifically it was calling eth_ring_pair_attach,
> eth_ring_pair_create and rte_eth_ring_devinit, which should only be called
> internally to the dpdk core library.  To correct this I've removed those
> functions, and instead allowed them to be called indirectly at initalization
> time using the vdev command line argument key
> nodeaction=<name>:<node>:<action>
> where action is one of ATTACH or CREATE.  I've tested out the functionality of
> the command line with the testpmd utility, with success, and have removed
> the
> called functions from the test utility.  This will affect how the test utility
> is invoked (the -d and --vdev option will need to be specified on the
> command
> line now), but honestly, given the way it was coded, I think the testing of the
> ring pmd was not the best example of how to code with dpdk to begin with.  I
> have also left the two layer violating functions in place, so as not to break
> existing applications, but added deprecation warnings to them so that apps
> can
> migrate off them.
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> ---
> Change notes
> 
> v2) fixed DEPDIR specifcation, should depend on RING not PCAP
> 
> v3) Cleaned up strcmp error checking, printfs, and other parsing issues as
> pointed out by Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>  app/test/test_pmd_ring.c           |  95 -------------------------
>  lib/librte_pmd_ring/Makefile       |   1 +
>  lib/librte_pmd_ring/rte_eth_ring.c | 141
> ++++++++++++++++++++++++++++++++++---
>  mk/rte.app.mk                      |  14 ++--
>  4 files changed, 142 insertions(+), 109 deletions(-)
> 
> diff --git a/app/test/test_pmd_ring.c b/app/test/test_pmd_ring.c
> index 4d9c2ba..1fe38fa 100644
> --- a/app/test/test_pmd_ring.c
> +++ b/app/test/test_pmd_ring.c
> @@ -42,7 +42,6 @@
>  /* two test rings, r1 is used by two ports, r2 just by one */
>  static struct rte_ring *r1[2], *r2;
> 
> -static struct rte_ring *nullring = NULL;
>  static struct rte_mempool *mp;
>  static uint8_t start_idx; /* will store the port id of the first of our new ports
> */
> 
> @@ -59,58 +58,6 @@ static uint8_t start_idx; /* will store the port id of the
> first of our new port
>  #define MBUF_SIZE (2048 + sizeof(struct rte_mbuf) +
> RTE_PKTMBUF_HEADROOM)
>  #define NB_MBUF   512
> 
> -
> -static int
> -test_ring_ethdev_create(void)
> -{
> -	int retval;
> -	printf("Testing ring pmd create\n");
> -
> -	retval = rte_eth_from_rings(NULL, 0, NULL, 0, SOCKET0);
> -	if (retval < 0) {
> -		printf("Failure, failed to create zero-sized RXTX ring pmd\n");
> -		return -1;
> -	}
> -
> -	retval = rte_eth_from_rings(NULL, 0, NULL, 0,
> RTE_MAX_NUMA_NODES);
> -	if (retval >= 0) {
> -		printf("Failure, can create ring pmd on socket %d\n",
> RTE_MAX_NUMA_NODES);
> -		return -1;
> -	}
> -
> -	retval = rte_eth_from_rings(NULL, 1, &r2, 1, SOCKET0);
> -	if (retval >= 0) {
> -		printf("Failure, can create pmd with null rx rings\n");
> -		return -1;
> -	}
> -
> -	retval = rte_eth_from_rings(r1, 1, NULL, 1, SOCKET0);
> -	if (retval >= 0) {
> -		printf("Failure, can create pmd with null tx rings\n");
> -		return -1;
> -	}
> -
> -	retval = rte_eth_from_rings(&nullring, 1, r1, 2, SOCKET0);
> -	if (retval < 0) {
> -		printf("Failure, failed to create TX-only ring pmd\n");
> -		return -1;
> -	}
> -
> -	retval = rte_eth_from_rings(r1, 1, &nullring, 1, SOCKET0);
> -	if (retval < 0) {
> -		printf("Failure, failed to create RX-only ring pmd\n");
> -		return -1;
> -	}
> -
> -	retval = rte_eth_from_rings(&r2, 1, &r2, 1, SOCKET0);
> -	if (retval < 0) {
> -		printf("Failure, failed to create RXTX ring pmd\n");
> -		return -1;
> -	}
> -
> -	return 0;
> -}
> -
>  static int
>  test_ethdev_configure(void)
>  {
> @@ -305,26 +252,12 @@ test_stats_reset(void)
>  static int
>  test_pmd_ring_init(void)
>  {
> -	const char * name1 = "R3";
> -	const char * name2 = "R4";
> -	const char * params_null = NULL;
> -	const char * params = "PARAMS";
>  	struct rte_eth_stats stats;
>  	struct rte_mbuf buf, *pbuf = &buf;
>  	struct rte_eth_conf null_conf;
> 
>  	printf("Testing ring pmd init\n");
> 
> -	if (rte_pmd_ring_devinit(name1, params_null) < 0) {
> -		printf("Testing ring pmd init fail\n");
> -		return -1;
> -	}
> -
> -	if (rte_pmd_ring_devinit(name2, params) < 0) {
> -		printf("Testing ring pmd init fail\n");
> -		return -1;
> -	}
> -
>  	if (RXTX_PORT2 >= RTE_MAX_ETHPORTS) {
>  		printf(" TX/RX port exceed max eth ports\n");
>  		return -1;
> @@ -371,24 +304,16 @@ test_pmd_ring_init(void)
> 
>  	rte_eth_dev_stop(RXTX_PORT2);
> 
> -	/* Test init same name pmd ring */
> -	rte_pmd_ring_devinit(name1, params_null);
>  	return 0;
>  }
> 
>  static int
>  test_pmd_ring_pair_create(void)
>  {
> -	const char * name1 = "_RNG_P0";
>  	struct rte_eth_stats stats, stats2;
>  	struct rte_mbuf buf, *pbuf = &buf;
>  	struct rte_eth_conf null_conf;
> 
> -	if (rte_eth_ring_pair_create(name1, SOCKET0) < 0) {
> -		printf("Create ring pair failed\n");
> -		return -1;
> -	}
> -
>  	if ((RXTX_PORT4 >= RTE_MAX_ETHPORTS) || (RXTX_PORT5 >=
> RTE_MAX_ETHPORTS)) {
>  		printf(" TX/RX port exceed max eth ports\n");
>  		return -1;
> @@ -447,28 +372,16 @@ test_pmd_ring_pair_create(void)
>  	rte_eth_dev_stop(RXTX_PORT4);
>  	rte_eth_dev_stop(RXTX_PORT5);
> 
> -	/* Test create same name ring pair */
> -	if (rte_eth_ring_pair_create(name1, SOCKET0) == 0) {
> -		printf("Create same name ring pair error\n");
> -		return -1;
> -	}
>  	return 0;
>  }
> 
>  static int
>  test_pmd_ring_pair_attach(void)
>  {
> -	const char * name1 = "_RNG_P0";
> -	const char * name2 = "_RNG_P1";
>  	struct rte_eth_stats stats, stats2;
>  	struct rte_mbuf buf, *pbuf = &buf;
>  	struct rte_eth_conf null_conf;
> 
> -	if (rte_eth_ring_pair_attach(name1, SOCKET0) < 0) {
> -		printf("Attach ring pair failed\n");
> -		return -1;
> -	}
> -
>  	if ((RXTX_PORT4 >= RTE_MAX_ETHPORTS) || (RXTX_PORT5 >=
> RTE_MAX_ETHPORTS)) {
>  		printf(" TX/RX port exceed max eth ports\n");
>  		return -1;
> @@ -529,11 +442,6 @@ test_pmd_ring_pair_attach(void)
>  	rte_eth_dev_stop(RXTX_PORT4);
>  	rte_eth_dev_stop(RXTX_PORT5);
> 
> -	/* Test attach non-existing ring pair */
> -	if (rte_eth_ring_pair_attach(name2, SOCKET0) == 0) {
> -		printf("Attach non-existing ring pair error\n");
> -		return -1;
> -	}
>  	return 0;
>  }
> 
> @@ -568,9 +476,6 @@ test_pmd_ring(void)
>  		return -1;
>  	}
> 
> -	if (test_ring_ethdev_create() < 0)
> -		return -1;
> -
>  	if (test_ethdev_configure() < 0)
>  		return -1;
> 
> diff --git a/lib/librte_pmd_ring/Makefile b/lib/librte_pmd_ring/Makefile
> index 73b2d38..35d40cb 100644
> --- a/lib/librte_pmd_ring/Makefile
> +++ b/lib/librte_pmd_ring/Makefile
> @@ -52,5 +52,6 @@ SYMLINK-y-include += rte_eth_ring.h
>  # this lib depends upon:
>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_RING) += lib/librte_eal lib/librte_ring
>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_RING) += lib/librte_mbuf
> lib/librte_ether
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_RING) += lib/librte_kvargs
> 
>  include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/lib/librte_pmd_ring/rte_eth_ring.c
> b/lib/librte_pmd_ring/rte_eth_ring.c
> index cee3fff..3a129b6 100644
> --- a/lib/librte_pmd_ring/rte_eth_ring.c
> +++ b/lib/librte_pmd_ring/rte_eth_ring.c
> @@ -38,6 +38,17 @@
>  #include <rte_memcpy.h>
>  #include <rte_string_fns.h>
>  #include <rte_vdev.h>
> +#include <rte_pmd.h>
> +#include <rte_kvargs.h>
> +
> +#define ETH_RING_NUMA_NODE_ACTION_ARG	"nodeaction"
> +#define ETH_RING_ACTION_CREATE		"CREATE"
> +#define ETH_RING_ACTION_ATTACH		"ATTACH"
> +
> +static const char *valid_arguments[] = {
> +	ETH_RING_NUMA_NODE_ACTION_ARG,
> +	NULL
> +};
> 
>  struct ring_queue {
>  	struct rte_ring *rng;
> @@ -373,28 +384,143 @@ eth_dev_ring_pair_create(const char *name,
> const unsigned numa_node,
>  int
>  rte_eth_ring_pair_create(const char *name, const unsigned numa_node)
>  {
> +	RTE_LOG(WARNING, PMD, "rte_eth_ring_pair_create is
> deprecated\n");
>  	return eth_dev_ring_pair_create(name, numa_node, DEV_CREATE);
>  }
> 
>  int
>  rte_eth_ring_pair_attach(const char *name, const unsigned numa_node)
>  {
> +	RTE_LOG(WARNING, PMD, "rte_eth_ring_pair_attach is
> deprecated\n");
>  	return eth_dev_ring_pair_create(name, numa_node, DEV_ATTACH);
>  }
> 
> +struct node_action_pair {
> +	char name[PATH_MAX];
> +	unsigned node;
> +	enum dev_action action;
> +};
> +
> +struct node_action_list {
> +	unsigned total;
> +	unsigned count;
> +	struct node_action_pair *list;
> +};
> +
> +static int parse_kvlist (const char *key __rte_unused, const char *value,
> void *data)
> +{
> +	struct node_action_list *info = data;
> +	int ret;
> +	char *name;
> +	char *action;
> +	char *node;
> +	char *end;
> +
> +	name = strdup(value);
> +
> +	ret = -EINVAL;
> +
> +	if (!name) {
> +		RTE_LOG(WARNING, PMD, "command line paramter is empty
> for ring pmd!\n");
> +		goto out;
> +	}
> +
> +	node = strchr(name, ':');
> +	if (!node) {
> +		RTE_LOG(WARNING, PMD, "could not parse node value from
> %s", name);
> +		goto out;
> +	}
> +
> +	*node = '\0';
> +	node++;
> +
> +	action = strchr(node, ':');
> +	if (!action) {
> +		RTE_LOG(WARNING, PMD, "could not action value from %s",
> node);
> +		goto out;
> +	}
> +
> +	*action = '\0';
> +	action++;
> +
> +	/*
> +	 * Need to do some sanity checking here
> +	 */
> +
> +	if (strcmp(action, ETH_RING_ACTION_ATTACH) == 0)
> +		info->list[info->count].action = DEV_ATTACH;
> +	else if (strcmp(action, ETH_RING_ACTION_CREATE) == 0)
> +		info->list[info->count].action = DEV_CREATE;
> +	else
> +		goto out;
> +
> +	errno = 0;
> +	info->list[info->count].node = strtol(node, &end, 10);
> +
> +	if ((errno != 0) || (*end != '\0')) {
> +		RTE_LOG(WARNING, PMD, "node value %s is unparseable as
> a number\n", node);
> +		goto out;
> +	}
> +
> +	rte_snprintf(info->list[info->count].name, sizeof(info->list[info-
> >count].name), "%s", name);
> +
> +	info->count++;
> +
> +	ret = 0;
> +out:
> +	free(name);
> +	return ret;
> +}
> +
>  int
>  rte_pmd_ring_devinit(const char *name, const char *params)
>  {
> +	struct rte_kvargs *kvlist;
> +	int ret = 0;
> +	struct node_action_list *info = NULL;
> +
>  	RTE_LOG(INFO, PMD, "Initializing pmd_ring for %s\n", name);
> 
> +
>  	if (params == NULL || params[0] == '\0')
>  		eth_dev_ring_create(name, rte_socket_id(), DEV_CREATE);
>  	else {
> -		RTE_LOG(INFO, PMD, "Ignoring unsupported parameters
> when creating"
> -				" rings-backed ethernet device\n");
> -		eth_dev_ring_create(name, rte_socket_id(), DEV_CREATE);
> +		kvlist = rte_kvargs_parse(params, valid_arguments);
> +
> +		if (!kvlist) {
> +			RTE_LOG(INFO, PMD, "Ignoring unsupported
> parameters when creating"
> +					" rings-backed ethernet device\n");
> +			eth_dev_ring_create(name, rte_socket_id(),
> DEV_CREATE);
> +			return 0;
> +		} else {
> +			eth_dev_ring_create(name, rte_socket_id(),
> DEV_CREATE);
> +			ret = rte_kvargs_count(kvlist,
> ETH_RING_NUMA_NODE_ACTION_ARG);
> +			info = rte_zmalloc("struct node_action_list",
> sizeof(struct node_action_list) +
> +					   (sizeof(struct node_action_pair) *
> ret), 0);
> +			if (!info)
> +				goto out;
> +
> +			info->total = ret;
> +			info->list = (struct node_action_pair*)(info + 1);
> +
> +			ret = rte_kvargs_process(kvlist,
> ETH_RING_NUMA_NODE_ACTION_ARG,
> +						 parse_kvlist, info);
> +
> +			if (ret < 0)
> +				goto out_free;
> +
> +			for (info->count = 0; info->count < info->total; info-
> >count++) {
> +				eth_dev_ring_pair_create(name, info-
> >list[info->count].node,
> +						    info->list[info-
> >count].action);
> +			}
> +
> +		}
>  	}
> -	return 0;
> +
> +out_free:
> +	rte_free(info);
> +out:
> +	return ret;
>  }
> 
>  static struct rte_vdev_driver pmd_ring_drv = {
> @@ -402,9 +528,4 @@ static struct rte_vdev_driver pmd_ring_drv = {
>  	.init = rte_pmd_ring_devinit,
>  };
> 
> -__attribute__((constructor))
> -static void
> -rte_pmd_ring_init(void)
> -{
> -	rte_eal_vdev_driver_register(&pmd_ring_drv);
> -}
> +PMD_REGISTER_DRIVER(pmd_ring_drv, PMD_VDEV);
> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> index e6d09b8..4f167aa 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -133,10 +133,6 @@ ifeq ($(CONFIG_RTE_LIBRTE_ETHER),y)
>  LDLIBS += -lethdev
>  endif
> 
> -ifeq ($(CONFIG_RTE_LIBRTE_PMD_RING),y)
> -LDLIBS += -lrte_pmd_ring
> -endif
> -
>  ifeq ($(CONFIG_RTE_LIBRTE_MALLOC),y)
>  LDLIBS += -lrte_malloc
>  endif
> @@ -173,9 +169,19 @@ LDLIBS += -lrte_cmdline
>  endif
> 
>  ifeq ($(RTE_BUILD_SHARED_LIB),n)
> +
> +ifeq ($(CONFIG_RTE_LIBRTE_PMD_RING),y)
> +LDLIBS += -lrte_pmd_ring
> +endif
> +
> +ifeq ($(CONFIG_RTE_LIBRTE_PMD_RING),y)
> +LDLIBS += -lrte_pmd_ring
> +endif
> +
>  ifeq ($(CONFIG_RTE_LIBRTE_PMD_PCAP),y)
>  LDLIBS += -lrte_pmd_pcap -lpcap
>  endif
> +
>  endif
> 
>  LDLIBS += $(EXECENV_LDLIBS)
> --
> 1.8.3.1

Hi Neil,

Sorry for opening this thread again, but I have encountered that the ring PMD unit test (ring_pmd_autotest in the test application) fails, due to this patch, as several functions calls were removed (such as rte_eth_ring_pair_create) . According to the explanation of this patch, you said that using -d/-vdev is required for the test app. Have you checked if unit test worked for you? In that case, could you tell me which parameters are needed, please?

Thanks,
Pablo