I've been looking a bit around this patch and something is not clear for me
about ODP_CONFIG_SEG_LEN_MIN.
Is is the minimal length of user data that you can fit in a segment, or data
altogether?
In the current implementation, we round up to SEG_LEN_MIN, and then round up
again to add head/tail room and align to the right buffer size.
Is this the expected behavior?
IMHO, it would make sense to use the space added by the round up to SEG_LEN_MIN
to use for headroom + footer
This would mean replacing the current
MAX(seg_len, SEG_LEN_MIN) + HEADROOM + FOOTROOM
by
MAX(seg_len + HEADROOM + FOOTROOM, SEG_LEN_MIN)
On x86 it probably won't mean much but on restricted memory H/W this can make
quite a difference.
Also, why do we need Headroom + Tail room in each segment and not to the total
length?
Adding it silently to the total length (still caped by BUF_MAX_LEN) would mean
that:
- we use less space on multiple segment packets
only 1 * (HR+TR) instead of MAX_SEGMENT * (HR+TR)
- users wouldn't have to do the current trick of subtracting the HR and TR from
the packet_len when allocating a packet.
I'm pretty sure 99% of new users will not think about doing this:
static const uint32_t packet_len = PACKET_BUF_LEN -
ODP_CONFIG_PACKET_HEADROOM -
ODP_CONFIG_PACKET_TAILROOM -
PACKET_TAILROOM_RESERVE;
On the down side, it will probably mean that smaller packets are more likely to
be fragmented but not more than a user would expect when picking a seg_len.
Nicolas
On 07/17/2015 01:42 PM, Bill Fischofer wrote:
> Nicolas opened this bug and assigned it to me and I see we cross-posted fix
> patches. My patch is at http://patches.opendataplane.org/patch/2298/ and is
> almost the same (I'm using <= rather than < which is slightly more efficient
> for the boundary case where seg_len == ODP_CONFIG_PACKET_SEG_LEN_MIN.
>
> On Fri, Jul 17, 2015 at 6:04 AM, Stuart Haslam <[email protected]
> <mailto:[email protected]>> wrote:
>
> On Fri, Jul 17, 2015 at 12:45:52PM +0200, Nicolas Morey-Chaisemartin
> wrote:
> > Fixes https://bugs.linaro.org/show_bug.cgi?id=1696
> >
> > Signed-off-by: Nicolas Morey-Chaisemartin <[email protected]
> <mailto:[email protected]>>
>
> Reviewed-by: Stuart Haslam <[email protected]
> <mailto:[email protected]>>
>
> > ---
> > platform/linux-generic/odp_pool.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/platform/linux-generic/odp_pool.c
> b/platform/linux-generic/odp_pool.c
> > index dcbdf07..0f84eb5 100644
> > --- a/platform/linux-generic/odp_pool.c
> > +++ b/platform/linux-generic/odp_pool.c
> > @@ -205,7 +205,7 @@ odp_pool_t odp_pool_create(const char *name,
> > tailroom = ODP_CONFIG_PACKET_TAILROOM;
> > buf_num = params->pkt.num;
> >
> > - seg_len = params->pkt.seg_len == 0 ?
> > + seg_len = params->pkt.seg_len <
> ODP_CONFIG_PACKET_SEG_LEN_MIN ?
> > ODP_CONFIG_PACKET_SEG_LEN_MIN :
> > (params->pkt.seg_len <=
> ODP_CONFIG_PACKET_SEG_LEN_MAX ?
> > params->pkt.seg_len :
> ODP_CONFIG_PACKET_SEG_LEN_MAX);
> > --
> > 2.4.5.3.g4915f6f
> >
> > _______________________________________________
> > lng-odp mailing list
> > [email protected] <mailto:[email protected]>
> > https://lists.linaro.org/mailman/listinfo/lng-odp
>
> --
> Stuart.
> _______________________________________________
> lng-odp mailing list
> [email protected] <mailto:[email protected]>
> https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
