Re: [PATCH 08/10] event/octeontx: add option to use fpavf as chunk pool

Hi Santosh,

On Sun, Feb 18, 2018 at 05:12:33PM +0530, santosh wrote:

Hi Pavan,


On Saturday 17 February 2018 03:06 AM, Pavan Nikhilesh wrote:

quoted

Add compile-time configurable option to force TIMvf to use Octeontx
FPAvf pool manager as its chunk pool.
When FPAvf is used as pool manager the TIMvf automatically frees the
chunks to FPAvf through gpool-id.

Signed-off-by: Pavan Nikhilesh <redacted>
---
 config/common_base                    |  1 +
 drivers/event/octeontx/timvf_evdev.c  | 23 +++++++++++++++++++++++
 drivers/event/octeontx/timvf_evdev.h  |  3 +++
 drivers/event/octeontx/timvf_worker.h | 35 +++++++++++++++++++++++++++++++++++
 4 files changed, 62 insertions(+)

diff --git a/config/common_base b/config/common_base
index ad03cf433..00010de92 100644
--- a/config/common_base
+++ b/config/common_base

@@ -562,6 +562,7 @@ CONFIG_RTE_LIBRTE_PMD_SW_EVENTDEV=y
 # Compile PMD for octeontx sso event device
 #
 CONFIG_RTE_LIBRTE_PMD_OCTEONTX_SSOVF=y
+CONFIG_RTE_PMD_OCTEONTX_TIMVF_USE_FPAVF=n

How about using CONFIG_RTE_LIBRTE_OCTEONTX_MEMPOOL?

I think CONFIG_RTE_LIBRTE_OCTEONTX_MEMPOOL is always enabled by default,using
something like CONFIG_RTE_LIBRTE_TIMVF_USE_OCTEONTX_MEMPOOL be better? i.e
telling TIMvf to specifically use fpa.

quoted

 #
 # Compile PMD for OPDL event device

diff --git a/drivers/event/octeontx/timvf_evdev.c b/drivers/event/octeontx/timvf_evdev.c
index ffdfbb387..386eaa08f 100644
--- a/drivers/event/octeontx/timvf_evdev.c
+++ b/drivers/event/octeontx/timvf_evdev.c

@@ -162,10 +162,27 @@ timvf_ring_start(const struct rte_event_timer_adapter *adptr)
 		1ull << 48 |
 		1ull << 47 |
 		1ull << 44 |
+#ifndef RTE_PMD_OCTEONTX_TIMVF_USE_FPAVF
+		1ull << 43 |
+#endif
 		(timr->meta.nb_bkts - 1);

 	rctrl.rctrl2 = (uint64_t)(TIM_CHUNK_SIZE / 16) << 40;

+#ifdef RTE_PMD_OCTEONTX_TIMVF_USE_FPAVF
+	uintptr_t pool;
+	pool = (uintptr_t)((struct rte_mempool *)
+			timr->meta.chunk_pool)->pool_id;
+	ret = octeontx_fpa_bufpool_gpool(pool);
+	if (ret < 0) {
+		timvf_log_dbg("Unable to get gaura id");
+		ret = -ENOMEM;
+		goto error;
+	}
+	timvf_write64((uint64_t)ret,
+			(uint8_t *)timr->vbar0 + TIM_VRING_AURA);
+#endif
+
 	timvf_write64((uint64_t)timr->meta.bkt,
 			(uint8_t *)timr->vbar0 + TIM_VRING_BASE);
 	if (timvf_ring_conf_set(&rctrl, timr->tim_ring_id)) {

@@ -296,9 +313,15 @@ timvf_ring_create(struct rte_event_timer_adapter *adptr)
 		return -ENOMEM;
 	}

+#ifdef RTE_PMD_OCTEONTX_TIMVF_USE_FPAVF
+	ret = rte_mempool_set_ops_byname(timr->meta.chunk_pool,
+			"octeontx_fpavf", NULL);
+	timvf_log_dbg("Giving back chunks to fpa gaura : %d", ret);
+#else
 	ret = rte_mempool_set_ops_byname(timr->meta.chunk_pool,
 			RTE_MBUF_DEFAULT_MEMPOOL_OPS, NULL);
 	timvf_log_dbg("Not giving back chunks to fpa");
+#endif

May be use rte_mbuf_best_mempool_ops Or rte_mbuf_user_mempool_ops,
that way avoid above ifdef.

In timvf_worker.h the logic used to arm the event timer changes with the
mempool used i.e. octeontx_fpavf has a different implementation vs ring.
We could use rte_mbuf_best_mempool_ops() but again need check if the
returned ops is fpa to enable automatic buffer recycling.

Also a suggestion, Try to reduce ifdef by creating a new header file
called timvf_fpa_evdev.h, abstract all possible ifdefs their,
create small static inline API and call them in timvf eventdev driver.
[...]

Agreed, will modify arm logic to reduce ifdef clutter.

Thanks.

Thanks,
Pavan.