| Message ID | 1516281992-6873-4-git-send-email-hemant.agrawal@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | Dynamic HW Mempool Detection Support | expand |
On Thu, Jan 18, 2018 at 06:56:28PM +0530, Hemant Agrawal wrote: > This patch add support for various mempool ops config helper APIs. > > 1.User defined mempool ops > 2.Platform detected HW mempool ops (active). > 3.Best selection of mempool ops by looking into user defined, > platform registered and compile time configured. > > Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com> > --- ... > --- /dev/null > +++ b/lib/librte_mbuf/rte_mbuf_pool_ops.c > @@ -0,0 +1,68 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright 2018 NXP > + */ > + > +#include <string.h> > +#include <rte_eal.h> > +#include <rte_mbuf.h> > +#include <rte_errno.h> > +#include <rte_mbuf_pool_ops.h> > +#include <rte_malloc.h> > + > +static char *plat_mbuf_pool_ops_name; I have some doubts about secondary processes. Maybe it's ok if the loaded driver and eal arguments are exactly the same in the secondary process. It would be safer to use a named memzone for that. It would be even safer to not use secondary processes ;) > + > +int > +rte_mbuf_register_platform_mempool_ops(const char *ops_name) > +{ We have "register" for platform and "set" for user. I think "set" should be used everywhere. > + if (plat_mbuf_pool_ops_name == NULL) { > + plat_mbuf_pool_ops_name = > + rte_malloc(NULL, RTE_MEMPOOL_OPS_NAMESIZE, 0); > + if (plat_mbuf_pool_ops_name == NULL) > + return -ENOMEM; > + strcpy((char *)plat_mbuf_pool_ops_name, ops_name); If strlen(ops_name) >= RTE_MEMPOOL_OPS_NAMESIZE, this may lead to bad behavior. I suggest to simply do a strdup() instead. > + return 0; > + } else if (strcmp(plat_mbuf_pool_ops_name, ops_name) == 0) { > + return 0; > + } > + > + RTE_LOG(ERR, MBUF, > + "%s is already registered as platform mbuf pool ops\n", > + plat_mbuf_pool_ops_name); So, this log means that a we should try to never have 2 drivers registering different platform drivers on the same machine, right? So this API is kind of reserved for network processors and should not be used in usual PCI PMDs? > + return -EEXIST; > +} > + > +const char * > +rte_mbuf_platform_mempool_ops(void) > +{ > + return (const char *)plat_mbuf_pool_ops_name; cast is not required > +} > + > +void > +rte_mbuf_set_user_mempool_ops(const char *ops_name) > +{ > + rte_eal_set_mbuf_user_mempool_ops(ops_name); > +} Instead of calling the EAL API, we can set a static variable as for platform ops. > + > +const char * > +rte_mbuf_user_mempool_ops(void) > +{ > + return rte_eal_mbuf_default_mempool_ops(); > +} And here, I suggest instead: rte_mbuf_user_mempool_ops(void) { if (user_mbuf_pool_ops_name != NULL) return user_mbuf_pool_ops_name; return rte_eal_mbuf_default_mempool_ops(); } i.e. rte_eal_mbuf_default_mempool_ops() remains the ops passed as command line arguments. > + > +/* Return mbuf pool ops name */ > +const char * > +rte_mbuf_best_mempool_ops(void) > +{ > + /* User defined mempool ops takes the priority */ > + const char *best_ops = rte_mbuf_user_mempool_ops(); > + if (best_ops) > + return best_ops; > + > + /* Next choice is platform configured mempool ops */ > + best_ops = rte_mbuf_platform_mempool_ops(); > + if (best_ops) > + return best_ops; > + > + /* Last choice is to use the compile time config pool */ > + return RTE_MBUF_DEFAULT_MEMPOOL_OPS; > +} I like this function, this is much clearer than what we have today :)
Hi Olivier, On 1/19/2018 3:31 PM, Olivier Matz wrote: > On Thu, Jan 18, 2018 at 06:56:28PM +0530, Hemant Agrawal wrote: >> This patch add support for various mempool ops config helper APIs. >> >> 1.User defined mempool ops >> 2.Platform detected HW mempool ops (active). >> 3.Best selection of mempool ops by looking into user defined, >> platform registered and compile time configured. >> >> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com> >> --- > > ... > >> --- /dev/null >> +++ b/lib/librte_mbuf/rte_mbuf_pool_ops.c >> @@ -0,0 +1,68 @@ >> +/* SPDX-License-Identifier: BSD-3-Clause >> + * Copyright 2018 NXP >> + */ >> + >> +#include <string.h> >> +#include <rte_eal.h> >> +#include <rte_mbuf.h> >> +#include <rte_errno.h> >> +#include <rte_mbuf_pool_ops.h> >> +#include <rte_malloc.h> >> + >> +static char *plat_mbuf_pool_ops_name; > > I have some doubts about secondary processes. > > Maybe it's ok if the loaded driver and eal arguments are exactly the > same in the secondary process. It would be safer to use a named memzone > for that. > Typically a secondary process should not set the platform mempool name. I can also add a check to know if secondary process is trying to do it. Yes. I can change it to a named memzone. > It would be even safer to not use secondary processes ;) > > >> + >> +int >> +rte_mbuf_register_platform_mempool_ops(const char *ops_name) >> +{ > > We have "register" for platform and "set" for user. > I think "set" should be used everywhere. > ok >> + if (plat_mbuf_pool_ops_name == NULL) { >> + plat_mbuf_pool_ops_name = >> + rte_malloc(NULL, RTE_MEMPOOL_OPS_NAMESIZE, 0); >> + if (plat_mbuf_pool_ops_name == NULL) >> + return -ENOMEM; >> + strcpy((char *)plat_mbuf_pool_ops_name, ops_name); > > If strlen(ops_name) >= RTE_MEMPOOL_OPS_NAMESIZE, this may lead to > bad behavior. > That should not happen, we can check that. > I suggest to simply do a strdup() instead. Well, strdup based string will not be readable/accessible from the secondary process? > > >> + return 0; >> + } else if (strcmp(plat_mbuf_pool_ops_name, ops_name) == 0) { >> + return 0; >> + } >> + >> + RTE_LOG(ERR, MBUF, >> + "%s is already registered as platform mbuf pool ops\n", >> + plat_mbuf_pool_ops_name); > > So, this log means that a we should try to never have 2 drivers registering > different platform drivers on the same machine, right? > > So this API is kind of reserved for network processors and should not be > used in usual PCI PMDs? > No, PCI PMDs can also use it. But only one registration allowed for now. As I mentioned in the cover letter: ~~~~~ This logic can be further extended with addition for following patch, which is still under discussion. The ethdev PMD capability exposed through existing rte_eth_dev_pool_ops_supported() to select the update the mempool ops with some "weight" based algorithm like: http://dpdk.org/dev/patchwork/patch/32245/ ~~~~~~ > >> + return -EEXIST; >> +} >> + >> +const char * >> +rte_mbuf_platform_mempool_ops(void) >> +{ >> + return (const char *)plat_mbuf_pool_ops_name; > > cast is not required > >> +} >> + >> +void >> +rte_mbuf_set_user_mempool_ops(const char *ops_name) >> +{ >> + rte_eal_set_mbuf_user_mempool_ops(ops_name); >> +} > > Instead of calling the EAL API, we can set a static variable as > for platform ops. > >> + >> +const char * >> +rte_mbuf_user_mempool_ops(void) >> +{ >> + return rte_eal_mbuf_default_mempool_ops(); >> +} > > And here, I suggest instead: > > rte_mbuf_user_mempool_ops(void) > { > if (user_mbuf_pool_ops_name != NULL) > return user_mbuf_pool_ops_name; > return rte_eal_mbuf_default_mempool_ops(); > } > > i.e. rte_eal_mbuf_default_mempool_ops() remains the ops passed as > command line arguments. > > >> + >> +/* Return mbuf pool ops name */ >> +const char * >> +rte_mbuf_best_mempool_ops(void) >> +{ >> + /* User defined mempool ops takes the priority */ >> + const char *best_ops = rte_mbuf_user_mempool_ops(); >> + if (best_ops) >> + return best_ops; >> + >> + /* Next choice is platform configured mempool ops */ >> + best_ops = rte_mbuf_platform_mempool_ops(); >> + if (best_ops) >> + return best_ops; >> + >> + /* Last choice is to use the compile time config pool */ >> + return RTE_MBUF_DEFAULT_MEMPOOL_OPS; >> +} > > I like this function, this is much clearer than what we have today :) >
Hi Hemant, On Fri, Jan 19, 2018 at 06:11:47PM +0530, Hemant Agrawal wrote: > Hi Olivier, > > On 1/19/2018 3:31 PM, Olivier Matz wrote: > > On Thu, Jan 18, 2018 at 06:56:28PM +0530, Hemant Agrawal wrote: > > > This patch add support for various mempool ops config helper APIs. > > > > > > 1.User defined mempool ops > > > 2.Platform detected HW mempool ops (active). > > > 3.Best selection of mempool ops by looking into user defined, > > > platform registered and compile time configured. > > > > > > Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com> > > > --- > > > > ... > > > > > --- /dev/null > > > +++ b/lib/librte_mbuf/rte_mbuf_pool_ops.c > > > @@ -0,0 +1,68 @@ > > > +/* SPDX-License-Identifier: BSD-3-Clause > > > + * Copyright 2018 NXP > > > + */ > > > + > > > +#include <string.h> > > > +#include <rte_eal.h> > > > +#include <rte_mbuf.h> > > > +#include <rte_errno.h> > > > +#include <rte_mbuf_pool_ops.h> > > > +#include <rte_malloc.h> > > > + > > > +static char *plat_mbuf_pool_ops_name; > > > > I have some doubts about secondary processes. > > > > Maybe it's ok if the loaded driver and eal arguments are exactly the > > same in the secondary process. It would be safer to use a named memzone > > for that. > > > Typically a secondary process should not set the platform mempool name. > I can also add a check to know if secondary process is trying to do it. > > Yes. I can change it to a named memzone. With a memzone, maybe there is even no need for a static variable. Something like this? set(): mz = rte_memzone_lookup("mbuf_platform_pool_ops"); if (mz == NULL) { mz = rte_memzone_reszerve("mbuf_platform_pool_ops", LEN); if (mz == NULL) return -rte_errno; /* << this even protects against a set() in a 2nd process */ } if (strlen(name) >= LEN) return -ENAMETOOLONG; strncpy(mz->addr, name, LEN); get(): mz = rte_memzone_lookup("mbuf_platform_pool_ops"); if (mz == NULL) return NULL; return mz->addr; > > > > It would be even safer to not use secondary processes ;) > > > > > > > + > > > +int > > > +rte_mbuf_register_platform_mempool_ops(const char *ops_name) > > > +{ > > > > We have "register" for platform and "set" for user. > > I think "set" should be used everywhere. > > > ok > > > > + if (plat_mbuf_pool_ops_name == NULL) { > > > + plat_mbuf_pool_ops_name = > > > + rte_malloc(NULL, RTE_MEMPOOL_OPS_NAMESIZE, 0); > > > + if (plat_mbuf_pool_ops_name == NULL) > > > + return -ENOMEM; > > > + strcpy((char *)plat_mbuf_pool_ops_name, ops_name); > > > > If strlen(ops_name) >= RTE_MEMPOOL_OPS_NAMESIZE, this may lead to > > bad behavior. > > > That should not happen, we can check that. > > > I suggest to simply do a strdup() instead. > > Well, strdup based string will not be readable/accessible from the secondary > process? Correct. something to check the length should be added though.
diff --git a/lib/librte_mbuf/Makefile b/lib/librte_mbuf/Makefile index 398f724..e2e3ec6 100644 --- a/lib/librte_mbuf/Makefile +++ b/lib/librte_mbuf/Makefile @@ -14,9 +14,9 @@ EXPORT_MAP := rte_mbuf_version.map LIBABIVER := 3 # all source are stored in SRCS-y -SRCS-$(CONFIG_RTE_LIBRTE_MBUF) := rte_mbuf.c rte_mbuf_ptype.c +SRCS-$(CONFIG_RTE_LIBRTE_MBUF) := rte_mbuf.c rte_mbuf_ptype.c rte_mbuf_pool_ops.c # install includes -SYMLINK-$(CONFIG_RTE_LIBRTE_MBUF)-include := rte_mbuf.h rte_mbuf_ptype.h +SYMLINK-$(CONFIG_RTE_LIBRTE_MBUF)-include := rte_mbuf.h rte_mbuf_ptype.h rte_mbuf_pool_ops.h include $(RTE_SDK)/mk/rte.lib.mk diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c index c085c37..0c4d374 100644 --- a/lib/librte_mbuf/rte_mbuf.c +++ b/lib/librte_mbuf/rte_mbuf.c @@ -54,6 +54,7 @@ #include <rte_branch_prediction.h> #include <rte_mempool.h> #include <rte_mbuf.h> +#include <rte_mbuf_pool_ops.h> #include <rte_string_fns.h> #include <rte_hexdump.h> #include <rte_errno.h> @@ -176,9 +177,7 @@ rte_pktmbuf_pool_create(const char *name, unsigned n, if (mp == NULL) return NULL; - mp_ops_name = rte_eal_mbuf_default_mempool_ops(); - if (mp_ops_name == NULL) - mp_ops_name = RTE_MBUF_DEFAULT_MEMPOOL_OPS; + mp_ops_name = rte_mbuf_best_mempool_ops(); ret = rte_mempool_set_ops_byname(mp, mp_ops_name, NULL); if (ret != 0) { RTE_LOG(ERR, MBUF, "error setting mempool handler\n"); diff --git a/lib/librte_mbuf/rte_mbuf_pool_ops.c b/lib/librte_mbuf/rte_mbuf_pool_ops.c new file mode 100644 index 0000000..8e3a053 --- /dev/null +++ b/lib/librte_mbuf/rte_mbuf_pool_ops.c @@ -0,0 +1,68 @@ +/* SPDX-License-Identifier: BSD-3-Clause + * Copyright 2018 NXP + */ + +#include <string.h> +#include <rte_eal.h> +#include <rte_mbuf.h> +#include <rte_errno.h> +#include <rte_mbuf_pool_ops.h> +#include <rte_malloc.h> + +static char *plat_mbuf_pool_ops_name; + +int +rte_mbuf_register_platform_mempool_ops(const char *ops_name) +{ + if (plat_mbuf_pool_ops_name == NULL) { + plat_mbuf_pool_ops_name = + rte_malloc(NULL, RTE_MEMPOOL_OPS_NAMESIZE, 0); + if (plat_mbuf_pool_ops_name == NULL) + return -ENOMEM; + strcpy((char *)plat_mbuf_pool_ops_name, ops_name); + return 0; + } else if (strcmp(plat_mbuf_pool_ops_name, ops_name) == 0) { + return 0; + } + + RTE_LOG(ERR, MBUF, + "%s is already registered as platform mbuf pool ops\n", + plat_mbuf_pool_ops_name); + return -EEXIST; +} + +const char * +rte_mbuf_platform_mempool_ops(void) +{ + return (const char *)plat_mbuf_pool_ops_name; +} + +void +rte_mbuf_set_user_mempool_ops(const char *ops_name) +{ + rte_eal_set_mbuf_user_mempool_ops(ops_name); +} + +const char * +rte_mbuf_user_mempool_ops(void) +{ + return rte_eal_mbuf_default_mempool_ops(); +} + +/* Return mbuf pool ops name */ +const char * +rte_mbuf_best_mempool_ops(void) +{ + /* User defined mempool ops takes the priority */ + const char *best_ops = rte_mbuf_user_mempool_ops(); + if (best_ops) + return best_ops; + + /* Next choice is platform configured mempool ops */ + best_ops = rte_mbuf_platform_mempool_ops(); + if (best_ops) + return best_ops; + + /* Last choice is to use the compile time config pool */ + return RTE_MBUF_DEFAULT_MEMPOOL_OPS; +} diff --git a/lib/librte_mbuf/rte_mbuf_pool_ops.h b/lib/librte_mbuf/rte_mbuf_pool_ops.h new file mode 100644 index 0000000..8f6db54 --- /dev/null +++ b/lib/librte_mbuf/rte_mbuf_pool_ops.h @@ -0,0 +1,87 @@ +/* SPDX-License-Identifier: BSD-3-Clause + * Copyright 2018 NXP + */ + +#ifndef _RTE_MBUF_POOL_OPS_H_ +#define _RTE_MBUF_POOL_OPS_H_ + +/** + * @file + * RTE Mbuf Pool Ops + * + * These APIs are for configuring the mbuf pool ops names to be largely used by + * rte_pktmbuf_pool_create(). However, this can also be used to set and inquire + * the best mempool ops available. + */ + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * Register the platform supported pktmbuf HW mempool ops name + * + * This function allow the HW to register the actively supported HW mempool + * ops_name. Only one HW mempool ops can be registered at any point of time. + * + * @param pool ops name + * @return + * - 0: Success + * - -EEXIST: platform mempool is already registered. + * - -ENOMEM: no mempory to save ops name. + */ +int +rte_mbuf_register_platform_mempool_ops(const char *ops_name); + +/** + * Register the user preferred pktmbuf mempool ops name + * + * This function can be used by the user to configure user preferred + * mempool ops name. + * + * @param pool ops name + */ +void +rte_mbuf_set_user_mempool_ops(const char *ops_name); + +/** + * Get the best mempool ops name for pktmbuf. + * + * This function is used to determine the best options for mempool ops for + * pktmbuf allocations. Following are the priority order: + * 1. User defined, 2. Platform HW supported, 3. Compile time configured. + * This function is also used by the rte_pktmbuf_pool_create to get the best + * mempool ops name. + * + * @param pool ops name + */ +const char * +rte_mbuf_best_mempool_ops(void); + +/** + * Get registered platform supported pool ops name for mbuf + * + * This function returns the platform supported mempool ops name. + * + * @return + * returns platform pool ops name. + */ +const char * +rte_mbuf_platform_mempool_ops(void); + +/** + * Get user preferred pool ops name for mbuf + * + * This function returns the user configured mempool ops name. + * + * @return + * returns user pool ops name. + */ +const char * +rte_mbuf_user_mempool_ops(void); + +#ifdef __cplusplus +} +#endif + +#endif /* _RTE_MBUF_POOL_OPS_H_ */ diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map index 6e2ea84..3d60046 100644 --- a/lib/librte_mbuf/rte_mbuf_version.map +++ b/lib/librte_mbuf/rte_mbuf_version.map @@ -35,3 +35,14 @@ DPDK_16.11 { rte_get_tx_ol_flag_list; } DPDK_2.1; + +DPDK_18.02 { + global: + + rte_mbuf_best_mempool_ops; + rte_mbuf_platform_mempool_ops; + rte_mbuf_register_platform_mempool_ops; + rte_mbuf_set_user_mempool_ops; + rte_mbuf_user_mempool_ops; + +} DPDK_16.11;
This patch add support for various mempool ops config helper APIs. 1.User defined mempool ops 2.Platform detected HW mempool ops (active). 3.Best selection of mempool ops by looking into user defined, platform registered and compile time configured. Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com> --- lib/librte_mbuf/Makefile | 4 +- lib/librte_mbuf/rte_mbuf.c | 5 +-- lib/librte_mbuf/rte_mbuf_pool_ops.c | 68 ++++++++++++++++++++++++++++ lib/librte_mbuf/rte_mbuf_pool_ops.h | 87 ++++++++++++++++++++++++++++++++++++ lib/librte_mbuf/rte_mbuf_version.map | 11 +++++ 5 files changed, 170 insertions(+), 5 deletions(-) create mode 100644 lib/librte_mbuf/rte_mbuf_pool_ops.c create mode 100644 lib/librte_mbuf/rte_mbuf_pool_ops.h -- 2.7.4