Hi Erik,
>Hi Pavan,
>
>Some comments below:
>
>> -----Original Message-----
>> From: pbhagavat...@marvell.com <pbhagavat...@marvell.com>
>> Sent: Wednesday, October 6, 2021 1:50 AM
>> To: jer...@marvell.com; Carrillo, Erik G <erik.g.carri...@intel.com>
>> Cc: dev@dpdk.org; Pavan Nikhilesh <pbhagavat...@marvell.com>
>> Subject: [dpdk-dev] [PATCH v3 11/14] eventdev: move timer adapters
>> memory to hugepage
>>
>> From: Pavan Nikhilesh <pbhagavat...@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 <pbhagavat...@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.
>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
