Re: [dpdk-dev] [RFC 00/11] Support externally allocated memory in DPDK

["László Vadkerti" <laszlo.vadkerti@ericsson.com>]

> 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