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 > <nmo...@kalray.eu <mailto:nmo...@kalray.eu>> 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 <stuart.has...@linaro.org >> <mailto:stuart.has...@linaro.org>> 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 <nmo...@kalray.eu >> <mailto:nmo...@kalray.eu>> >> >> Reviewed-by: Stuart Haslam <stuart.has...@linaro.org >> <mailto:stuart.has...@linaro.org>> >> >> >> > --- >> > 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 >> > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> > https://lists.linaro.org/mailman/listinfo/lng-odp >> >> -- >> Stuart. >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> > >
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
