[v3,2/4] mempool: add non-IO flag

  Mempool is a generic allocator that is not necessarily used for device
IO operations and its memory for DMA. Add MEMPOOL_F_NON_IO flag to mark
such mempools.
Discussion: https://mails.dpdk.org/archives/dev/2021-August/216654.html

Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>
---
 doc/guides/rel_notes/release_21_11.rst | 3 +++
 lib/mempool/rte_mempool.h              | 4 ++++
 2 files changed, 7 insertions(+)
  

On Tue, Oct 12, 2021 at 5:34 AM Dmitry Kozlyuk <dkozlyuk@oss.nvidia.com> wrote:
>
> Mempool is a generic allocator that is not necessarily used for device
> IO operations and its memory for DMA. Add MEMPOOL_F_NON_IO flag to mark
> such mempools.
> Discussion: https://mails.dpdk.org/archives/dev/2021-August/216654.html
>
> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>
> ---
>  doc/guides/rel_notes/release_21_11.rst | 3 +++
>  lib/mempool/rte_mempool.h              | 4 ++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index 5036641842..dbabdc9759 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -208,6 +208,9 @@ API Changes
>    the crypto/security operation. This field will be used to communicate
>    events such as soft expiry with IPsec in lookaside mode.
>
> +* mempool: Added ``MEMPOOL_F_NON_IO`` flag to give a hint to DPDK components
> +  that objects from this pool will not be used for device IO (e.g. DMA).
> +
>
>  ABI Changes
>  -----------
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index e2bf40aa09..b48d9f89c2 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -262,6 +262,7 @@ struct rte_mempool {
>  #define MEMPOOL_F_SC_GET         0x0008 /**< Default get is "single-consumer".*/
>  #define MEMPOOL_F_POOL_CREATED   0x0010 /**< Internal: pool is created. */
>  #define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**< Don't need IOVA contiguous objs. */
> +#define MEMPOOL_F_NON_IO         0x0040 /**< Not used for device IO (DMA). */

Since it is the hint, How about changing the flag to  MEMPOOL_F_HINT_NON_IO.
Otherwise, it looks good to me.
Acked-by: Jerin Jacob <jerinj@marvell.com>


>
>  /**
>   * @internal When debug is enabled, store some statistics.
> @@ -991,6 +992,9 @@ typedef void (rte_mempool_ctor_t)(struct rte_mempool *, void *);
>   *     "single-consumer". Otherwise, it is "multi-consumers".
>   *   - MEMPOOL_F_NO_IOVA_CONTIG: If set, allocated objects won't
>   *     necessarily be contiguous in IO memory.
> + *   - MEMPOOL_F_NON_IO: If set, the mempool is considered to be
> + *     never used for device IO, i.e. for DMA operations.
> + *     It's a hint to other components and does not affect the mempool behavior.
>   * @return
>   *   The pointer to the new allocated mempool, on success. NULL on error
>   *   with rte_errno set appropriately. Possible rte_errno values include:
> --
> 2.25.1
>
  

On 10/12/21 3:04 AM, Dmitry Kozlyuk wrote:
> Mempool is a generic allocator that is not necessarily used for device
> IO operations and its memory for DMA. Add MEMPOOL_F_NON_IO flag to mark
> such mempools.
> Discussion: https://mails.dpdk.org/archives/dev/2021-August/216654.html
> 
> Signed-off-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>
> ---
>  doc/guides/rel_notes/release_21_11.rst | 3 +++
>  lib/mempool/rte_mempool.h              | 4 ++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index 5036641842..dbabdc9759 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -208,6 +208,9 @@ API Changes
>    the crypto/security operation. This field will be used to communicate
>    events such as soft expiry with IPsec in lookaside mode.
>  
> +* mempool: Added ``MEMPOOL_F_NON_IO`` flag to give a hint to DPDK components
> +  that objects from this pool will not be used for device IO (e.g. DMA).
> +
>  
>  ABI Changes
>  -----------
> diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h
> index e2bf40aa09..b48d9f89c2 100644
> --- a/lib/mempool/rte_mempool.h
> +++ b/lib/mempool/rte_mempool.h
> @@ -262,6 +262,7 @@ struct rte_mempool {
>  #define MEMPOOL_F_SC_GET         0x0008 /**< Default get is "single-consumer".*/
>  #define MEMPOOL_F_POOL_CREATED   0x0010 /**< Internal: pool is created. */
>  #define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**< Don't need IOVA contiguous objs. */
> +#define MEMPOOL_F_NON_IO         0x0040 /**< Not used for device IO (DMA). */

Doesn't it imply MEMPOOL_F_NO_IOVA_CONTIG?
Shouldn't it reject mempool population with not RTE_BAD_IOVA
iova parameter?

I see that it is just a hint, but just trying to make
full picture consistent.

As the second thought: isn't iova==RTE_BAD_IOVA
sufficient as a hint?

>  
>  /**
>   * @internal When debug is enabled, store some statistics.
> @@ -991,6 +992,9 @@ typedef void (rte_mempool_ctor_t)(struct rte_mempool *, void *);
>   *     "single-consumer". Otherwise, it is "multi-consumers".
>   *   - MEMPOOL_F_NO_IOVA_CONTIG: If set, allocated objects won't
>   *     necessarily be contiguous in IO memory.
> + *   - MEMPOOL_F_NON_IO: If set, the mempool is considered to be
> + *     never used for device IO, i.e. for DMA operations.
> + *     It's a hint to other components and does not affect the mempool behavior.
>   * @return
>   *   The pointer to the new allocated mempool, on success. NULL on error
>   *   with rte_errno set appropriately. Possible rte_errno values include:
>
  

> [...]
> > +#define MEMPOOL_F_NON_IO         0x0040 /**< Not used for device IO
> (DMA). */
> 
> Doesn't it imply MEMPOOL_F_NO_IOVA_CONTIG?

Let's leave this explicit. NO_IOVA_CONFIG could result in MEMZONE_IOVA_CONTIG (although it doesn't now), which can affect how many pages are used, which may affect performance due to TLB caches.

> Shouldn't it reject mempool population with not RTE_BAD_IOVA iova
> parameter?
>
> I see that it is just a hint, but just trying to make full picture consistent.
> 
> As the second thought: isn't iova==RTE_BAD_IOVA sufficient as a hint?

1. It looks true that if RTE_BAD_IOVA is used, we can infer it's a non-IO mempool.
2. The new flag is needed or at least handly, because otherwise to check this property of a mempool, but how? Allocating a test mbuf is doable but looks like a hack. Or we can pass this information to the callback, complicating its signature. Do you think it's better?
3. Theoretically, user may want to use mempools for objects that are used for IO, but not with DPDK. In this case IOVA will be valid, but the flag can also be set.

So I'd keep the flag and also infer it for RTE_BAD_IOVA, but allow normal IOVA.
What do you think?
  

On 10/12/21 3:40 PM, Dmitry Kozlyuk wrote:
>> [...]
>>> +#define MEMPOOL_F_NON_IO         0x0040 /**< Not used for device IO
>> (DMA). */
>>
>> Doesn't it imply MEMPOOL_F_NO_IOVA_CONTIG?
> 
> Let's leave this explicit. NO_IOVA_CONFIG could result in MEMZONE_IOVA_CONTIG (although it doesn't now), which can affect how many pages are used, which may affect performance due to TLB caches.

It sounds like a usage of a side effect of
MEMPOOL_F_NO_IOVA_CONTIG absence. It does not
sound good.

> 
>> Shouldn't it reject mempool population with not RTE_BAD_IOVA iova
>> parameter?
>>
>> I see that it is just a hint, but just trying to make full picture consistent.
>>
>> As the second thought: isn't iova==RTE_BAD_IOVA sufficient as a hint?
> 
> 1. It looks true that if RTE_BAD_IOVA is used, we can infer it's a non-IO mempool.
> 2. The new flag is needed or at least handly, because otherwise to check this property of a mempool, but how? Allocating a test mbuf is doable but looks like a hack. Or we can pass this information to the callback, complicating its signature. Do you think it's better?

mempool knows it when the mempool is populated.
So, it can just set the flag itself.

> 3. Theoretically, user may want to use mempools for objects that are used for IO, but not with DPDK. In this case IOVA will be valid, but the flag can also be set.

It sounds very artificial. Also in this case I guess
MEMPOOL_F_NON_IO should be clear anyway.

> 
> So I'd keep the flag and also infer it for RTE_BAD_IOVA, but allow normal IOVA.
> What do you think?
>
  

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: 12 октября 2021 г. 15:53
> To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>; dev@dpdk.org
> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Matan Azrad
> <matan@nvidia.com>; Olivier Matz <olivier.matz@6wind.com>
> Subject: Re: [PATCH v3 2/4] mempool: add non-IO flag
> 
> External email: Use caution opening links or attachments
> 
> 
> On 10/12/21 3:40 PM, Dmitry Kozlyuk wrote:
> >> [...]
> >>> +#define MEMPOOL_F_NON_IO         0x0040 /**< Not used for device IO
> >> (DMA). */
> >>
> >> Doesn't it imply MEMPOOL_F_NO_IOVA_CONTIG?
> >
> > Let's leave this explicit. NO_IOVA_CONFIG could result in
> MEMZONE_IOVA_CONTIG (although it doesn't now), which can affect how
> many pages are used, which may affect performance due to TLB caches.
> 
> It sounds like a usage of a side effect of MEMPOOL_F_NO_IOVA_CONTIG
> absence. It does not sound good.

I agree, but my point is that behavior should not change when specifying a hint flag.
NO_IOVA_CONTIG  => NON_IO is feasible, NON_IO => NO_IOVA_CONTIG is against
the declared NON_IO properties.

> 
> >
> >> Shouldn't it reject mempool population with not RTE_BAD_IOVA iova
> >> parameter?
> >>
> >> I see that it is just a hint, but just trying to make full picture consistent.
> >>
> >> As the second thought: isn't iova==RTE_BAD_IOVA sufficient as a hint?
> >
> > 1. It looks true that if RTE_BAD_IOVA is used, we can infer it's a non-IO
> mempool.
> > 2. The new flag is needed or at least handly, because otherwise to check this
> property of a mempool, but how? Allocating a test mbuf is doable but looks like
> a hack. Or we can pass this information to the callback, complicating its
> signature. Do you think it's better?
> 
> mempool knows it when the mempool is populated.
> So, it can just set the flag itself.

Of course, I'm only arguing that to analyze mempool properties this flag
is needed, even if the user isn't supposed to set it themselves.
Looks like we agree on this one.

> > 3. Theoretically, user may want to use mempools for objects that are used for
> IO, but not with DPDK. In this case IOVA will be valid, but the flag can also be
> set.
> 
> It sounds very artificial.
> Also in this case I guess MEMPOOL_F_NON_IO should be clear anyway.

I see NON_IO as a hint to DPDK components, not sure about non-DPDK ones.
But since there isn't a use case indeed, the flag doesn't need to eb exposed now.

To summarize:
1. MEMPOOL_F_NON_IO is considered internal, like MEMPOOL_F_CREATED.
2. It is set automatically:
a) for MEMPOOL_F_NO_IOVA_CONTIG mempools;
b) if RTE_BAD_IOVA is used to populate.
I doubt HINT should be added to the name, because it's not a hint, it's a conclusion.