Sorry about that. It works because buffer_alloc will allocate another segment And because buffer_alloc checks that len + HR + TR <= seg_size * MAX_SEG, it always works.
On 07/21/2015 08:27 AM, Nicolas Morey-Chaisemartin wrote: > Does that not apply to the len parameter too? > if the user expects to have a buffer of N bytes, shouldn't the internal > buffer size be N + Headroom + Tailroom? > > I don't think this is the case today. > If a user asks for seg_len=1598 and len = 6656 ((1598 + 66 + 0) * 4), he > will end up with a buffer just large enough for its > data but with no headroom at all. > > On 07/21/2015 12:42 AM, Bill Fischofer wrote: >> The tests are just approximations. >> >> ODP_CONFIG_SEG_LEN_MIN is supposed to be the implementation's minimum >> supportable segment size, independent of how that segment is used. We >> really want to change all of these from #defines to APIs so that they can be >> more portable, but they're #defines for now. >> >> The seg_len specified on the odp_pool_param_t is supposed to specify the >> minimum segment size that the application wishes to see for its data, hence >> headroom and tailroom are not part of that. >> >> On Mon, Jul 20, 2015 at 11:09 AM, Nicolas Morey-Chaisemartin >> <[email protected]> wrote: >> >> 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]> >>> >>> >>> > --- >>> > 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
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
