diff mbox

[PATCHv2,4/4] linux-generic: pools: cleanup to reflect new pool parameters

Message ID 1423869261-591-1-git-send-email-bill.fischofer@linaro.org
State New
Headers show

Commit Message

Bill Fischofer Feb. 13, 2015, 11:14 p.m. UTC
Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
---
 platform/linux-generic/include/odp_pool_internal.h |  1 +
 platform/linux-generic/odp_pool.c                  | 66 +++++++++++-----------
 2 files changed, 34 insertions(+), 33 deletions(-)

Comments

Maxim Uvarov Feb. 17, 2015, 2:33 p.m. UTC | #1
On 02/17/2015 05:29 PM, Savolainen, Petri (NSN - FI/Espoo) wrote:
>
>> -----Original Message-----
>> From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
>> bounces@lists.linaro.org] On Behalf Of ext Bill Fischofer
>> Sent: Saturday, February 14, 2015 1:14 AM
>> To: lng-odp@lists.linaro.org
>> Subject: [lng-odp] [PATCHv2 4/4] linux-generic: pools: cleanup to reflect
>> new pool parameters
>>
>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
>> ---
>>   platform/linux-generic/include/odp_pool_internal.h |  1 +
>>   platform/linux-generic/odp_pool.c                  | 66 +++++++++++------
>> -----
>>   2 files changed, 34 insertions(+), 33 deletions(-)
>>
>> diff --git a/platform/linux-generic/include/odp_pool_internal.h
>> b/platform/linux-generic/include/odp_pool_internal.h
>> index c5354e5..0c0c0be 100644
>> --- a/platform/linux-generic/include/odp_pool_internal.h
>> +++ b/platform/linux-generic/include/odp_pool_internal.h
>> @@ -119,6 +119,7 @@ struct pool_entry_s {
>>   	odp_atomic_u64_t        blkempty;
>>   	odp_atomic_u64_t        high_wm_count;
>>   	odp_atomic_u64_t        low_wm_count;
>> +	uint32_t                buf_num;
>>   	uint32_t                seg_size;
>>   	uint32_t                high_wm;
>>   	uint32_t                low_wm;
>> diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-
>> generic/odp_pool.c
>> index c07392f..1fe0c5f 100644
>> --- a/platform/linux-generic/odp_pool.c
>> +++ b/platform/linux-generic/odp_pool.c
>> @@ -136,13 +136,9 @@ odp_pool_t odp_pool_create(const char *name,
>>   		ODP_CACHE_LINE_SIZE_ROUNDUP(init_params->udata_size) :
>>   		0;
>>
>> -	uint32_t blk_size, buf_stride;
>> -	uint32_t buf_align;
>> -
>> -	if (params->type == ODP_POOL_PACKET)
>> -		buf_align = 0;
>> -	else
>> -		buf_align = params->buf.align;
>> +	uint32_t blk_size, buf_stride, buf_num;
>> +	uint32_t buf_align =
>> +		params->type == ODP_POOL_BUFFER ? params->buf.align : 0;
>>
>>   	/* Validate requested buffer alignment */
>>   	if (buf_align > ODP_CONFIG_BUFFER_ALIGN_MAX ||
>> @@ -158,35 +154,41 @@ odp_pool_t odp_pool_create(const char *name,
>>   	/* Calculate space needed for buffer blocks and metadata */
>>   	switch (params->type) {
>>   	case ODP_POOL_BUFFER:
>> -	case ODP_POOL_TIMEOUT:
>> +		buf_num  = params->buf.num;
>>   		blk_size = params->buf.size;
>>
>>   		/* Optimize small raw buffers */
>>   		if (blk_size > ODP_MAX_INLINE_BUF || params->buf.align != 0)
>>   			blk_size = ODP_ALIGN_ROUNDUP(blk_size, buf_align);
>>
>> -		buf_stride = params->type == ODP_POOL_BUFFER ?
>> -			sizeof(odp_buffer_hdr_stride) :
>> -			sizeof(odp_timeout_hdr_stride);
>> +		buf_stride = sizeof(odp_buffer_hdr_stride);
>>   		break;
>>
>>   	case ODP_POOL_PACKET:
>>   		headroom = ODP_CONFIG_PACKET_HEADROOM;
>>   		tailroom = ODP_CONFIG_PACKET_TAILROOM;
>> -		unseg = params->pkt.seg_len > ODP_CONFIG_PACKET_BUF_LEN_MAX;
>> +		buf_num = params->pkt.num;
>> +		uint32_t seg_len = params->pkt.seg_len == 0 ?
>> +			ODP_CONFIG_PACKET_SEG_LEN_MIN :

can all uint32_t seg_len, uint32_t pkt_len, be simple len and defined at 
the top of the function?

Maxim.

> Similar to the next line, this needs headroom/tailroom added. In other words,
>
> if (params->pkt.seg_len == 0)
> 	params->pkt.seg_len = ODP_CONFIG_PACKET_SEG_LEN_MIN;
>
>
>> +			headroom + params->pkt.seg_len + tailroom;
>> +		uint32_t pkt_len = params->pkt.len <= seg_len ?
>> +			seg_len : params->pkt.len;
>> +		unseg = pkt_len > ODP_CONFIG_PACKET_BUF_LEN_MAX;
> ODP_CONFIG_PACKET_BUF_LEN_MAX defines the absolute maximum number of bytes that can be stored into a packet (over all segments). The pool create should fail if pkt_len (+headroom+tailroom) > ODP_CONFIG_PACKET_BUF_LEN_MAX.
>
> With this code, "unseg" would be true if params->pkt.len or params->pkt.seg_len > 6 * 1664 ??
>
>>   		if (unseg)
>> -			blk_size = ODP_ALIGN_ROUNDUP(
>> -				headroom + params->pkt.seg_len + tailroom,
>> -				buf_align);
>> +			blk_size = ODP_ALIGN_ROUNDUP(seg_len,
>> +				ODP_CONFIG_BUFFER_ALIGN_MIN);
>>   		else
>> -			blk_size = ODP_ALIGN_ROUNDUP(
>> -				headroom + params->pkt.seg_len + tailroom,
>> +			blk_size = ODP_ALIGN_ROUNDUP(pkt_len,
>>   				ODP_CONFIG_PACKET_SEG_LEN_MIN);
>
> Not sure if the unseg logic above makes sense. Would it be easier to consider all packets "segmented". Depending on packet data length and pool setup, most or even all packets in the pool could have only one segment.
>
> -Petri
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
Bill Fischofer Feb. 17, 2015, 6:40 p.m. UTC | #2
On Tue, Feb 17, 2015 at 10:29 PM, Savolainen, Petri (NSN - FI/Espoo) <
petri.savolainen@nsn.com> wrote:

>
>
> > -----Original Message-----
> > From: lng-odp-bounces@lists.linaro.org [mailto:lng-odp-
> > bounces@lists.linaro.org] On Behalf Of ext Bill Fischofer
> > Sent: Saturday, February 14, 2015 1:14 AM
> > To: lng-odp@lists.linaro.org
> > Subject: [lng-odp] [PATCHv2 4/4] linux-generic: pools: cleanup to reflect
> > new pool parameters
> >
> > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
> > ---
> >  platform/linux-generic/include/odp_pool_internal.h |  1 +
> >  platform/linux-generic/odp_pool.c                  | 66
> +++++++++++------
> > -----
> >  2 files changed, 34 insertions(+), 33 deletions(-)
> >
> > diff --git a/platform/linux-generic/include/odp_pool_internal.h
> > b/platform/linux-generic/include/odp_pool_internal.h
> > index c5354e5..0c0c0be 100644
> > --- a/platform/linux-generic/include/odp_pool_internal.h
> > +++ b/platform/linux-generic/include/odp_pool_internal.h
> > @@ -119,6 +119,7 @@ struct pool_entry_s {
> >       odp_atomic_u64_t        blkempty;
> >       odp_atomic_u64_t        high_wm_count;
> >       odp_atomic_u64_t        low_wm_count;
> > +     uint32_t                buf_num;
> >       uint32_t                seg_size;
> >       uint32_t                high_wm;
> >       uint32_t                low_wm;
> > diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-
> > generic/odp_pool.c
> > index c07392f..1fe0c5f 100644
> > --- a/platform/linux-generic/odp_pool.c
> > +++ b/platform/linux-generic/odp_pool.c
> > @@ -136,13 +136,9 @@ odp_pool_t odp_pool_create(const char *name,
> >               ODP_CACHE_LINE_SIZE_ROUNDUP(init_params->udata_size) :
> >               0;
> >
> > -     uint32_t blk_size, buf_stride;
> > -     uint32_t buf_align;
> > -
> > -     if (params->type == ODP_POOL_PACKET)
> > -             buf_align = 0;
> > -     else
> > -             buf_align = params->buf.align;
> > +     uint32_t blk_size, buf_stride, buf_num;
> > +     uint32_t buf_align =
> > +             params->type == ODP_POOL_BUFFER ? params->buf.align : 0;
> >
> >       /* Validate requested buffer alignment */
> >       if (buf_align > ODP_CONFIG_BUFFER_ALIGN_MAX ||
> > @@ -158,35 +154,41 @@ odp_pool_t odp_pool_create(const char *name,
> >       /* Calculate space needed for buffer blocks and metadata */
> >       switch (params->type) {
> >       case ODP_POOL_BUFFER:
> > -     case ODP_POOL_TIMEOUT:
> > +             buf_num  = params->buf.num;
> >               blk_size = params->buf.size;
> >
> >               /* Optimize small raw buffers */
> >               if (blk_size > ODP_MAX_INLINE_BUF || params->buf.align !=
> 0)
> >                       blk_size = ODP_ALIGN_ROUNDUP(blk_size, buf_align);
> >
> > -             buf_stride = params->type == ODP_POOL_BUFFER ?
> > -                     sizeof(odp_buffer_hdr_stride) :
> > -                     sizeof(odp_timeout_hdr_stride);
> > +             buf_stride = sizeof(odp_buffer_hdr_stride);
> >               break;
> >
> >       case ODP_POOL_PACKET:
> >               headroom = ODP_CONFIG_PACKET_HEADROOM;
> >               tailroom = ODP_CONFIG_PACKET_TAILROOM;
> > -             unseg = params->pkt.seg_len >
> ODP_CONFIG_PACKET_BUF_LEN_MAX;
> > +             buf_num = params->pkt.num;
> > +             uint32_t seg_len = params->pkt.seg_len == 0 ?
> > +                     ODP_CONFIG_PACKET_SEG_LEN_MIN :
>
> Similar to the next line, this needs headroom/tailroom added. In other
> words,
>

In the current definition of config.h, the ODP_CONFIG_PACKET_SEG_LEN_MIN
includes the headroom and tailroom, which is why it isn't added twice
here.  I can change the code and the config.h value to standardize that if
that's preferable.

>
> if (params->pkt.seg_len == 0)
>         params->pkt.seg_len = ODP_CONFIG_PACKET_SEG_LEN_MIN;
>
>
> > +                     headroom + params->pkt.seg_len + tailroom;
> > +             uint32_t pkt_len = params->pkt.len <= seg_len ?
> > +                     seg_len : params->pkt.len;
> > +             unseg = pkt_len > ODP_CONFIG_PACKET_BUF_LEN_MAX;
>
> ODP_CONFIG_PACKET_BUF_LEN_MAX defines the absolute maximum number of bytes
> that can be stored into a packet (over all segments). The pool create
> should fail if pkt_len (+headroom+tailroom) >
> ODP_CONFIG_PACKET_BUF_LEN_MAX.

With this code, "unseg" would be true if params->pkt.len or
> params->pkt.seg_len > 6 * 1664 ??
>

That's correct.  The segmentation support is only applicable up to
ODP_CONFIG_PACKET_BUF_LEN_MAX.  Beyond that the pool will be unsegmented
and it's assumed that the application has its own reasons for creating
pools with super-sized packets. This was originally put into place for
crypto which was allocating pools with 32K blocks.  Given that we can
support unsegmented pools of unlimited size I don't see a need to restrict
this.


>
> >
> >               if (unseg)
> > -                     blk_size = ODP_ALIGN_ROUNDUP(
> > -                             headroom + params->pkt.seg_len + tailroom,
> > -                             buf_align);
> > +                     blk_size = ODP_ALIGN_ROUNDUP(seg_len,
> > +                             ODP_CONFIG_BUFFER_ALIGN_MIN);
> >               else
> > -                     blk_size = ODP_ALIGN_ROUNDUP(
> > -                             headroom + params->pkt.seg_len + tailroom,
> > +                     blk_size = ODP_ALIGN_ROUNDUP(pkt_len,
> >                               ODP_CONFIG_PACKET_SEG_LEN_MIN);
>
>
> Not sure if the unseg logic above makes sense. Would it be easier to
> consider all packets "segmented". Depending on packet data length and pool
> setup, most or even all packets in the pool could have only one segment.
>

Within this code, unsegmented means "will never have more than one
segment".  All packets have a segcount of at least 1.


>
> -Petri
>
>
diff mbox

Patch

diff --git a/platform/linux-generic/include/odp_pool_internal.h b/platform/linux-generic/include/odp_pool_internal.h
index c5354e5..0c0c0be 100644
--- a/platform/linux-generic/include/odp_pool_internal.h
+++ b/platform/linux-generic/include/odp_pool_internal.h
@@ -119,6 +119,7 @@  struct pool_entry_s {
 	odp_atomic_u64_t        blkempty;
 	odp_atomic_u64_t        high_wm_count;
 	odp_atomic_u64_t        low_wm_count;
+	uint32_t                buf_num;
 	uint32_t                seg_size;
 	uint32_t                high_wm;
 	uint32_t                low_wm;
diff --git a/platform/linux-generic/odp_pool.c b/platform/linux-generic/odp_pool.c
index c07392f..1fe0c5f 100644
--- a/platform/linux-generic/odp_pool.c
+++ b/platform/linux-generic/odp_pool.c
@@ -136,13 +136,9 @@  odp_pool_t odp_pool_create(const char *name,
 		ODP_CACHE_LINE_SIZE_ROUNDUP(init_params->udata_size) :
 		0;
 
-	uint32_t blk_size, buf_stride;
-	uint32_t buf_align;
-
-	if (params->type == ODP_POOL_PACKET)
-		buf_align = 0;
-	else
-		buf_align = params->buf.align;
+	uint32_t blk_size, buf_stride, buf_num;
+	uint32_t buf_align =
+		params->type == ODP_POOL_BUFFER ? params->buf.align : 0;
 
 	/* Validate requested buffer alignment */
 	if (buf_align > ODP_CONFIG_BUFFER_ALIGN_MAX ||
@@ -158,35 +154,41 @@  odp_pool_t odp_pool_create(const char *name,
 	/* Calculate space needed for buffer blocks and metadata */
 	switch (params->type) {
 	case ODP_POOL_BUFFER:
-	case ODP_POOL_TIMEOUT:
+		buf_num  = params->buf.num;
 		blk_size = params->buf.size;
 
 		/* Optimize small raw buffers */
 		if (blk_size > ODP_MAX_INLINE_BUF || params->buf.align != 0)
 			blk_size = ODP_ALIGN_ROUNDUP(blk_size, buf_align);
 
-		buf_stride = params->type == ODP_POOL_BUFFER ?
-			sizeof(odp_buffer_hdr_stride) :
-			sizeof(odp_timeout_hdr_stride);
+		buf_stride = sizeof(odp_buffer_hdr_stride);
 		break;
 
 	case ODP_POOL_PACKET:
 		headroom = ODP_CONFIG_PACKET_HEADROOM;
 		tailroom = ODP_CONFIG_PACKET_TAILROOM;
-		unseg = params->pkt.seg_len > ODP_CONFIG_PACKET_BUF_LEN_MAX;
+		buf_num = params->pkt.num;
+		uint32_t seg_len = params->pkt.seg_len == 0 ?
+			ODP_CONFIG_PACKET_SEG_LEN_MIN :
+			headroom + params->pkt.seg_len + tailroom;
+		uint32_t pkt_len = params->pkt.len <= seg_len ?
+			seg_len : params->pkt.len;
+		unseg = pkt_len > ODP_CONFIG_PACKET_BUF_LEN_MAX;
 
 		if (unseg)
-			blk_size = ODP_ALIGN_ROUNDUP(
-				headroom + params->pkt.seg_len + tailroom,
-				buf_align);
+			blk_size = ODP_ALIGN_ROUNDUP(seg_len,
+				ODP_CONFIG_BUFFER_ALIGN_MIN);
 		else
-			blk_size = ODP_ALIGN_ROUNDUP(
-				headroom + params->pkt.seg_len + tailroom,
+			blk_size = ODP_ALIGN_ROUNDUP(pkt_len,
 				ODP_CONFIG_PACKET_SEG_LEN_MIN);
 
-		buf_stride = params->type == ODP_POOL_PACKET ?
-			sizeof(odp_packet_hdr_stride) :
-			sizeof(odp_any_hdr_stride);
+		buf_stride = sizeof(odp_packet_hdr_stride);
+		break;
+
+	case ODP_POOL_TIMEOUT:
+		blk_size = 0;
+		buf_num = params->tmo.num;
+		buf_stride = sizeof(odp_timeout_hdr_stride);
 		break;
 
 	default:
@@ -194,7 +196,7 @@  odp_pool_t odp_pool_create(const char *name,
 	}
 
 	/* Validate requested number of buffers against addressable limits */
-	if (params->buf.num >
+	if (buf_num >
 	    (ODP_BUFFER_MAX_BUFFERS / (buf_stride / ODP_CACHE_LINE_SIZE)))
 		return ODP_POOL_INVALID;
 
@@ -231,14 +233,15 @@  odp_pool_t odp_pool_create(const char *name,
 			block_size = 0;
 			pool->s.buf_align = blk_size == 0 ? 0 : sizeof(void *);
 		} else {
-			block_size = params->buf.num * blk_size;
+			block_size = buf_num * blk_size;
 			pool->s.buf_align = buf_align;
 		}
 
 		pad_size = ODP_CACHE_LINE_SIZE_ROUNDUP(block_size) - block_size;
-		mdata_size = params->buf.num * buf_stride;
-		udata_size = params->buf.num * udata_stride;
+		mdata_size = buf_num * buf_stride;
+		udata_size = buf_num * udata_stride;
 
+		pool->s.buf_num   = buf_num;
 		pool->s.pool_size = ODP_PAGE_SIZE_ROUNDUP(block_size +
 							  pad_size +
 							  mdata_size +
@@ -375,8 +378,8 @@  odp_pool_t odp_pool_create(const char *name,
 		pool->s.tailroom = tailroom;
 
 		/* Watermarks are hard-coded for now to control caching */
-		pool->s.high_wm = params->buf.num / 2;
-		pool->s.low_wm = params->buf.num / 4;
+		pool->s.high_wm = buf_num / 2;
+		pool->s.low_wm  = buf_num / 4;
 
 		pool_hdl = pool->s.pool_hdl;
 		break;
@@ -417,10 +420,7 @@  int odp_pool_info(odp_pool_t pool_hdl, odp_pool_info_t *info)
 	info->name = pool->s.name;
 	info->shm  = pool->s.flags.user_supplied_shm ?
 		pool->s.pool_shm : ODP_SHM_INVALID;
-	info->params.buf.size  = pool->s.params.buf.size;
-	info->params.buf.align = pool->s.params.buf.align;
-	info->params.buf.num   = pool->s.params.buf.num;
-	info->params.type      = pool->s.params.type;
+	info->params = pool->s.params;
 
 	return 0;
 }
@@ -446,7 +446,7 @@  int odp_pool_destroy(odp_pool_t pool_hdl)
 	flush_cache(&local_cache[pool_id], &pool->s);
 
 	/* Call fails if pool has allocated buffers */
-	if (odp_atomic_load_u32(&pool->s.bufcount) < pool->s.params.buf.num) {
+	if (odp_atomic_load_u32(&pool->s.bufcount) < pool->s.buf_num) {
 		POOL_UNLOCK(&pool->s.lock);
 		return -1;
 	}
@@ -591,10 +591,10 @@  void odp_pool_print(odp_pool_t pool_hdl)
 	ODP_DBG(" tailroom        %u\n",  pool->s.tailroom);
 	ODP_DBG(" buf align       %u requested, %u used\n",
 		pool->s.params.buf.align, pool->s.buf_align);
-	ODP_DBG(" num bufs        %u\n",  pool->s.params.buf.num);
+	ODP_DBG(" num bufs        %u\n",  pool->s.buf_num);
 	ODP_DBG(" bufs available  %u %s\n", bufcount,
 		pool->s.low_wm_assert ? " **low wm asserted**" : "");
-	ODP_DBG(" bufs in use     %u\n",  pool->s.params.buf.num - bufcount);
+	ODP_DBG(" bufs in use     %u\n",  pool->s.buf_num - bufcount);
 	ODP_DBG(" buf allocs      %lu\n", bufallocs);
 	ODP_DBG(" buf frees       %lu\n", buffrees);
 	ODP_DBG(" buf empty       %lu\n", bufempty);