[RFC,00/11] Support externally allocated memory in DPDK
mbox series

Message ID cover.1530881548.git.anatoly.burakov@intel.com
Headers show
Series
  • Support externally allocated memory in DPDK
Related show

Message

Burakov, Anatoly July 6, 2018, 1:17 p.m. UTC
This is a proposal to enable using externally allocated memory
in DPDK.

In a nutshell, here is what is being done here:

- Index malloc heaps by NUMA node index, rather than NUMA node itself
- Add identifier string to malloc heap, to uniquely identify it
- Allow creating named heaps and add/remove memory to/from those heaps
- Allocate memseg lists at runtime, to keep track of IOVA addresses
  of externally allocated memory
  - If IOVA addresses aren't provided, use RTE_BAD_IOVA
- Allow malloc and memzones to allocate from named heaps

The responsibility to ensure memory is accessible before using it is
on the shoulders of the user - there is no checking done with regards
to validity of the memory (nor could there be...).

The following limitations are present:

- No multiprocess support
- No thread safety

There is currently no way to allocate memory during initialization
stage, so even if multiprocess support is added, it is not guaranteed
to work because of underlying issues with mapping fbarrays in
secondary processes. This is not an issue in single process scenario,
but it may be an issue in a multiprocess scenario in case where
primary doesn't intend to share the externally allocated memory, yet
adding such memory could fail because some other process failed to
attach to this shared memory when it wasn't needed.

Anatoly Burakov (11):
  mem: allow memseg lists to be marked as external
  eal: add function to rerieve socket index by socket ID
  malloc: index heaps using heap ID rather than NUMA node
  malloc: add name to malloc heaps
  malloc: enable retrieving statistics from named heaps
  malloc: enable allocating from named heaps
  malloc: enable creating new malloc heaps
  malloc: allow adding memory to named heaps
  malloc: allow removing memory from named heaps
  malloc: allow destroying heaps
  memzone: enable reserving memory from named heaps

 config/common_base                            |   1 +
 lib/librte_eal/common/eal_common_lcore.c      |  15 +
 lib/librte_eal/common/eal_common_memory.c     |  51 +++-
 lib/librte_eal/common/eal_common_memzone.c    | 283 ++++++++++++++----
 .../common/include/rte_eal_memconfig.h        |   5 +-
 lib/librte_eal/common/include/rte_lcore.h     |  19 +-
 lib/librte_eal/common/include/rte_malloc.h    | 158 +++++++++-
 .../common/include/rte_malloc_heap.h          |   2 +
 lib/librte_eal/common/include/rte_memzone.h   | 183 +++++++++++
 lib/librte_eal/common/malloc_heap.c           | 277 +++++++++++++++--
 lib/librte_eal/common/malloc_heap.h           |  26 ++
 lib/librte_eal/common/rte_malloc.c            | 197 +++++++++++-
 lib/librte_eal/rte_eal_version.map            |  10 +
 13 files changed, 1118 insertions(+), 109 deletions(-)

Comments

Burakov, Anatoly July 13, 2018, 5:10 p.m. UTC | #1
On 06-Jul-18 2:17 PM, Anatoly Burakov wrote:
> This is a proposal to enable using externally allocated memory
> in DPDK.
> 
> In a nutshell, here is what is being done here:
> 
> - Index malloc heaps by NUMA node index, rather than NUMA node itself
> - Add identifier string to malloc heap, to uniquely identify it
> - Allow creating named heaps and add/remove memory to/from those heaps
> - Allocate memseg lists at runtime, to keep track of IOVA addresses
>    of externally allocated memory
>    - If IOVA addresses aren't provided, use RTE_BAD_IOVA
> - Allow malloc and memzones to allocate from named heaps
> 
> The responsibility to ensure memory is accessible before using it is
> on the shoulders of the user - there is no checking done with regards
> to validity of the memory (nor could there be...).
> 
> The following limitations are present:
> 
> - No multiprocess support
> - No thread safety
> 
> There is currently no way to allocate memory during initialization
> stage, so even if multiprocess support is added, it is not guaranteed
> to work because of underlying issues with mapping fbarrays in
> secondary processes. This is not an issue in single process scenario,
> but it may be an issue in a multiprocess scenario in case where
> primary doesn't intend to share the externally allocated memory, yet
> adding such memory could fail because some other process failed to
> attach to this shared memory when it wasn't needed.
> 
> Anatoly Burakov (11):
>    mem: allow memseg lists to be marked as external
>    eal: add function to rerieve socket index by socket ID
>    malloc: index heaps using heap ID rather than NUMA node
>    malloc: add name to malloc heaps
>    malloc: enable retrieving statistics from named heaps
>    malloc: enable allocating from named heaps
>    malloc: enable creating new malloc heaps
>    malloc: allow adding memory to named heaps
>    malloc: allow removing memory from named heaps
>    malloc: allow destroying heaps
>    memzone: enable reserving memory from named heaps
> 
>   config/common_base                            |   1 +
>   lib/librte_eal/common/eal_common_lcore.c      |  15 +
>   lib/librte_eal/common/eal_common_memory.c     |  51 +++-
>   lib/librte_eal/common/eal_common_memzone.c    | 283 ++++++++++++++----
>   .../common/include/rte_eal_memconfig.h        |   5 +-
>   lib/librte_eal/common/include/rte_lcore.h     |  19 +-
>   lib/librte_eal/common/include/rte_malloc.h    | 158 +++++++++-
>   .../common/include/rte_malloc_heap.h          |   2 +
>   lib/librte_eal/common/include/rte_memzone.h   | 183 +++++++++++
>   lib/librte_eal/common/malloc_heap.c           | 277 +++++++++++++++--
>   lib/librte_eal/common/malloc_heap.h           |  26 ++
>   lib/librte_eal/common/rte_malloc.c            | 197 +++++++++++-
>   lib/librte_eal/rte_eal_version.map            |  10 +
>   13 files changed, 1118 insertions(+), 109 deletions(-)
> 

So, now that the RFC is out, i would like to ask a general question.

One other thing that this patchset is missing, is the ability for data 
structures (e.g. hash, mempool, etc.) to be allocated from external 
heaps. Currently, we can kinda sorta do that with various _init() API's 
(initializing a data structure over already allocated memzone), but this 
is not ideal and is a hassle for anyone using external memory in DPDK.

There are basically four ways to approach this problem (that i can see).

First way is to change "socket ID" to mean "heap ID" everywhere. This 
has an upside of having a consistent API to allocate from internal and 
external heaps, with little to no API additions, only internal changes 
to account for the fact that "socket ID" is now "heap ID".

However, there is a massive downside to this approach: it is a *giant* 
API change, and it's also a giant *ABI-compatible* API change. Meaning, 
replacing socket ID with heap ID will not cause compile failures for old 
code, which would result in many subtle bugs in already existing 
codebases. So, while in the perfect world this would've been my 
preferred approach, realistically i think this is a very, very bad idea.

Second one is to add a separate "heap name" API's to everything. This 
has an upside of clean separation between allocation from internal and 
external heaps. (well, whether it's an upside is debatable...) This is 
the approach i expected to take when i was creating this patchset.

The downside is that we have to add new API's to every library and every 
DPDK data structure, to allow explicit allocation from external heaps. 
We will have to maintain both, and things like hardware drivers will 
need to have a way to indicate the need to allocate things from a 
particular external heap.

The third way is to expose the "heap ID" externally, and allow a single, 
unified API to reserve memory. That is, create an API that would map 
either a NUMA node ID or a heap name to an ID, and allow reserving 
memory through that ID regardless of whether it's internal or external 
memory. This would also allow to gradually phase out socket-based ID's 
in favor of heap ID API, should we choose to do so.

The downside for this is, it adds a layer of indirection between socket 
ID and reserving memory on a particular NUMA node, and it makes it hard 
to produce a single value of "heap ID" in such a way as to replicate 
current functionality of allocating with SOCKET_ID_ANY. Most likely user 
will have to explicitly try to allocate on all sockets, unless we keep 
old API's around in parallel.

Finally, a fourth way would be to abuse the socket ID to also mean 
something else, which is an approach i've seen numerous times already, 
and one that i don't like. We could register new heaps as a new, fake 
socket ID, and use that to address external heaps (each heap would get 
its own socket). So, keep current socket ID behavior, but for 
non-existent sockets it would be possible to be registered as a fake 
socket pointing to an external heap.

The upside for this approach would be that no API changes are required 
whatsoever to existing libraries - this scheme is compatible with both 
internal and external heaps without adding a separate API.

The downside is bad semantics - "special" sockets, handling of 
SOCKET_ID_ANY, handling of "invalid socket" vs. "invalid socket that 
happens to correspond to an existing external heap", and many other 
things that can be confusing. I don't like this option, but it's an 
option :)

Thoughts? Comments?

I myself still favor the second way, however there are good arguments to 
be made for each of these options.
Wiles, Keith July 13, 2018, 5:56 p.m. UTC | #2
> On Jul 13, 2018, at 12:10 PM, Burakov, Anatoly <anatoly.burakov@intel.com> wrote:
> 
> On 06-Jul-18 2:17 PM, Anatoly Burakov wrote:
>> This is a proposal to enable using externally allocated memory
>> in DPDK.
>> In a nutshell, here is what is being done here:
>> - Index malloc heaps by NUMA node index, rather than NUMA node itself
>> - Add identifier string to malloc heap, to uniquely identify it
>> - Allow creating named heaps and add/remove memory to/from those heaps
>> - Allocate memseg lists at runtime, to keep track of IOVA addresses
>>   of externally allocated memory
>>   - If IOVA addresses aren't provided, use RTE_BAD_IOVA
>> - Allow malloc and memzones to allocate from named heaps
>> The responsibility to ensure memory is accessible before using it is
>> on the shoulders of the user - there is no checking done with regards
>> to validity of the memory (nor could there be...).
>> The following limitations are present:
>> - No multiprocess support
>> - No thread safety
>> There is currently no way to allocate memory during initialization
>> stage, so even if multiprocess support is added, it is not guaranteed
>> to work because of underlying issues with mapping fbarrays in
>> secondary processes. This is not an issue in single process scenario,
>> but it may be an issue in a multiprocess scenario in case where
>> primary doesn't intend to share the externally allocated memory, yet
>> adding such memory could fail because some other process failed to
>> attach to this shared memory when it wasn't needed.
>> Anatoly Burakov (11):
>>   mem: allow memseg lists to be marked as external
>>   eal: add function to rerieve socket index by socket ID
>>   malloc: index heaps using heap ID rather than NUMA node
>>   malloc: add name to malloc heaps
>>   malloc: enable retrieving statistics from named heaps
>>   malloc: enable allocating from named heaps
>>   malloc: enable creating new malloc heaps
>>   malloc: allow adding memory to named heaps
>>   malloc: allow removing memory from named heaps
>>   malloc: allow destroying heaps
>>   memzone: enable reserving memory from named heaps
>>  config/common_base                            |   1 +
>>  lib/librte_eal/common/eal_common_lcore.c      |  15 +
>>  lib/librte_eal/common/eal_common_memory.c     |  51 +++-
>>  lib/librte_eal/common/eal_common_memzone.c    | 283 ++++++++++++++----
>>  .../common/include/rte_eal_memconfig.h        |   5 +-
>>  lib/librte_eal/common/include/rte_lcore.h     |  19 +-
>>  lib/librte_eal/common/include/rte_malloc.h    | 158 +++++++++-
>>  .../common/include/rte_malloc_heap.h          |   2 +
>>  lib/librte_eal/common/include/rte_memzone.h   | 183 +++++++++++
>>  lib/librte_eal/common/malloc_heap.c           | 277 +++++++++++++++--
>>  lib/librte_eal/common/malloc_heap.h           |  26 ++
>>  lib/librte_eal/common/rte_malloc.c            | 197 +++++++++++-
>>  lib/librte_eal/rte_eal_version.map            |  10 +
>>  13 files changed, 1118 insertions(+), 109 deletions(-)
> 
> So, now that the RFC is out, i would like to ask a general question.
> 
> One other thing that this patchset is missing, is the ability for data structures (e.g. hash, mempool, etc.) to be allocated from external heaps. Currently, we can kinda sorta do that with various _init() API's (initializing a data structure over already allocated memzone), but this is not ideal and is a hassle for anyone using external memory in DPDK.
> 
> There are basically four ways to approach this problem (that i can see).
> 
> First way is to change "socket ID" to mean "heap ID" everywhere. This has an upside of having a consistent API to allocate from internal and external heaps, with little to no API additions, only internal changes to account for the fact that "socket ID" is now "heap ID".
> 
> However, there is a massive downside to this approach: it is a *giant* API change, and it's also a giant *ABI-compatible* API change. Meaning, replacing socket ID with heap ID will not cause compile failures for old code, which would result in many subtle bugs in already existing codebases. So, while in the perfect world this would've been my preferred approach, realistically i think this is a very, very bad idea.
> 
> Second one is to add a separate "heap name" API's to everything. This has an upside of clean separation between allocation from internal and external heaps. (well, whether it's an upside is debatable...) This is the approach i expected to take when i was creating this patchset.
> 
> The downside is that we have to add new API's to every library and every DPDK data structure, to allow explicit allocation from external heaps. We will have to maintain both, and things like hardware drivers will need to have a way to indicate the need to allocate things from a particular external heap.
> 
> The third way is to expose the "heap ID" externally, and allow a single, unified API to reserve memory. That is, create an API that would map either a NUMA node ID or a heap name to an ID, and allow reserving memory through that ID regardless of whether it's internal or external memory. This would also allow to gradually phase out socket-based ID's in favor of heap ID API, should we choose to do so.
> 
> The downside for this is, it adds a layer of indirection between socket ID and reserving memory on a particular NUMA node, and it makes it hard to produce a single value of "heap ID" in such a way as to replicate current functionality of allocating with SOCKET_ID_ANY. Most likely user will have to explicitly try to allocate on all sockets, unless we keep old API's around in parallel.
> 
> Finally, a fourth way would be to abuse the socket ID to also mean something else, which is an approach i've seen numerous times already, and one that i don't like. We could register new heaps as a new, fake socket ID, and use that to address external heaps (each heap would get its own socket). So, keep current socket ID behavior, but for non-existent sockets it would be possible to be registered as a fake socket pointing to an external heap.
> 
> The upside for this approach would be that no API changes are required whatsoever to existing libraries - this scheme is compatible with both internal and external heaps without adding a separate API.
> 
> The downside is bad semantics - "special" sockets, handling of SOCKET_ID_ANY, handling of "invalid socket" vs. "invalid socket that happens to correspond to an existing external heap", and many other things that can be confusing. I don't like this option, but it's an option :)
> 
> Thoughts? Comments?

#1 is super clean, but very disruptive to everyone. Very Bad IMO
#2 is also clean, but adds a lot of new APIs that everyone needs to use or at least in the external heap cases.
#3 not sure I fully understand it, but reproducing heap IDs for testing is a problem and requires new/old APIs

#4 Very easy to add, IMO it is clean and very small disruption to developers. It does require the special handling, but I feel it is OK and can be explained in the docs. Having a socket id as an ‘int’ gives us a lot room e.g. id < 64K is normal socket and > 64K is external id.

My vote would be #4, as it seems the least risk and work. :-)

> 
> I myself still favor the second way, however there are good arguments to be made for each of these options.
> 
> -- 
> Thanks,
> Anatoly

Regards,
Keith
László Vadkerti July 19, 2018, 10:58 a.m. UTC | #3
> On Jul 13, 2018, at 7:57 PM, Wiles, Keith <keith.wiles@intel.com> wrote:
>
> > On Jul 13, 2018, at 12:10 PM, Burakov, Anatoly <anatoly.burakov@intel.com> wrote:
> >
> > On 06-Jul-18 2:17 PM, Anatoly Burakov wrote:
> >> This is a proposal to enable using externally allocated memory in
> >> DPDK.
> >> In a nutshell, here is what is being done here:
> >> - Index malloc heaps by NUMA node index, rather than NUMA node itself
> >> - Add identifier string to malloc heap, to uniquely identify it
> >> - Allow creating named heaps and add/remove memory to/from those
> >> heaps
> >> - Allocate memseg lists at runtime, to keep track of IOVA addresses
> >>   of externally allocated memory
> >>   - If IOVA addresses aren't provided, use RTE_BAD_IOVA
> >> - Allow malloc and memzones to allocate from named heaps The
> >> responsibility to ensure memory is accessible before using it is on
> >> the shoulders of the user - there is no checking done with regards to
> >> validity of the memory (nor could there be...).
> >> The following limitations are present:
> >> - No multiprocess support
> >> - No thread safety
> >> There is currently no way to allocate memory during initialization
> >> stage, so even if multiprocess support is added, it is not guaranteed
> >> to work because of underlying issues with mapping fbarrays in
> >> secondary processes. This is not an issue in single process scenario,
> >> but it may be an issue in a multiprocess scenario in case where
> >> primary doesn't intend to share the externally allocated memory, yet
> >> adding such memory could fail because some other process failed to
> >> attach to this shared memory when it wasn't needed.
> >> Anatoly Burakov (11):
> >>   mem: allow memseg lists to be marked as external
> >>   eal: add function to rerieve socket index by socket ID
> >>   malloc: index heaps using heap ID rather than NUMA node
> >>   malloc: add name to malloc heaps
> >>   malloc: enable retrieving statistics from named heaps
> >>   malloc: enable allocating from named heaps
> >>   malloc: enable creating new malloc heaps
> >>   malloc: allow adding memory to named heaps
> >>   malloc: allow removing memory from named heaps
> >>   malloc: allow destroying heaps
> >>   memzone: enable reserving memory from named heaps
> >>  config/common_base                            |   1 +
> >>  lib/librte_eal/common/eal_common_lcore.c      |  15 +
> >>  lib/librte_eal/common/eal_common_memory.c     |  51 +++-
> >>  lib/librte_eal/common/eal_common_memzone.c    | 283
> ++++++++++++++----
> >>  .../common/include/rte_eal_memconfig.h        |   5 +-
> >>  lib/librte_eal/common/include/rte_lcore.h     |  19 +-
> >>  lib/librte_eal/common/include/rte_malloc.h    | 158 +++++++++-
> >>  .../common/include/rte_malloc_heap.h          |   2 +
> >>  lib/librte_eal/common/include/rte_memzone.h   | 183 +++++++++++
> >>  lib/librte_eal/common/malloc_heap.c           | 277 +++++++++++++++--
> >>  lib/librte_eal/common/malloc_heap.h           |  26 ++
> >>  lib/librte_eal/common/rte_malloc.c            | 197 +++++++++++-
> >>  lib/librte_eal/rte_eal_version.map            |  10 +
> >>  13 files changed, 1118 insertions(+), 109 deletions(-)
> >
> > So, now that the RFC is out, i would like to ask a general question.
> >
> > One other thing that this patchset is missing, is the ability for data
> structures (e.g. hash, mempool, etc.) to be allocated from external heaps.
> Currently, we can kinda sorta do that with various _init() API's (initializing a
> data structure over already allocated memzone), but this is not ideal and is a
> hassle for anyone using external memory in DPDK.
> >
> > There are basically four ways to approach this problem (that i can see).
> >
> > First way is to change "socket ID" to mean "heap ID" everywhere. This has
> an upside of having a consistent API to allocate from internal and external
> heaps, with little to no API additions, only internal changes to account for the
> fact that "socket ID" is now "heap ID".
> >
> > However, there is a massive downside to this approach: it is a *giant* API
> change, and it's also a giant *ABI-compatible* API change. Meaning,
> replacing socket ID with heap ID will not cause compile failures for old code,
> which would result in many subtle bugs in already existing codebases. So,
> while in the perfect world this would've been my preferred approach,
> realistically i think this is a very, very bad idea.
> >
> > Second one is to add a separate "heap name" API's to everything. This has
> an upside of clean separation between allocation from internal and external
> heaps. (well, whether it's an upside is debatable...) This is the approach i
> expected to take when i was creating this patchset.
> >
> > The downside is that we have to add new API's to every library and every
> DPDK data structure, to allow explicit allocation from external heaps. We will
> have to maintain both, and things like hardware drivers will need to have a
> way to indicate the need to allocate things from a particular external heap.
> >
> > The third way is to expose the "heap ID" externally, and allow a single,
> unified API to reserve memory. That is, create an API that would map either
> a NUMA node ID or a heap name to an ID, and allow reserving memory
> through that ID regardless of whether it's internal or external memory. This
> would also allow to gradually phase out socket-based ID's in favor of heap ID
> API, should we choose to do so.
> >
> > The downside for this is, it adds a layer of indirection between socket ID
> and reserving memory on a particular NUMA node, and it makes it hard to
> produce a single value of "heap ID" in such a way as to replicate current
> functionality of allocating with SOCKET_ID_ANY. Most likely user will have to
> explicitly try to allocate on all sockets, unless we keep old API's around in
> parallel.
> >
> > Finally, a fourth way would be to abuse the socket ID to also mean
> something else, which is an approach i've seen numerous times already, and
> one that i don't like. We could register new heaps as a new, fake socket ID,
> and use that to address external heaps (each heap would get its own
> socket). So, keep current socket ID behavior, but for non-existent sockets it
> would be possible to be registered as a fake socket pointing to an external
> heap.
> >
> > The upside for this approach would be that no API changes are required
> whatsoever to existing libraries - this scheme is compatible with both internal
> and external heaps without adding a separate API.
> >
> > The downside is bad semantics - "special" sockets, handling of
> > SOCKET_ID_ANY, handling of "invalid socket" vs. "invalid socket that
> > happens to correspond to an existing external heap", and many other
> > things that can be confusing. I don't like this option, but it's an
> > option :)
> >
> > Thoughts? Comments?
> 
> #1 is super clean, but very disruptive to everyone. Very Bad IMO
> #2 is also clean, but adds a lot of new APIs that everyone needs to use or at
> least in the external heap cases.
> #3 not sure I fully understand it, but reproducing heap IDs for testing is a
> problem and requires new/old APIs
> 
> #4 Very easy to add, IMO it is clean and very small disruption to developers.
> It does require the special handling, but I feel it is OK and can be explained in
> the docs. Having a socket id as an ‘int’ gives us a lot room e.g. id < 64K is
> normal socket and > 64K is external id.
> 
> My vote would be #4, as it seems the least risk and work. :-)
> 
We are living with #4 (overloaded socket_ids) since ~5 years now but it indeed generates some confusion and it is a kind of hack so it may not be the best choice going forward in official releases but for sure is the easiest/simplest solution requiring the least modifications.
Using an overloaded socket_id is especially disturbing in the dump memory config printout where the user will see multiple socket ids on a single socket system or more than the available real number of sockets, however it could still be explained in the notes and the documentation.
The allocation behavior with SOCKET_ID_ANY is also a question as I think it shouldn’t roll over to allocate memory in the external heap, we especially disabled this feature in our implementation. The reason behind is that the external memory may be a limited resource where only explicit allocation requests would be allowed and also in a multi-process environment we may not want all external heaps to be mapped into all other processes address space meaning that not all heaps are accessible from every process (I’m not sure if it is planned to be supported though but it would be an important and useful feature based on our experiences).
Anyway I think the confusion with this option comes due to the misleading “socket_id” name which would not really mean socket id anymore. So we should probably just document it as pseudo socket_id  and problem solved with #4 :)

The cleanest solution in my opinion would be #1 which could be combined with #4 so that the physical socket_id could be directly passed as the heap_id (or rather call it allocation id or just location?) so that backward compatibility could also be kept.
Meaning to apply #1, change “socket_id” to “heap_id” (or “alloc_id”?) in all functions which are today expecting the socket_id to indicate the location of the allocations but keep a direct mapping from socket_id to heap_id, e.g. as Keith suggested lower range of heap_id would be equivalent to the socket_id and upper range would be the external id, this way even existing applications would still work without changing the code just by passing the socket_id in the heap_id parameter. However it is a question what would happen with the socket_id stored in data structures such as struct rte_mempool where socket_id is stored with a meaning “Socket id passed at create.”
SOCKET_ID_ANY would only mean lower range of heap_ids (physical socket ids) but not the external heap and if needed a new HEAP_ID_ANY could be introduced.

If changing heap_id to socket_id in existing functions is a big issue then one option would be to keep the original API and introduce new equivalent functions allowing to use the heap_id instead of the socket_id, e.g. rte_mempool_create would have an equivalent function to use with the heap_id instead of the socket_id.
Socket_id could then be converted to heap_id with a new function which should always be possible and can still use to direct mapping approach with lower/upper range convention.
The socket_id based functions would then just be wrappers calling the heap_id equivalent function after converting the socket_id to heap_id.
Using socket_id to indicate the location could still be relevant so the old socket_id based functions may not even need to be deprecated unless it would become hard to maintain.

Irrespective of the chosen option, external heaps should be registered/identified by name and there could be a function to fetch/lookup the id (heap_id or pseudo socket_id) by registered heap name which then could be used in the related API calls.

It would also be another work item to update all the data structures which are storing the socket_id to use it as the location identifier and I think few of them may need to store both the real physical socket_id and the heap_id, e.g. in struct lcore_config where the user may want to know the real physical socket id but want to set specific heap_id as the default allocation location for the given lcore.

> >
> > I myself still favor the second way, however there are good arguments to
> be made for each of these options.
> >
> > --
> > Thanks,
> > Anatoly
> 
> Regards,
> Keith

Thanks,
 Laszlo
Burakov, Anatoly July 26, 2018, 1:48 p.m. UTC | #4
On 19-Jul-18 11:58 AM, László Vadkerti wrote:
>> On Jul 13, 2018, at 7:57 PM, Wiles, Keith <keith.wiles@intel.com> wrote:
>>
>>> On Jul 13, 2018, at 12:10 PM, Burakov, Anatoly <anatoly.burakov@intel.com> wrote:
>>>
>>> On 06-Jul-18 2:17 PM, Anatoly Burakov wrote:
>>>> This is a proposal to enable using externally allocated memory in
>>>> DPDK.
>>>> In a nutshell, here is what is being done here:
>>>> - Index malloc heaps by NUMA node index, rather than NUMA node itself
>>>> - Add identifier string to malloc heap, to uniquely identify it
>>>> - Allow creating named heaps and add/remove memory to/from those
>>>> heaps
>>>> - Allocate memseg lists at runtime, to keep track of IOVA addresses
>>>>    of externally allocated memory
>>>>    - If IOVA addresses aren't provided, use RTE_BAD_IOVA
>>>> - Allow malloc and memzones to allocate from named heaps The
>>>> responsibility to ensure memory is accessible before using it is on
>>>> the shoulders of the user - there is no checking done with regards to
>>>> validity of the memory (nor could there be...).
>>>> The following limitations are present:
>>>> - No multiprocess support
>>>> - No thread safety
>>>> There is currently no way to allocate memory during initialization
>>>> stage, so even if multiprocess support is added, it is not guaranteed
>>>> to work because of underlying issues with mapping fbarrays in
>>>> secondary processes. This is not an issue in single process scenario,
>>>> but it may be an issue in a multiprocess scenario in case where
>>>> primary doesn't intend to share the externally allocated memory, yet
>>>> adding such memory could fail because some other process failed to
>>>> attach to this shared memory when it wasn't needed.
>>>> Anatoly Burakov (11):
>>>>    mem: allow memseg lists to be marked as external
>>>>    eal: add function to rerieve socket index by socket ID
>>>>    malloc: index heaps using heap ID rather than NUMA node
>>>>    malloc: add name to malloc heaps
>>>>    malloc: enable retrieving statistics from named heaps
>>>>    malloc: enable allocating from named heaps
>>>>    malloc: enable creating new malloc heaps
>>>>    malloc: allow adding memory to named heaps
>>>>    malloc: allow removing memory from named heaps
>>>>    malloc: allow destroying heaps
>>>>    memzone: enable reserving memory from named heaps
>>>>   config/common_base                            |   1 +
>>>>   lib/librte_eal/common/eal_common_lcore.c      |  15 +
>>>>   lib/librte_eal/common/eal_common_memory.c     |  51 +++-
>>>>   lib/librte_eal/common/eal_common_memzone.c    | 283
>> ++++++++++++++----
>>>>   .../common/include/rte_eal_memconfig.h        |   5 +-
>>>>   lib/librte_eal/common/include/rte_lcore.h     |  19 +-
>>>>   lib/librte_eal/common/include/rte_malloc.h    | 158 +++++++++-
>>>>   .../common/include/rte_malloc_heap.h          |   2 +
>>>>   lib/librte_eal/common/include/rte_memzone.h   | 183 +++++++++++
>>>>   lib/librte_eal/common/malloc_heap.c           | 277 +++++++++++++++--
>>>>   lib/librte_eal/common/malloc_heap.h           |  26 ++
>>>>   lib/librte_eal/common/rte_malloc.c            | 197 +++++++++++-
>>>>   lib/librte_eal/rte_eal_version.map            |  10 +
>>>>   13 files changed, 1118 insertions(+), 109 deletions(-)
>>>
>>> So, now that the RFC is out, i would like to ask a general question.
>>>
>>> One other thing that this patchset is missing, is the ability for data
>> structures (e.g. hash, mempool, etc.) to be allocated from external heaps.
>> Currently, we can kinda sorta do that with various _init() API's (initializing a
>> data structure over already allocated memzone), but this is not ideal and is a
>> hassle for anyone using external memory in DPDK.
>>>
>>> There are basically four ways to approach this problem (that i can see).
>>>
>>> First way is to change "socket ID" to mean "heap ID" everywhere. This has
>> an upside of having a consistent API to allocate from internal and external
>> heaps, with little to no API additions, only internal changes to account for the
>> fact that "socket ID" is now "heap ID".
>>>
>>> However, there is a massive downside to this approach: it is a *giant* API
>> change, and it's also a giant *ABI-compatible* API change. Meaning,
>> replacing socket ID with heap ID will not cause compile failures for old code,
>> which would result in many subtle bugs in already existing codebases. So,
>> while in the perfect world this would've been my preferred approach,
>> realistically i think this is a very, very bad idea.
>>>
>>> Second one is to add a separate "heap name" API's to everything. This has
>> an upside of clean separation between allocation from internal and external
>> heaps. (well, whether it's an upside is debatable...) This is the approach i
>> expected to take when i was creating this patchset.
>>>
>>> The downside is that we have to add new API's to every library and every
>> DPDK data structure, to allow explicit allocation from external heaps. We will
>> have to maintain both, and things like hardware drivers will need to have a
>> way to indicate the need to allocate things from a particular external heap.
>>>
>>> The third way is to expose the "heap ID" externally, and allow a single,
>> unified API to reserve memory. That is, create an API that would map either
>> a NUMA node ID or a heap name to an ID, and allow reserving memory
>> through that ID regardless of whether it's internal or external memory. This
>> would also allow to gradually phase out socket-based ID's in favor of heap ID
>> API, should we choose to do so.
>>>
>>> The downside for this is, it adds a layer of indirection between socket ID
>> and reserving memory on a particular NUMA node, and it makes it hard to
>> produce a single value of "heap ID" in such a way as to replicate current
>> functionality of allocating with SOCKET_ID_ANY. Most likely user will have to
>> explicitly try to allocate on all sockets, unless we keep old API's around in
>> parallel.
>>>
>>> Finally, a fourth way would be to abuse the socket ID to also mean
>> something else, which is an approach i've seen numerous times already, and
>> one that i don't like. We could register new heaps as a new, fake socket ID,
>> and use that to address external heaps (each heap would get its own
>> socket). So, keep current socket ID behavior, but for non-existent sockets it
>> would be possible to be registered as a fake socket pointing to an external
>> heap.
>>>
>>> The upside for this approach would be that no API changes are required
>> whatsoever to existing libraries - this scheme is compatible with both internal
>> and external heaps without adding a separate API.
>>>
>>> The downside is bad semantics - "special" sockets, handling of
>>> SOCKET_ID_ANY, handling of "invalid socket" vs. "invalid socket that
>>> happens to correspond to an existing external heap", and many other
>>> things that can be confusing. I don't like this option, but it's an
>>> option :)
>>>
>>> Thoughts? Comments?
>>
>> #1 is super clean, but very disruptive to everyone. Very Bad IMO
>> #2 is also clean, but adds a lot of new APIs that everyone needs to use or at
>> least in the external heap cases.
>> #3 not sure I fully understand it, but reproducing heap IDs for testing is a
>> problem and requires new/old APIs
>>
>> #4 Very easy to add, IMO it is clean and very small disruption to developers.
>> It does require the special handling, but I feel it is OK and can be explained in
>> the docs. Having a socket id as an ‘int’ gives us a lot room e.g. id < 64K is
>> normal socket and > 64K is external id.
>>
>> My vote would be #4, as it seems the least risk and work. :-)
>>
> We are living with #4 (overloaded socket_ids) since ~5 years now but it indeed generates some confusion and it is a kind of hack so it may not be the best choice going forward in official releases but for sure is the easiest/simplest solution requiring the least modifications.
> Using an overloaded socket_id is especially disturbing in the dump memory config printout where the user will see multiple socket ids on a single socket system or more than the available real number of sockets, however it could still be explained in the notes and the documentation.
> The allocation behavior with SOCKET_ID_ANY is also a question as I think it shouldn’t roll over to allocate memory in the external heap, we especially disabled this feature in our implementation. The reason behind is that the external memory may be a limited resource where only explicit allocation requests would be allowed and also in a multi-process environment we may not want all external heaps to be mapped into all other processes address space meaning that not all heaps are accessible from every process (I’m not sure if it is planned to be supported though but it would be an important and useful feature based on our experiences).

Hi Laszlo,

That depends on what you mean by "all other processes". If they are all 
part of the primary-secondary process prefix, then my plan is to enable 
private and shared heaps - i.e. a heap is either available to a single 
process, or it is available to some or all processes within a prefix.

It is also not possible to share the same area with different process 
prefixes (i.e. between two different primaries) because each of the 
processes will think it owns the entire memory and will do with it as it 
pleases. Using the same memory region with two different process 
prefixes will break many assumptions heap has - for example, it relies 
on a per-heap lock to control access to the heap, and that will not work 
if you map the same memory area into multiple primary processes. I do 
not foresee a mechanism to fix this problem within DPDK, but obviously 
if you have any suggestions, they will be considered :)

The reason we have to care about private vs. shared heaps is because of 
how DPDK handles memory management. In order for DPDK facilities such as 
rte_mem_virt2iova() or rte_memseg_walk() to work, we need to keep track 
of the pages we use for the heap - i.e. from DPDK's point of view, 
external memory behaves just like regular memory and is tracked using 
the same method of keeping page tables around (see rte_memseg_list).

These page tables need to be shared between all processes that use a 
specific heap. This introduces an inherent point of failure - you may 
mmap() the *area itself* successfully at the same address, but you may 
still fail to *attach to the page tables*, which will cause a particular 
heap to not be available in a process. This is a problem that i do not 
see a solution for at the moment, and it is something that users 
attempting to use external memory in secondary processes will have to 
deal with.

I haven't yet decided whether this should be automatic (i.e. shared 
heaps "automagically" appearing in all processes) or manual (make the 
user explicitly attach to an externally allocated heap in each process 
within the prefix). I would tend to go for the latter as it gives the 
user more control, and it is easier to implement because there's no need 
to engage IPC to make this work.

> Anyway I think the confusion with this option comes due to the misleading “socket_id” name which would not really mean socket id anymore. So we should probably just document it as pseudo socket_id  and problem solved with #4 :)
> 
> The cleanest solution in my opinion would be #1 which could be combined with #4 so that the physical socket_id could be directly passed as the heap_id (or rather call it allocation id or just location?) so that backward compatibility could also be kept.
> Meaning to apply #1, change “socket_id” to “heap_id” (or “alloc_id”?) in all functions which are today expecting the socket_id to indicate the location of the allocations but keep a direct mapping from socket_id to heap_id, e.g. as Keith suggested lower range of heap_id would be equivalent to the socket_id and upper range would be the external id, this way even existing applications would still work without changing the code just by passing the socket_id in the heap_id parameter. However it is a question what would happen with the socket_id stored in data structures such as struct rte_mempool where socket_id is stored with a meaning “Socket id passed at create.”
> SOCKET_ID_ANY would only mean lower range of heap_ids (physical socket ids) but not the external heap and if needed a new HEAP_ID_ANY could be introduced.
> 
> If changing heap_id to socket_id in existing functions is a big issue then one option would be to keep the original API and introduce new equivalent functions allowing to use the heap_id instead of the socket_id, e.g. rte_mempool_create would have an equivalent function to use with the heap_id instead of the socket_id.
> Socket_id could then be converted to heap_id with a new function which should always be possible and can still use to direct mapping approach with lower/upper range convention.
> The socket_id based functions would then just be wrappers calling the heap_id equivalent function after converting the socket_id to heap_id.
> Using socket_id to indicate the location could still be relevant so the old socket_id based functions may not even need to be deprecated unless it would become hard to maintain.
> 
> Irrespective of the chosen option, external heaps should be registered/identified by name and there could be a function to fetch/lookup the id (heap_id or pseudo socket_id) by registered heap name which then could be used in the related API calls.

So, in other words, the consesus seems to be that we need to stay with 
the old socket_id and just use weird socket ID's for external heaps. 
Okay, so be it. Less work for me implementing it :)

> 
> It would also be another work item to update all the data structures which are storing the socket_id to use it as the location identifier and I think few of them may need to store both the real physical socket_id and the heap_id, e.g. in struct lcore_config where the user may want to know the real physical socket id but want to set specific heap_id as the default allocation location for the given lcore.

I do not see physical socket ID of externally allocated memory as a 
matter of concern for DPDK. I think this information should be up to the 
user application to handle, not DPDK. From my point of view, we 
shouldn't care where the memory came from, we just facilitate using it. 
If the user chooses to store additional metadata about the memory 
somewhere else - that is his prerogative, but i don't think having a 
provision for "physical socket ID" etc for external heaps should be in DPDK.

> 
>>>
>>> I myself still favor the second way, however there are good arguments to
>> be made for each of these options.
>>>
>>> --
>>> Thanks,
>>> Anatoly
>>
>> Regards,
>> Keith
> 
> Thanks,
>   Laszlo
>