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.agra...@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 :)
