> -----Original Message----- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Neil Horman > Sent: Thursday, April 17, 2014 4:17 PM > To: dev at 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 at 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 at 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
