[dpdk-dev] [PATCH v3 11/14] eventdev: move timer adapters memory to hugepage

[Carrillo, Erik G]

> -----Original Message-----
> From: Pavan Nikhilesh Bhagavatula <pbhagavatula at marvell.com>
> Sent: Friday, October 8, 2021 12:38 AM
> To: Carrillo, Erik G <erik.g.carrillo at intel.com>; Jerin Jacob Kollanukkaran
> <jerinj at marvell.com>
> Cc: dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v3 11/14] eventdev: move timer adapters
> memory to hugepage
> 
> Hi Erik,
> 
> >Hi Pavan,
> >
> >Some comments below:
> >
> >> -----Original Message-----
> >> From: pbhagavatula at marvell.com <pbhagavatula at marvell.com>
> >> Sent: Wednesday, October 6, 2021 1:50 AM
> >> To: jerinj at marvell.com; Carrillo, Erik G <erik.g.carrillo at intel.com>
> >> Cc: dev at dpdk.org; Pavan Nikhilesh <pbhagavatula at marvell.com>
> >> Subject: [dpdk-dev] [PATCH v3 11/14] eventdev: move timer adapters
> >> memory to hugepage
> >>
> >> From: Pavan Nikhilesh <pbhagavatula at marvell.com>
> >>
> >> Move memory used by timer adapters to hugepage.
> >> Allocate memory on the first adapter create or lookup to address both
> >> primary and secondary process usecases.
> >> This will prevent TLB misses if any and aligns to memory structure of
> >other
> >> subsystems.
> >>
> >> Signed-off-by: Pavan Nikhilesh <pbhagavatula at marvell.com>
> >> ---
> >>  lib/eventdev/rte_event_timer_adapter.c | 24
> >> +++++++++++++++++++++++-
> >>  1 file changed, 23 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/eventdev/rte_event_timer_adapter.c
> >> b/lib/eventdev/rte_event_timer_adapter.c
> >> index ae55407042..c4dc7a5fd4 100644
> >> --- a/lib/eventdev/rte_event_timer_adapter.c
> >> +++ b/lib/eventdev/rte_event_timer_adapter.c
> >> @@ -33,7 +33,7 @@ RTE_LOG_REGISTER_SUFFIX(evtim_logtype,
> >> adapter.timer, NOTICE);
> >> RTE_LOG_REGISTER_SUFFIX(evtim_buffer_logtype, adapter.timer,
> >NOTICE);
> >> RTE_LOG_REGISTER_SUFFIX(evtim_svc_logtype, adapter.timer.svc,
> >> NOTICE);
> >>
> >> -static struct rte_event_timer_adapter
> >> adapters[RTE_EVENT_TIMER_ADAPTER_NUM_MAX];
> >> +static struct rte_event_timer_adapter *adapters;
> >>
> >>  static const struct event_timer_adapter_ops swtim_ops;
> >>
> >> @@ -138,6 +138,17 @@ rte_event_timer_adapter_create_ext(
> >>  	int n, ret;
> >>  	struct rte_eventdev *dev;
> >>
> >> +	if (adapters == NULL) {
> >> +		adapters = rte_zmalloc("Eventdev",
> >> +				       sizeof(struct
> >rte_event_timer_adapter) *
> >> +
> >> RTE_EVENT_TIMER_ADAPTER_NUM_MAX,
> >> +				       RTE_CACHE_LINE_SIZE);
> >> +		if (adapters == NULL) {
> >> +			rte_errno = ENOMEM;
> >> +			return NULL;
> >> +		}
> >> +	}
> >> +
> >>  	if (conf == NULL) {
> >>  		rte_errno = EINVAL;
> >>  		return NULL;
> >> @@ -312,6 +323,17 @@ rte_event_timer_adapter_lookup(uint16_t
> >> adapter_id)
> >>  	int ret;
> >>  	struct rte_eventdev *dev;
> >>
> >> +	if (adapters == NULL) {
> >> +		adapters = rte_zmalloc("Eventdev",
> >> +				       sizeof(struct
> >rte_event_timer_adapter) *
> >> +
> >> RTE_EVENT_TIMER_ADAPTER_NUM_MAX,
> >> +				       RTE_CACHE_LINE_SIZE);
> >> +		if (adapters == NULL) {
> >> +			rte_errno = ENOMEM;
> >> +			return NULL;
> >> +		}
> >> +	}
> >> +
> >>  	if (adapters[adapter_id].allocated)
> >>  		return &adapters[adapter_id]; /* Adapter is already
> >loaded
> >> */
> >>
> >> --
> >> 2.17.1
> >
> >The rte_event_timer_adapter struct has several fields that have per-
> >process values.
> >
> >For example, there are three fast path function pointers and each will
> >be assigned distinct addresses for each process in a multi-process
> >scenario.  The "allocated" field is also per-process.  With the changes
> >above, if a secondary process did a lookup() after a primary process
> >did a create(),  the secondary would get a reference to an object with
> >function pointers that are invalid in the secondary process.
> >
> 
> I understand, the current patch doesn't unify the memory between
> processes.
> Instead, we zmalloc the per-process 'array' that holds the adapter objects
> when ever the process calls either create or lookup and initialize the per-
> process data structure.
> 
> The pointer to the adapter array is static, so when ever a process is initialized
> it will be NULL.
> 

Ah, right - I missed that.  This looks OK to me now.

One other thing: we never do a free of the array we zmalloc'd, but it looks like we could in rte_event_timer_adapter_free(), if we were freeing the last adapter instance.  

> 
> >To fully move the adapter object table into shared hugepage memory,
> >those "per-process" members would need to be collected into a per-
> >process data structure that could be independently allocated for each
> >process.  However, that would add one more pointer dereference to get
> >to the fast path functions, and avoiding that was the original reason
> >to put those pointers there.  This is similar to the rte_eventdev struct.
> >
> >Thanks,
> >Erik