Re: [dpdk-dev] [PATCH 18.05 v4] eal: add function to return number of detected sockets

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 22-Mar-18 5:16 AM, gowrishankar muthukrishnan wrote:
> On Wednesday 21 March 2018 03:54 PM, Burakov, Anatoly wrote:
>>
>>>> +    config->numa_node_count = max_socket_id + 1;
>>>
>>> In some IBM servers, socket ID number does not seem to be in 
>>> sequence. For an instance, 0 and 8 for a 2 node server.
>>>
>>> In this case, numa_node_count would mislead users if wrongly 
>>> understood by its variable name IMO (see below)
>>>> +    RTE_LOG(INFO, EAL, "Detected %u NUMA nodes\n", 
>>>> config->numa_node_count);
>>>
>>> For an instance, reading above message would tell 'EAL detected 8 
>>> nodes' in my server, but actually there are only two nodes.
>>>
>>> Could its name better be 'numa_node_id_max' ?. Also, we store in 
>>> actual count of numa nodes in _count variable.
>>>
>>> Also, there could be a case when there is no local memory available 
>>> to a numa node too.
>>>
>>> Thanks,
>>> Gowrishankar
>>
>> The point of this patchset is to (pre)allocate memory only on existing 
>> sockets.
>>
>> If we don't know how many sockets there are, we are forced to 
>> preallocate VA space per each *possible* NUMA node - that is, reserve 
>> e.g. 8x128G of memory, 6 of which will go unused on a 2-socket system. 
>> We can't know if there is no memory on socket in advance, but we can 
>> at least avoid preallocating VA space for sockets that don't exist in 
>> the first place.
>>
> 
> Sounds good Anatoly.
> May be, sysfs/ might help to confirm if a numa node has local memory ?.

We can't go to sysfs every time we want to allocate memory, and we can't 
really depend on what sysfs tells us about availability of hugepages on 
a particular socket (assuming that's what you meant by "confirm if a 
numa node has local memory"). User may modify hugepage numbers for each 
socket at runtime, and suddenly we do (or don't) have memory on local 
socket.

Therefore i think a better approach would be - if a socket exists (that 
is, if we can find lcores on that socket, even if they're not active), 
assume it has/had/will have memory, and store it as a valid socket id.

I'll respin a v5 with changes outlined below then. Thanks!

> 
> Anyway, for the context of this particular patch (return numa nodes), 
> below approach you mentioned is good.
> 
>> How about we store all possible socket id's instead? e.g. something like:
>>
>> static int numa_node_ids[MAX_NUMA_NODES];
>> <...>
>> int rte_eal_cpu_init() {
>>     int sockets[RTE_MAX_LCORE];
>>     <...>
>>     for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
>>         core_to_socket[lcore_id] = socket;
> sockets[lcore_id] = eal_cpu_socket_id(lcore_id);
>>     }
>>     <...>
>>     qsort(sockets);
>>     <...>
>>     // store all unique sockets in numa_node_ids in ascending order
> 
> Just thinking that, is there a purpose of retaining a numa ID which does 
> not have local memory attached ?
> but sockets[] is suppose to reflect all available nodes though (and 
> assuming, its calling place to ensure
> for the existence of numa local memory).
> 
> 
>> }
>> <...>
>>
>> on a 2 socket system we then get:
>>
>> rte_num_sockets() => return 2
>> rte_get_socket_id(int idx) => return numa_node_ids[idx]
> rte_get_socket_mem(idx) might help to validate for local memory existence ?
> 
>>
>> Would that be suitable?
>>
> 
> Thanks,
> Gowrishankar
> 
> 


-- 
Thanks,
Anatoly