From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f41.google.com (mail-wm0-f41.google.com [74.125.82.41]) by dpdk.org (Postfix) with ESMTP id C46FE374 for ; Fri, 30 Jun 2017 16:12:57 +0200 (CEST) Received: by mail-wm0-f41.google.com with SMTP id b184so47382934wme.1 for ; Fri, 30 Jun 2017 07:12:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=z5ljuJ8JG9/qEe4QemvY+vdnnrNYsNHbksXMZ73HJRA=; b=u6JJjRcEaajIJYY/smpe6hGLDWqPGaioK5dWZx2S1lT6Cz21uf3ePhbYk4xTgqYW3z /a7wfjMAK/Y4HkXKtY6nPFLszJOdUP1qg8p3BTt8qk6pAqysY9sJjbr5fWuX57wgzMK6 n7+/fDlVmbxoUUjVWnpjZMx+UCZAE/ZO+rZikz+gF/MPGPlmpwRAOBXAWmu1VdVvZkii p8ygQNlHIlKripVgKzK/Ke/sALAR8KsG3fXZIffNx2SlprFL440UhODzL5EzM5kV693m gAQCRKRJL131xX4WehVF403qOeP/0a2sxeVJ3KWLcCEs1GcLzrX2sHqAX7uLFq9cMCoR j1Iw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=z5ljuJ8JG9/qEe4QemvY+vdnnrNYsNHbksXMZ73HJRA=; b=f072jFwZfy2M+qWyaUQ7YpxSUE9w4gly+XFXa0ES8KvLGE3cfH3SdirXaa4G+HNhJf kZkacF7N1atrkRFwCW16qxp/i3RPnjfZoVlluRa3EiqpL3nUiQ31nEpl6LGzb4xFzbke HCWSIXujTaOXTs/6IIIcQv3i3R8UfuWr9gHfmcA2VqSnbAk423Je8uVKl2QxwcT5w5Q1 9f42kK8CnFKKEIMJFDg5mjTEwWRa26crT93qKAvPxKWcB9UWCOimx27hxmzB49896iSa 39wbXTSjkOhdXdDONeyjZKqCezht5gRfr1k0hRdC/S7+OoiNf+dQCWf8t+YOxkfGz8es 1KGA== X-Gm-Message-State: AIVw110cdH8UFASfGwpbdYI8w9fVrjAMJ8/ijexzTOHg2e7hZCCFHy5K k6j0vn+5QaXKIisM X-Received: by 10.28.17.11 with SMTP id 11mr2192332wmr.109.1498831977261; Fri, 30 Jun 2017 07:12:57 -0700 (PDT) Received: from platinum (2a01cb0c03c651000226b0fffeed02fc.ipv6.abo.wanadoo.fr. [2a01:cb0c:3c6:5100:226:b0ff:feed:2fc]) by smtp.gmail.com with ESMTPSA id m26sm9247580wrm.4.2017.06.30.07.12.56 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 30 Jun 2017 07:12:57 -0700 (PDT) Date: Fri, 30 Jun 2017 16:12:54 +0200 From: Olivier Matz To: Santosh Shukla Cc: dev@dpdk.org, hemant.agrawal@nxp.com, jerin.jacob@caviumnetworks.com Message-ID: <20170630161254.36730e2a@platinum> In-Reply-To: <20170601080559.10684-2-santosh.shukla@caviumnetworks.com> References: <20170601080559.10684-1-santosh.shukla@caviumnetworks.com> <20170601080559.10684-2-santosh.shukla@caviumnetworks.com> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/2] eal: Introducing option to set mempool handle X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Jun 2017 14:12:58 -0000 Hi Santosh, On Thu, 1 Jun 2017 13:35:58 +0530, Santosh Shukla wrote: > Platform can have external PCI cards like Intel 40G card and > Integrated NIC like OcteoTX. Where both NIC has their > preferred pool handle. Example: Intel 40G NIC preferred pool > is ring_mp_mc and OcteonTX preferred pool handle would be > ext-mempool's handle named 'octeontx-fpavf'. > > There is no way that either of NIC's could use their choice > of mempool handle. > > Because Currently mempool handle programmed in static way i.e. > User has to set pool handle name in CONFIG_RTE_MEMPOOL_OPS_DEFAULT='' > > So introducing eal option --pkt-mempool="". > > Signed-off-by: Santosh Shukla > --- > lib/librte_eal/bsdapp/eal/eal.c | 9 +++++++ > lib/librte_eal/bsdapp/eal/rte_eal_version.map | 7 +++++ > lib/librte_eal/common/eal_common_options.c | 3 +++ > lib/librte_eal/common/eal_internal_cfg.h | 2 ++ > lib/librte_eal/common/eal_options.h | 2 ++ > lib/librte_eal/common/include/rte_eal.h | 9 +++++++ > lib/librte_eal/linuxapp/eal/eal.c | 36 +++++++++++++++++++++++++ > lib/librte_eal/linuxapp/eal/rte_eal_version.map | 7 +++++ > lib/librte_mbuf/rte_mbuf.c | 8 ++++-- > 9 files changed, 81 insertions(+), 2 deletions(-) > > diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c > index 05f0c1f90..7d8824707 100644 > --- a/lib/librte_eal/bsdapp/eal/eal.c > +++ b/lib/librte_eal/bsdapp/eal/eal.c > @@ -113,6 +113,15 @@ struct internal_config internal_config; > /* used by rte_rdtsc() */ > int rte_cycles_vmware_tsc_map; > > +char *__rte_unused why __rte_unused? I'll also add a const > +rte_eal_get_mp_name(void) I suggest rte_eal_mbuf_default_mempool_ops() It's longer but it shows it's only about mbufs, and it is consistent with the #define. > +{ > + if (internal_config.mp_name[0] == 0x0) > + return NULL; > + else > + return internal_config.mp_name; > +} > + Would returning RTE_MBUF_DEFAULT_MEMPOOL_OPS instead of NULL make sense? > /* Return a pointer to the configuration structure */ > struct rte_config * > rte_eal_get_configuration(void) > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map > index 2e48a7366..a1e9ad95f 100644 > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map > @@ -193,3 +193,10 @@ DPDK_17.05 { > vfio_get_group_no; > > } DPDK_17.02; > + > +DPDK_17.08 { > + global: > + > + rte_eal_get_mp_name; > + > +} DPDK_17.05; > diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c > index f470195f3..1c147a696 100644 > --- a/lib/librte_eal/common/eal_common_options.c > +++ b/lib/librte_eal/common/eal_common_options.c > @@ -95,6 +95,7 @@ eal_long_options[] = { > {OPT_VFIO_INTR, 1, NULL, OPT_VFIO_INTR_NUM }, > {OPT_VMWARE_TSC_MAP, 0, NULL, OPT_VMWARE_TSC_MAP_NUM }, > {OPT_XEN_DOM0, 0, NULL, OPT_XEN_DOM0_NUM }, > + {OPT_PKT_MEMPOOL, 1, NULL, OPT_PKT_MEMPOOL_NUM }, > {0, 0, NULL, 0 } > }; > > @@ -161,6 +162,7 @@ eal_reset_internal_config(struct internal_config *internal_cfg) > #endif > internal_cfg->vmware_tsc_map = 0; > internal_cfg->create_uio_dev = 0; > + memset(&internal_cfg->mp_name[0], 0x0, MAX_POOL_NAME_LEN); > } > > static int Or it could be initialized to RTE_MBUF_DEFAULT_MEMPOOL_OPS here? > @@ -1083,5 +1085,6 @@ eal_common_usage(void) > " --"OPT_NO_PCI" Disable PCI\n" > " --"OPT_NO_HPET" Disable HPET\n" > " --"OPT_NO_SHCONF" No shared config (mmap'd files)\n" > + " --"OPT_PKT_MEMPOOL" Use pool name as mempool for port\n" > "\n", RTE_MAX_LCORE); > } > diff --git a/lib/librte_eal/common/eal_internal_cfg.h b/lib/librte_eal/common/eal_internal_cfg.h > index 7b7e8c887..b8fedd2e6 100644 > --- a/lib/librte_eal/common/eal_internal_cfg.h > +++ b/lib/librte_eal/common/eal_internal_cfg.h > @@ -43,6 +43,7 @@ > #include > > #define MAX_HUGEPAGE_SIZES 3 /**< support up to 3 page sizes */ > +#define MAX_POOL_NAME_LEN 256 /**< Max len of a pool name */ > > /* > * internal configuration structure for the number, size and Shouldn't we have the same length than RTE_MEMPOOL_OPS_NAMESIZE? I think we should try to avoid introducing new defines without RTE_ prefix. > @@ -84,6 +85,7 @@ struct internal_config { > const char *hugepage_dir; /**< specific hugetlbfs directory to use */ > > unsigned num_hugepage_sizes; /**< how many sizes on this system */ > + char mp_name[MAX_POOL_NAME_LEN]; /**< mempool handle name */ > struct hugepage_info hugepage_info[MAX_HUGEPAGE_SIZES]; > }; If it's in internal config, I think we can use a char pointer instead of a table. What is the expected behavior in case of multiprocess? > extern struct internal_config internal_config; /**< Global EAL configuration. */ > diff --git a/lib/librte_eal/common/eal_options.h b/lib/librte_eal/common/eal_options.h > index a881c62e2..4e52ee255 100644 > --- a/lib/librte_eal/common/eal_options.h > +++ b/lib/librte_eal/common/eal_options.h > @@ -83,6 +83,8 @@ enum { > OPT_VMWARE_TSC_MAP_NUM, > #define OPT_XEN_DOM0 "xen-dom0" > OPT_XEN_DOM0_NUM, > +#define OPT_PKT_MEMPOOL "pkt-mempool" > + OPT_PKT_MEMPOOL_NUM, > OPT_LONG_MAX_NUM > }; > While "pkt-mempool" is probably ok, here are some other suggestions, in case you feel it's clearer: pkt-pool-ops mbuf-pool-ops mbuf-default-mempool-ops (too long, but consistent with #define and api) > diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h > index abf020bf9..c2f696a3d 100644 > --- a/lib/librte_eal/common/include/rte_eal.h > +++ b/lib/librte_eal/common/include/rte_eal.h > @@ -283,6 +283,15 @@ static inline int rte_gettid(void) > return RTE_PER_LCORE(_thread_id); > } > > +/** > + * Get mempool name from cmdline. > + * > + * @return > + * On success, returns the pool name. > + * On Failure, returs NULL. > + */ > +char *rte_eal_get_mp_name(void); > + > #define RTE_INIT(func) \ > static void __attribute__((constructor, used)) func(void) > > diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c > index 7c78f2dc2..b2a6c8068 100644 > --- a/lib/librte_eal/linuxapp/eal/eal.c > +++ b/lib/librte_eal/linuxapp/eal/eal.c > @@ -122,6 +122,15 @@ struct internal_config internal_config; > /* used by rte_rdtsc() */ > int rte_cycles_vmware_tsc_map; > > +char * > +rte_eal_get_mp_name(void) > +{ > + if (internal_config.mp_name[0] == 0x0) > + return NULL; > + else > + return internal_config.mp_name; > +} > + > /* Return a pointer to the configuration structure */ > struct rte_config * > rte_eal_get_configuration(void) > @@ -478,6 +487,23 @@ eal_parse_vfio_intr(const char *mode) > return -1; > } > > +static int > +eal_parse_mp_name(const char *name) > +{ > + int len; > + > + if (name == NULL) > + return -1; > + > + len = strlen(name); > + if (len >= MAX_POOL_NAME_LEN) > + return -1; > + > + strcpy(internal_config.mp_name, name); > + > + return 0; > +} > + Why is it linuxapp only? Using strcpy() is not a good habbit, I suggest to use snprintf instead. Also, prefer to use sizeof() instead of using the #define size. ret = snprintf(internal_config.mp_name, sizeof(internal_config.mp_name), "%s", name); if (ret < 0 || ret >= sizeof(internal_config.mp_name)) return -1; return 0; > /* Parse the arguments for --log-level only */ > static void > eal_log_level_parse(int argc, char **argv) > @@ -611,6 +637,16 @@ eal_parse_args(int argc, char **argv) > internal_config.create_uio_dev = 1; > break; > > + case OPT_PKT_MEMPOOL_NUM: > + if (eal_parse_mp_name(optarg) < 0) { > + RTE_LOG(ERR, EAL, "invalid parameters for --" > + OPT_PKT_MEMPOOL "\n"); > + eal_usage(prgname); > + ret = -1; > + goto out; > + } > + break; > + > default: > if (opt < OPT_LONG_MIN_NUM && isprint(opt)) { > RTE_LOG(ERR, EAL, "Option %c is not supported " > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map > index 670bab3a5..e57330bec 100644 > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map > @@ -198,3 +198,10 @@ DPDK_17.05 { > vfio_get_group_no; > > } DPDK_17.02; > + > +DPDK_17.08 { > + global: > + > + rte_eal_get_mp_name; > + > +} DPDK_17.05 > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c > index 0e3e36a58..38f4b3de0 100644 > --- a/lib/librte_mbuf/rte_mbuf.c > +++ b/lib/librte_mbuf/rte_mbuf.c > @@ -158,6 +158,7 @@ rte_pktmbuf_pool_create(const char *name, unsigned n, > { > struct rte_mempool *mp; > struct rte_pktmbuf_pool_private mbp_priv; > + const char *mp_name = NULL; > unsigned elt_size; > int ret; > > @@ -177,8 +178,11 @@ rte_pktmbuf_pool_create(const char *name, unsigned n, > if (mp == NULL) > return NULL; > > - ret = rte_mempool_set_ops_byname(mp, > - RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL); > + mp_name = rte_eal_get_mp_name(); > + if (mp_name == NULL) > + mp_name = RTE_MBUF_DEFAULT_MEMPOOL_OPS; > + > + ret = rte_mempool_set_ops_byname(mp, mp_name, NULL); > if (ret != 0) { > RTE_LOG(ERR, MBUF, "error setting mempool handler\n"); > rte_mempool_free(mp); If NULL is never returned, the code can be simplified a bit. Olivier