[dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations

[Shreyansh Jain]

Hi David,

Thanks for explanation. I have some comments inline...

> -----Original Message-----
> From: Hunt, David [mailto:david.hunt at intel.com]
> Sent: Tuesday, June 07, 2016 2:56 PM
> To: Shreyansh Jain <shreyansh.jain at nxp.com>; dev at dpdk.org
> Cc: olivier.matz at 6wind.com; viktorin at rehivetech.com;
> jerin.jacob at caviumnetworks.com
> Subject: Re: [dpdk-dev] [PATCH v8 1/3] mempool: support external mempool
> operations
> 
> Hi Shreyansh,
> 
> On 6/6/2016 3:38 PM, Shreyansh Jain wrote:
> > Hi,
> >
> > (Apologies for overly-eager email sent on this thread earlier. Will be more
> careful in future).
> >
> > This is more of a question/clarification than a comment. (And I have taken
> only some snippets from original mail to keep it cleaner)
> >
> > <snip>
> >> +MEMPOOL_REGISTER_OPS(ops_mp_mc);
> >> +MEMPOOL_REGISTER_OPS(ops_sp_sc);
> >> +MEMPOOL_REGISTER_OPS(ops_mp_sc);
> >> +MEMPOOL_REGISTER_OPS(ops_sp_mc);
> > <snip>
> >
> >  From the above what I understand is that multiple packet pool handlers can
> be created.
> >
> > I have a use-case where application has multiple pools but only the packet
> pool is hardware backed. Using the hardware for general buffer requirements
> would prove costly.
> >  From what I understand from the patch, selection of the pool is based on
> the flags below.
> 
> The flags are only used to select one of the default handlers for
> backward compatibility through
> the rte_mempool_create call. If you wish to use a mempool handler that
> is not one of the
> defaults, (i.e. a new hardware handler), you would use the
> rte_create_mempool_empty
> followed by the rte_mempool_set_ops_byname call.
> So, for the external handlers, you create and empty mempool, then set
> the operations (ops)
> for that particular mempool.

I am concerned about the existing applications (for example, l3fwd).
Explicit calls to 'rte_create_mempool_empty->rte_mempool_set_ops_byname' model would require modifications to these applications.
Ideally, without any modifications, these applications should be able to use packet pools (backed by hardware) and buffer pools (backed by ring/others) - transparently.

If I go by your suggestions, what I understand is, doing the above without modification to applications would be equivalent to:

  struct rte_mempool_ops custom_hw_allocator = {...}

thereafter, in config/common_base:

  CONFIG_RTE_DEFAULT_MEMPOOL_OPS="custom_hw_allocator"

calls to rte_pktmbuf_pool_create would use the new allocator.

But, another problem arises here.

There are two distinct paths for allocations of a memory pool:
1. A 'pkt' pool:
   rte_pktmbuf_pool_create   
     \- rte_mempool_create_empty
     |   \- rte_mempool_set_ops_byname(..ring_mp_mc..)
     |
     `- rte_mempool_set_ops_byname
           (...RTE_MBUF_DEFAULT_MEMPOOL_OPS..)
           /* Override default 'ring_mp_mc' of
            * rte_mempool_create */

2. Through generic mempool create API
   rte_mempool_create
     \- rte_mempool_create_empty
           (passing pktmbuf and pool constructors)
  
I found various instances in example applications where rte_mempool_create() is being called directly for packet pools - bypassing the more semantically correct call to rte_pktmbuf_* for packet pools.

In (2) control path, RTE_MBUF_DEFAULT_MEMPOOLS_OPS wouldn't be able to replace custom handler operations for packet buffer allocations.

>From a performance point-of-view, Applications should be able to select between packet pools and non-packet pools.

> 
> > <snip>
> >> +	/*
> >> +	 * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to
> >> +	 * set the correct index into the table of ops structs.
> >> +	 */
> >> +	if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
> >> +		rte_mempool_set_ops_byname(mp, "ring_sp_sc");
> >> +	else if (flags & MEMPOOL_F_SP_PUT)
> >> +		rte_mempool_set_ops_byname(mp, "ring_sp_mc");
> >> +	else if (flags & MEMPOOL_F_SC_GET)
> >> +		rte_mempool_set_ops_byname(mp, "ring_mp_sc");
> >> +	else
> >> +		rte_mempool_set_ops_byname(mp, "ring_mp_mc");
> >> +

My suggestion is to have an additional flag, 'MEMPOOL_F_PKT_ALLOC', which, if specified, would:

...
#define MEMPOOL_F_SC_GET    0x0008
#define MEMPOOL_F_PKT_ALLOC 0x0010
...

in rte_mempool_create_empty:
   ... after checking the other MEMPOOL_F_* flags...

    if (flags & MEMPOOL_F_PKT_ALLOC)
        rte_mempool_set_ops_byname(mp, RTE_MBUF_DEFAULT_MEMPOOL_OPS)

And removing the redundant call to rte_mempool_set_ops_byname() in rte_pktmbuf_create_pool().

Thereafter, rte_pktmbuf_pool_create can be changed to:

      ...
    mp = rte_mempool_create_empty(name, n, elt_size, cache_size,
-        sizeof(struct rte_pktmbuf_pool_private), socket_id, 0);
+        sizeof(struct rte_pktmbuf_pool_private), socket_id,
+        MEMPOOL_F_PKT_ALLOC);
    if (mp == NULL)
        return NULL;

> > Is there any way I can achieve the above use case of multiple pools which
> can be selected by an application - something like a run-time toggle/flag?
> >
> > -
> > Shreyansh
> 
> Yes, you can create multiple pools, some using the default handlers, and
> some using external handlers.
> There is an example of this in the autotests (app/test/test_mempool.c).
> This test creates multiple
> mempools, of which one is a custom malloc based mempool handler. The
> test puts and gets mbufs
> to/from each mempool all in the same application.

Thanks for the explanation.

> 
> Regards,
> Dave.
> 

-
Shreyansh