| Message ID | 1423869261-591-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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 --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);
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(-)