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

[Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>]

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:
> > 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 <pbhagavatula@caviumnetworks.com>
> > ---
> >  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.

>
> >  #
> >  # 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.