Re: [SPDK] spdk_malloc vs. malloc

[Meneghini, John <John.Meneghini at netapp.com>]

[-- Attachment #1: Type: text/plain, Size: 4994 bytes --]

Thanks, Ben, et al, for your reply.

> you may want to avoid calling them at run-time because they can fail, but SPDK generally already does
> that for performance reasons anyway. Can you outline what you mean?

What is run-time?   We are using the SPDK libraries inside of a kernel kmod.  So, all time is run-time.

I think we can end this thread.  I tried to win this argument once before. I guess calloc() and malloc() are here to stay... so
it doesn't matter what we do with spdk_malloc().

:-)

If anyone wants to ask me about my views on this topic, I'll be at the SPDK Developer's Summit in two weeks.

See you then.

/John 

On 5/3/18, 7:24 PM, "Walker, Benjamin" <benjamin.walker(a)intel.com> wrote:

    On Thu, 2018-05-03 at 20:14 +0000, Meneghini, John wrote:
    > The issue is: spdk_malloc and spdk_calloc are logical APIs to abstract the
    > POSIX malloc and calloc calls.  We chose to NOT use those API names originally
    > and went with the spdk_dma_{} api names. My concern is that taking those API
    > names now will eliminate the possibility of any other use in the future.
    > 
    > I am not asking to "fully abstract POSIX".  I am simply saying that the
    > malloc(), calloc() and free()  POSIX APIs suffer from some problems.  This is
    > why DPDK implemented rte_malloc() and rte_calloc().   As we move forward to
    > build production quality products out of SPDK we will want to abstract malloc,
    > calloc and free; especially in the libraries.  I don't care about what the
    > applications do.  I want a better memory management APIs in the libraries.
    
    DPDK implemented rte_malloc and rte_calloc to allocate DMA-safe and shared
    memory. They still call POSIX malloc and calloc for all other types of
    allocations. This is the same thing that SPDK has done.
    
    I've never heard of malloc, calloc, and free having problems to the extent that
    you'd want to reimplement them in your application some other way. Certainly,
    you may want to avoid calling them at run-time because they can fail, but SPDK
    generally already does that for performance reasons anyway. Can you outline what
    you mean?
    
    > 
    > I'd be happy to simply map spdk_malloc/calloc() to rte_malloc/calloc() and I
    > don't see the point introducing an spdk_malloc() that only extends
    > spdk_dma_malloc().  I'd rather absorb an API change to spdk_dma_malloc().  We
    > change many APIs in SPDK from release to release. I don't see why this API
    > can't be changed - with agreement from the SPDK community.
    > 
    > /*
    >  * Allocate memory on default heap.
    >  */
    > void *
    > rte_malloc(const char *type, size_t size, unsigned align)
    > {
    >         return rte_malloc_socket(type, size, align, SOCKET_ID_ANY);
    > }
    > 
    > /*     
    >  * Allocate zero'd memory on default heap.
    >  */
    > void *
    > rte_calloc(const char *type, size_t num, size_t size, unsigned align)
    > {
    >         return rte_zmalloc(type, num * size, align);
    > }
    > 
    > /John
    > 
    > On 5/3/18, 3:21 PM, "Walker, Benjamin" <benjamin.walker(a)intel.com> wrote:
    > 
    >     On Thu, 2018-05-03 at 18:23 +0000, Meneghini, John wrote:
    >     > >  If the user passes flags == 0 to the new spdk_malloc() call, this
    > could be
    >     > implemented by malloc() or equivalent behind the scenes,
    >     >  
    >     > So, does this mean you’re willing to change all calls to malloc(size)
    > with
    >     > spdk_malloc(size, 0, NULL, SPDK_ENV_SOCKET_ID_ANY, SPDK_MALLOC_SHARE)?
    >     >  
    >     > Is that the plan?
    >     > 
    >     
    >     Hi John,
    >         
    >     SPDK explicitly depends on POSIX throughout the code base. We do have an
    >     environment abstraction layer designed to make porting SPDK to various
    >     environments (i.e. away from DPDK) easier, but this only abstracts
    > operations
    >     that are not available through standard POSIX calls. We're concerned that
    > fully
    >     abstracting POSIX would introduce a significant barrier to entry for new
    >     contributors to the project, while only enabling one additional use case
    > that
    >     we're aware of. I understand that this one use case happens to be yours,
    > and so
    >     this is a challenge for you.
    >         
    >     In your code today, I assume you carry patches to replace the POSIX calls
    > with
    >     your available alternatives. We've attempted to make carrying these
    > patches
    >     reasonably easy by moving all of the POSIX includes into a single header
    >     (include/stdinc.h). Since you're already carrying those patches, can you
    > simply
    >     choose a different name for your replacement for malloc/calloc?
    >     
    >     Thanks,
    >     Ben
    >     
    >