From mboxrd@z Thu Jan  1 00:00:00 1970
From: "De Lara Guarch, Pablo" <pablo.de.lara.guarch-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Subject: Re: [PATCH 05/15 v3] ring: Convert to use
	of	PMD_REGISTER_DRIVER and fix linking
Date: Fri, 13 Jun 2014 13:28:36 +0000
Message-ID: <E115CCD9D858EF4F90C690B0DCB4D897082CACFF@IRSMSX103.ger.corp.intel.com>
References: <2601191342CEEE43887BDE71AB9772580EF973E4@IRSMSX105.ger.corp.intel.com>
 <1397747816-23245-1-git-send-email-nhorman@tuxdriver.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
To: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>, "dev-VfR2kkLFssw@public.gmane.org" <dev-VfR2kkLFssw@public.gmane.org>
Return-path: <dev-bounces-VfR2kkLFssw@public.gmane.org>
In-Reply-To: <1397747816-23245-1-git-send-email-nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Content-Language: en-US
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev-VfR2kkLFssw@public.gmane.org>
List-Help: <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=subscribe>
Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org
Sender: "dev" <dev-bounces-VfR2kkLFssw@public.gmane.org>

> -----Original Message-----
> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Neil Horman
> Sent: Thursday, April 17, 2014 4:17 PM
> To: dev-VfR2kkLFssw@public.gmane.org
> Subject: [dpdk-dev] [PATCH 05/15 v3] ring: Convert to use of
> PMD_REGISTER_DRIVER and fix linking
>=20
> 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 libra=
ries.
> 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.
>=20
> 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 calle=
d
> internally to the dpdk core library.  To correct this I've removed those
> functions, and instead allowed them to be called indirectly at initalizat=
ion
> time using the vdev command line argument key
> nodeaction=3D<name>:<node>:<action>
> where action is one of ATTACH or CREATE.  I've tested out the functionali=
ty 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 ut=
ility
> 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 b=
reak
> existing applications, but added deprecation warnings to them so that app=
s
> can
> migrate off them.
>=20
> Signed-off-by: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
>=20
> ---
> Change notes
>=20
> v2) fixed DEPDIR specifcation, should depend on RING not PCAP
>=20
> v3) Cleaned up strcmp error checking, printfs, and other parsing issues a=
s
> pointed out by Konstantin Ananyev <konstantin.ananyev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  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(-)
>=20
> 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;
>=20
> -static struct rte_ring *nullring =3D NULL;
>  static struct rte_mempool *mp;
>  static uint8_t start_idx; /* will store the port id of the first of our =
new ports
> */
>=20
> @@ -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
>=20
> -
> -static int
> -test_ring_ethdev_create(void)
> -{
> -	int retval;
> -	printf("Testing ring pmd create\n");
> -
> -	retval =3D 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 =3D rte_eth_from_rings(NULL, 0, NULL, 0,
> RTE_MAX_NUMA_NODES);
> -	if (retval >=3D 0) {
> -		printf("Failure, can create ring pmd on socket %d\n",
> RTE_MAX_NUMA_NODES);
> -		return -1;
> -	}
> -
> -	retval =3D rte_eth_from_rings(NULL, 1, &r2, 1, SOCKET0);
> -	if (retval >=3D 0) {
> -		printf("Failure, can create pmd with null rx rings\n");
> -		return -1;
> -	}
> -
> -	retval =3D rte_eth_from_rings(r1, 1, NULL, 1, SOCKET0);
> -	if (retval >=3D 0) {
> -		printf("Failure, can create pmd with null tx rings\n");
> -		return -1;
> -	}
> -
> -	retval =3D 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 =3D 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 =3D 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 =3D "R3";
> -	const char * name2 =3D "R4";
> -	const char * params_null =3D NULL;
> -	const char * params =3D "PARAMS";
>  	struct rte_eth_stats stats;
>  	struct rte_mbuf buf, *pbuf =3D &buf;
>  	struct rte_eth_conf null_conf;
>=20
>  	printf("Testing ring pmd init\n");
>=20
> -	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 >=3D RTE_MAX_ETHPORTS) {
>  		printf(" TX/RX port exceed max eth ports\n");
>  		return -1;
> @@ -371,24 +304,16 @@ test_pmd_ring_init(void)
>=20
>  	rte_eth_dev_stop(RXTX_PORT2);
>=20
> -	/* Test init same name pmd ring */
> -	rte_pmd_ring_devinit(name1, params_null);
>  	return 0;
>  }
>=20
>  static int
>  test_pmd_ring_pair_create(void)
>  {
> -	const char * name1 =3D "_RNG_P0";
>  	struct rte_eth_stats stats, stats2;
>  	struct rte_mbuf buf, *pbuf =3D &buf;
>  	struct rte_eth_conf null_conf;
>=20
> -	if (rte_eth_ring_pair_create(name1, SOCKET0) < 0) {
> -		printf("Create ring pair failed\n");
> -		return -1;
> -	}
> -
>  	if ((RXTX_PORT4 >=3D RTE_MAX_ETHPORTS) || (RXTX_PORT5 >=3D
> 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);
>=20
> -	/* Test create same name ring pair */
> -	if (rte_eth_ring_pair_create(name1, SOCKET0) =3D=3D 0) {
> -		printf("Create same name ring pair error\n");
> -		return -1;
> -	}
>  	return 0;
>  }
>=20
>  static int
>  test_pmd_ring_pair_attach(void)
>  {
> -	const char * name1 =3D "_RNG_P0";
> -	const char * name2 =3D "_RNG_P1";
>  	struct rte_eth_stats stats, stats2;
>  	struct rte_mbuf buf, *pbuf =3D &buf;
>  	struct rte_eth_conf null_conf;
>=20
> -	if (rte_eth_ring_pair_attach(name1, SOCKET0) < 0) {
> -		printf("Attach ring pair failed\n");
> -		return -1;
> -	}
> -
>  	if ((RXTX_PORT4 >=3D RTE_MAX_ETHPORTS) || (RXTX_PORT5 >=3D
> 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);
>=20
> -	/* Test attach non-existing ring pair */
> -	if (rte_eth_ring_pair_attach(name2, SOCKET0) =3D=3D 0) {
> -		printf("Attach non-existing ring pair error\n");
> -		return -1;
> -	}
>  	return 0;
>  }
>=20
> @@ -568,9 +476,6 @@ test_pmd_ring(void)
>  		return -1;
>  	}
>=20
> -	if (test_ring_ethdev_create() < 0)
> -		return -1;
> -
>  	if (test_ethdev_configure() < 0)
>  		return -1;
>=20
> 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 +=3D rte_eth_ring.h
>  # this lib depends upon:
>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_RING) +=3D lib/librte_eal lib/librte_rin=
g
>  DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_RING) +=3D lib/librte_mbuf
> lib/librte_ether
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_RING) +=3D lib/librte_kvargs
>=20
>  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[] =3D {
> +	ETH_RING_NUMA_NODE_ACTION_ARG,
> +	NULL
> +};
>=20
>  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);
>  }
>=20
>  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);
>  }
>=20
> +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 =3D data;
> +	int ret;
> +	char *name;
> +	char *action;
> +	char *node;
> +	char *end;
> +
> +	name =3D strdup(value);
> +
> +	ret =3D -EINVAL;
> +
> +	if (!name) {
> +		RTE_LOG(WARNING, PMD, "command line paramter is empty
> for ring pmd!\n");
> +		goto out;
> +	}
> +
> +	node =3D strchr(name, ':');
> +	if (!node) {
> +		RTE_LOG(WARNING, PMD, "could not parse node value from
> %s", name);
> +		goto out;
> +	}
> +
> +	*node =3D '\0';
> +	node++;
> +
> +	action =3D strchr(node, ':');
> +	if (!action) {
> +		RTE_LOG(WARNING, PMD, "could not action value from %s",
> node);
> +		goto out;
> +	}
> +
> +	*action =3D '\0';
> +	action++;
> +
> +	/*
> +	 * Need to do some sanity checking here
> +	 */
> +
> +	if (strcmp(action, ETH_RING_ACTION_ATTACH) =3D=3D 0)
> +		info->list[info->count].action =3D DEV_ATTACH;
> +	else if (strcmp(action, ETH_RING_ACTION_CREATE) =3D=3D 0)
> +		info->list[info->count].action =3D DEV_CREATE;
> +	else
> +		goto out;
> +
> +	errno =3D 0;
> +	info->list[info->count].node =3D strtol(node, &end, 10);
> +
> +	if ((errno !=3D 0) || (*end !=3D '\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 =3D 0;
> +out:
> +	free(name);
> +	return ret;
> +}
> +
>  int
>  rte_pmd_ring_devinit(const char *name, const char *params)
>  {
> +	struct rte_kvargs *kvlist;
> +	int ret =3D 0;
> +	struct node_action_list *info =3D NULL;
> +
>  	RTE_LOG(INFO, PMD, "Initializing pmd_ring for %s\n", name);
>=20
> +
>  	if (params =3D=3D NULL || params[0] =3D=3D '\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 =3D 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 =3D rte_kvargs_count(kvlist,
> ETH_RING_NUMA_NODE_ACTION_ARG);
> +			info =3D rte_zmalloc("struct node_action_list",
> sizeof(struct node_action_list) +
> +					   (sizeof(struct node_action_pair) *
> ret), 0);
> +			if (!info)
> +				goto out;
> +
> +			info->total =3D ret;
> +			info->list =3D (struct node_action_pair*)(info + 1);
> +
> +			ret =3D rte_kvargs_process(kvlist,
> ETH_RING_NUMA_NODE_ACTION_ARG,
> +						 parse_kvlist, info);
> +
> +			if (ret < 0)
> +				goto out_free;
> +
> +			for (info->count =3D 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;
>  }
>=20
>  static struct rte_vdev_driver pmd_ring_drv =3D {
> @@ -402,9 +528,4 @@ static struct rte_vdev_driver pmd_ring_drv =3D {
>  	.init =3D rte_pmd_ring_devinit,
>  };
>=20
> -__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 +=3D -lethdev
>  endif
>=20
> -ifeq ($(CONFIG_RTE_LIBRTE_PMD_RING),y)
> -LDLIBS +=3D -lrte_pmd_ring
> -endif
> -
>  ifeq ($(CONFIG_RTE_LIBRTE_MALLOC),y)
>  LDLIBS +=3D -lrte_malloc
>  endif
> @@ -173,9 +169,19 @@ LDLIBS +=3D -lrte_cmdline
>  endif
>=20
>  ifeq ($(RTE_BUILD_SHARED_LIB),n)
> +
> +ifeq ($(CONFIG_RTE_LIBRTE_PMD_RING),y)
> +LDLIBS +=3D -lrte_pmd_ring
> +endif
> +
> +ifeq ($(CONFIG_RTE_LIBRTE_PMD_RING),y)
> +LDLIBS +=3D -lrte_pmd_ring
> +endif
> +
>  ifeq ($(CONFIG_RTE_LIBRTE_PMD_PCAP),y)
>  LDLIBS +=3D -lrte_pmd_pcap -lpcap
>  endif
> +
>  endif
>=20
>  LDLIBS +=3D $(EXECENV_LDLIBS)
> --
> 1.8.3.1

Hi Neil,

Sorry for opening this thread again, but I have encountered that the ring P=
MD 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, pleas=
e?

Thanks,
Pablo