[dpdk-dev] [PATCH v3 3/7] mbuf: add pool ops name selection API helpers

[Hemant Agrawal]

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 at 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 :)
>