[dpdk-dev,v3,08/12] mempool: allow config override on element alignment
Commit Message
On TILE-Gx and TILE-Mx platforms, the buffers fed into the hardware
buffer manager require a 128-byte alignment. With this change, we
allow configuration based override of the element alignment, and
default to RTE_CACHE_LINE_SIZE if left unspecified.
Change-Id: I9cd789d92b0bc9c8f44a633de59bb04d45d927a7
Signed-off-by: Zhigang Lu <zlu@ezchip.com>
---
lib/librte_mempool/rte_mempool.c | 16 +++++++++-------
lib/librte_mempool/rte_mempool.h | 6 ++++++
2 files changed, 15 insertions(+), 7 deletions(-)
Comments
On Mon, Jul 06, 2015 at 04:51:33PM +0800, Zhigang Lu wrote:
> On TILE-Gx and TILE-Mx platforms, the buffers fed into the hardware
> buffer manager require a 128-byte alignment. With this change, we
> allow configuration based override of the element alignment, and
> default to RTE_CACHE_LINE_SIZE if left unspecified.
>
> Change-Id: I9cd789d92b0bc9c8f44a633de59bb04d45d927a7
> Signed-off-by: Zhigang Lu <zlu@ezchip.com>
This looks an OK change. However, would it be worthwhile making this a runtime
parameter rather than a compile-time one? Is it likely that we will ever have
a case where someone wants two mempools with different alignments (and where
using the larger of the two would be problematic)?
/Bruce
>-----Original Message-----
>From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
>Sent: Monday, July 06, 2015 11:38 PM
>To: Zhigang Lu
>Cc: dev@dpdk.org
>Subject: Re: [dpdk-dev] [PATCH v3 08/12] mempool: allow config override on
>element alignment
>
>On Mon, Jul 06, 2015 at 04:51:33PM +0800, Zhigang Lu wrote:
>> On TILE-Gx and TILE-Mx platforms, the buffers fed into the hardware
>> buffer manager require a 128-byte alignment. With this change, we
>> allow configuration based override of the element alignment, and
>> default to RTE_CACHE_LINE_SIZE if left unspecified.
>>
>> Change-Id: I9cd789d92b0bc9c8f44a633de59bb04d45d927a7
>> Signed-off-by: Zhigang Lu <zlu@ezchip.com>
>
>This looks an OK change. However, would it be worthwhile making this a
runtime
>parameter rather than a compile-time one? Is it likely that we will ever
have a
>case where someone wants two mempools with different alignments (and
>where using the larger of the two would be problematic)?
For now, I don't think it is very much worthwhile making this a runtime
parameter,
since doing so requires changing the mempool library API, and also the users
of mempool
do not quite care about the underlying alignments. Currently, the alignment
for mempool
objects is mostly a hardware requirement (currently RTE_CACHE_LINE_SIZE for
good
performance). And now we are defining a new RTE_MEMPOOL_ALIGN for mempool
alignment requirement for cases where someone needs other alignments than
RTE_CACHE_LINE_SIZE.
If someone wants two mempools with different alignments, using the larger
one would
not be a problem in current mempool implementation. Because even for the
case where
there is one alignment requirement RTE_CACHE_LINE_SIZE, we would provide
larger one
(2* RTE_CACHE_LINE_SIZE and larger) for allocated mempool objects, as those
objects
are continuous in memory. So we could not avoid larger one in current
implementation.
Thanks again for reviewing!
-Zhigang
>/Bruce
On Tue, Jul 07, 2015 at 05:15:41PM +0800, Tony Lu wrote:
>
>
> >-----Original Message-----
> >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> >Sent: Monday, July 06, 2015 11:38 PM
> >To: Zhigang Lu
> >Cc: dev@dpdk.org
> >Subject: Re: [dpdk-dev] [PATCH v3 08/12] mempool: allow config override on
> >element alignment
> >
> >On Mon, Jul 06, 2015 at 04:51:33PM +0800, Zhigang Lu wrote:
> >> On TILE-Gx and TILE-Mx platforms, the buffers fed into the hardware
> >> buffer manager require a 128-byte alignment. With this change, we
> >> allow configuration based override of the element alignment, and
> >> default to RTE_CACHE_LINE_SIZE if left unspecified.
> >>
> >> Change-Id: I9cd789d92b0bc9c8f44a633de59bb04d45d927a7
> >> Signed-off-by: Zhigang Lu <zlu@ezchip.com>
> >
> >This looks an OK change. However, would it be worthwhile making this a
> runtime
> >parameter rather than a compile-time one? Is it likely that we will ever
> have a
> >case where someone wants two mempools with different alignments (and
> >where using the larger of the two would be problematic)?
>
> For now, I don't think it is very much worthwhile making this a runtime
> parameter,
> since doing so requires changing the mempool library API, and also the users
> of mempool
> do not quite care about the underlying alignments. Currently, the alignment
> for mempool
> objects is mostly a hardware requirement (currently RTE_CACHE_LINE_SIZE for
> good
> performance). And now we are defining a new RTE_MEMPOOL_ALIGN for mempool
> alignment requirement for cases where someone needs other alignments than
> RTE_CACHE_LINE_SIZE.
>
> If someone wants two mempools with different alignments, using the larger
> one would
> not be a problem in current mempool implementation. Because even for the
> case where
> there is one alignment requirement RTE_CACHE_LINE_SIZE, we would provide
> larger one
> (2* RTE_CACHE_LINE_SIZE and larger) for allocated mempool objects, as those
> objects
> are continuous in memory. So we could not avoid larger one in current
> implementation.
>
> Thanks again for reviewing!
> -Zhigang
>
> >/Bruce
>
Ok, makes sense.
/Bruce
@@ -120,10 +120,10 @@ static unsigned optimize_object_size(unsigned obj_size)
nrank = 1;
/* process new object size */
- new_obj_size = (obj_size + RTE_CACHE_LINE_MASK) / RTE_CACHE_LINE_SIZE;
+ new_obj_size = (obj_size + RTE_MEMPOOL_ALIGN_MASK) / RTE_MEMPOOL_ALIGN;
while (get_gcd(new_obj_size, nrank * nchan) != 1)
new_obj_size++;
- return new_obj_size * RTE_CACHE_LINE_SIZE;
+ return new_obj_size * RTE_MEMPOOL_ALIGN;
}
static void
@@ -267,7 +267,7 @@ rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
#endif
if ((flags & MEMPOOL_F_NO_CACHE_ALIGN) == 0)
sz->header_size = RTE_ALIGN_CEIL(sz->header_size,
- RTE_CACHE_LINE_SIZE);
+ RTE_MEMPOOL_ALIGN);
/* trailer contains the cookie in debug mode */
sz->trailer_size = 0;
@@ -281,9 +281,9 @@ rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
if ((flags & MEMPOOL_F_NO_CACHE_ALIGN) == 0) {
sz->total_size = sz->header_size + sz->elt_size +
sz->trailer_size;
- sz->trailer_size += ((RTE_CACHE_LINE_SIZE -
- (sz->total_size & RTE_CACHE_LINE_MASK)) &
- RTE_CACHE_LINE_MASK);
+ sz->trailer_size += ((RTE_MEMPOOL_ALIGN -
+ (sz->total_size & RTE_MEMPOOL_ALIGN_MASK)) &
+ RTE_MEMPOOL_ALIGN_MASK);
}
/*
@@ -498,7 +498,7 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
* cache-aligned
*/
private_data_size = (private_data_size +
- RTE_CACHE_LINE_MASK) & (~RTE_CACHE_LINE_MASK);
+ RTE_MEMPOOL_ALIGN_MASK) & (~RTE_MEMPOOL_ALIGN_MASK);
if (! rte_eal_has_hugepages()) {
/*
@@ -525,6 +525,7 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
* enough to hold mempool header and metadata plus mempool objects.
*/
mempool_size = MEMPOOL_HEADER_SIZE(mp, pg_num) + private_data_size;
+ mempool_size = RTE_ALIGN_CEIL(mempool_size, RTE_MEMPOOL_ALIGN);
if (vaddr == NULL)
mempool_size += (size_t)objsz.total_size * n;
@@ -580,6 +581,7 @@ rte_mempool_xmem_create(const char *name, unsigned n, unsigned elt_size,
/* calculate address of the first element for continuous mempool. */
obj = (char *)mp + MEMPOOL_HEADER_SIZE(mp, pg_num) +
private_data_size;
+ obj = RTE_PTR_ALIGN_CEIL(obj, RTE_MEMPOOL_ALIGN);
/* populate address translation fields. */
mp->pg_num = pg_num;
@@ -142,6 +142,12 @@ struct rte_mempool_objsz {
/** Mempool over one chunk of physically continuous memory */
#define MEMPOOL_PG_NUM_DEFAULT 1
+#ifndef RTE_MEMPOOL_ALIGN
+#define RTE_MEMPOOL_ALIGN RTE_CACHE_LINE_SIZE
+#endif
+
+#define RTE_MEMPOOL_ALIGN_MASK (RTE_MEMPOOL_ALIGN - 1)
+
/**
* Mempool object header structure
*