From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id AA9A768B5 for ; Thu, 17 Apr 2014 11:51:43 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 17 Apr 2014 02:51:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,878,1389772800"; d="scan'208";a="522152913" Received: from irsmsx104.ger.corp.intel.com ([163.33.3.159]) by fmsmga002.fm.intel.com with ESMTP; 17 Apr 2014 02:51:40 -0700 Received: from irsmsx154.ger.corp.intel.com (163.33.192.96) by IRSMSX104.ger.corp.intel.com (163.33.3.159) with Microsoft SMTP Server (TLS) id 14.3.123.3; Thu, 17 Apr 2014 10:50:04 +0100 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.78]) by IRSMSX154.ger.corp.intel.com ([163.33.192.96]) with mapi id 14.03.0123.003; Thu, 17 Apr 2014 10:50:03 +0100 From: "Ananyev, Konstantin" To: Neil Horman , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 05/15] ring: Convert to use of PMD_REGISTER_DRIVER and fix linking Thread-Index: AQHPWNXVsR1M8j6TkE2YjdndnXnKfpsVkZJQ Date: Thu, 17 Apr 2014 09:50:02 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772580EF973E4@IRSMSX105.ger.corp.intel.com> References: <1397585169-14537-1-git-send-email-nhorman@tuxdriver.com> <1397585169-14537-6-git-send-email-nhorman@tuxdriver.com> In-Reply-To: <1397585169-14537-6-git-send-email-nhorman@tuxdriver.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 05/15] ring: Convert to use of PMD_REGISTER_DRIVER and fix linking X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Apr 2014 09:51:44 -0000 Hi Neil, Few comments from me there. Thanks Konstantin - parse_kvlist(): 1) node =3D strchr(name, ':'); ... action =3D strchr(node, ':'); We can't expect that input parameter will always be valid. So need to check that strchr() doesn't return NULL. 2) if (strcmp(action, "ATTACH")) if (strcmp(action, "CREATE")) goto out; ... info->list[info->count].action =3D strcmp(action, "ATTACH") ? DEV_CREATE : = DEV_ATTACH; Can you create a macros for these 2 string constants and use them instead. Another thing, probably better to reorder it that way: if (strcmp(action, "ATTACH") =3D=3D 0) info->list[info->count].action =3D DEV_ATTACH; else if (strcmp(action, "CREATE") =3D=3D 0) info->list[info->count].action =3D DEV_CREATE; else=20 goto out; Would save you one strcmp() and looks a bit cleaner. 3) info->list[info->count].node =3D strtol(node, NULL, 10); Again we can't assume that input string will always be valid. Something like that should do, I think: char *end; ... errno =3D 0; info->list[info->count].node =3D strtoul(node, &end, 10); if (errno !=3D 0 || *end !=3D 0) { ret =3D -EINVAL; goto out; } 4) strncpy(info->list[info->count].name, name, PATH_MAX); When RTE_INSECURE_FUNCTION_WARNING is defined, strncpy() (and some other f= unctions) are marked as poisoned. Another thing - as I remember, if strlen(name) >=3D PATH_MAX, then destinat= ion string will not be null terminated. So probably something like that instead: rte_snprintf(info->list[info->count].name, sizeof(info->list[info->count].n= ame), "%s", name); - rte_pmd_ring_devinit(): 5) printf("Parsing kvargs\n");=20 Here and everywhere - please use RTE_LOG() instead. -----Original Message----- From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman Sent: Tuesday, April 15, 2014 7:06 PM To: dev@dpdk.org Subject: [dpdk-dev] [PATCH 05/15] ring: Convert to use of PMD_REGISTER_DRIV= ER 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 librar= ies. This means that the test applications now have no reference to the ring lib= rary when building DSO's and must specify its use on the command line with = the -d option. Static linking will still initalize the driver automaticall= y. Note that the ring driver was also written in such a way that it violated s= ome general layering principles, several functions were contained in the pm= d which were being called by example from the test application in the app/t= est directory. Specifically it was calling eth_ring_pair_attach, eth_ring_= pair_create and rte_eth_ring_devinit, which should only be called internall= y to the dpdk core library. To correct this I've removed those functions, = and instead allowed them to be called indirectly at initalization time usin= g the vdev command line argument key nodeaction=3D:: wh= ere action is one of ATTACH or CREATE. I've tested out the functionality o= f 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 th= e 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 plac= e, 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 --- app/test/test_pmd_ring.c | 95 ---------------------------- lib/librte_pmd_ring/Makefile | 1 + lib/librte_pmd_ring/rte_eth_ring.c | 123 +++++++++++++++++++++++++++++++++= +--- mk/rte.app.mk | 14 +++-- 4 files changed, 124 insertions(+), 109 deletions(-) diff --git a/app/test/test_pmd_ring.c b/app/test/test_pmd_ring.c index 4d9c= 2ba..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 stru= ct 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 n= ew ports */ =20 @@ -59,58 +58,6 @@ static uint8_t start_idx; /* will store the port id of t= he 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_ETHPOR= TS)) { 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_ETHPOR= TS)) { 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); =09 - /* 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 in= dex 73b2d38..a62f805 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_ring DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_RING) +=3D lib/librte_mbuf lib/librte_ethe= r +DEPDIRS-$(CONFIG_RTE_LIBRTE_PMD_PCAP) +=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_e= th_ring.c index cee3fff..266cfc0 100644 --- a/lib/librte_pmd_ring/rte_eth_ring.c +++ b/lib/librte_pmd_ring/rte_eth_ring.c @@ -38,6 +38,15 @@ #include #include #include +#include +#include + +#define ETH_RING_NUMA_NODE_ACTION_ARG "nodeaction" + +static const char *valid_arguments[] =3D { + ETH_RING_NUMA_NODE_ACTION_ARG, + NULL +}; =20 struct ring_queue { struct rte_ring *rng; @@ -373,28 +382,127 @@ eth_dev_ring_pair_create(const char *name, const uns= igned numa_node, int rte_eth_ring_pair_create(const char *name, const uns= igned 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=20 +*value, void *data) { + struct node_action_list *info =3D data; + int ret; + char *name; + char *action; + char *node; + + name =3D strdup(value); + + ret =3D -1; + + if (!name) + goto out; + + node =3D strchr(name, ':'); + *node =3D '\0'; + node++; + + action =3D strchr(node, ':'); + *action =3D '\0'; + action++; + + /* + * Need to do some sanity checking here + */ + + if (strcmp(action, "ATTACH")) + if (strcmp(action, "CREATE")) + goto out; + + info->list[info->count].node =3D strtol(node, NULL, 10); + + info->list[info->count].action =3D strcmp(action, "ATTACH") ? DEV_CREATE= =20 +: DEV_ATTACH; + + strncpy(info->list[info->count].name, name, PATH_MAX); + + 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); + printf("Parsing kvargs\n"); + 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); + printf("counting kvargs\n"); + ret =3D rte_kvargs_count(kvlist, ETH_RING_NUMA_NODE_ACTION_ARG); + info =3D rte_zmalloc("struct node_action_list", sizeof(struct node_acti= on_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); + + printf("We have %d nodeaction pairs\n", info->total); + 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++) { + printf("Doing something with a ring\n"); + eth_dev_ring_pair_create(name, info->list[info->count].node, + info->list[info->count].action); + } + =09 + } } - return 0; + +out_free: + rte_free(info); +out: + return ret; } =20 static struct rte_vdev_driver pmd_ring_drv =3D { @@ -402,9 +510,4 @@ stati= c 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 -leth= dev 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