Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_pool.c
@@ -348,6 +348,11 @@ static odp_pool_t pool_create(const char *name,
odp_pool_param_t *params,
break;
case ODP_POOL_PACKET:
+ if (params->pkt.min_headroom > CONFIG_PACKET_HEADROOM) {
+ ODP_ERR("Headroom size not supported");
+ return ODP_POOL_INVALID;
+ }
+
headroom = CONFIG_PACKET_HEADROOM;
Comment:
I see no reason not to change it now, since linux-generic can support zero
headroom if that's what the application requests. Besides, this is supposed to
be a reference implementation which shows how we intend APIs to behave. We
intend lower headroom requests to be honored, so we should do so here.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> This PR helps implementation which have the ability to maintain zero headroom
> size and our linux-generic implementation does not violate the API spec. We
> can change linux-generic in future also.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> True, but I don't see why we wouldn't want to honor the application's
>> requested headroom size. Setting this unconditionally this way is just
>> ignoring the request and if we're going to do that there's no reason to have
>> this PR at all.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> setting headroom = CONFIG_PACKET_HEADROOM does not violate the API
>>> requirement and it also fits well with our linux-generic implementation.
>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>> Okay
>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>> Since we are now exposing max_headroom we need to fail the pool creating
>>>>> if the headroom size requested could not be supported.
>>>>>
>>>>> pkt.min_headroom is odp_pool_param_init() is set to
>>>>> CONFIG_PACKET_HEADROOM already. Its is the second part of this patch.
>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>> I'd still like to see a usage note somewhere indicating that setting
>>>>>> `min_headroom` to 0 means that the application does not require any
>>>>>> packet headroom so the implementation may optimize pool storage usage
>>>>>> accordingly.
>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>> It's not clear why we're making this change if we don't intend to do
>>>>>>> anything with it. Shouldn't this be:
>>>>>>> `headroom = params->pkt.min_headroom;` ?
>>>>>>>
>>>>>>> To make this convention work correctly, `odp_pool_param_init()` needs
>>>>>>> to be updated to set `pkt.min_headroom` to `CONFIG_PACKET_HEADROOM` so
>>>>>>> that existing applications behave unchanged, which you've already done.
>>>>>>> So this looks like an omission?
>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>> Typo: at least
>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>> @psavol CONFIG_PACKET_HEADROOM is set as default in
>>>>>>>>> odp_pool_params_init() function. So it should not break any existing
>>>>>>>>> implementation.
>>>>>>>>>
>>>>>>>>> Also do you mean to set CONFIG_PACKET_HEADROOM as the maximum
>>>>>>>>> headroom in capability.
>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>> @psavol I'm not sure what you're saying here. This PR has two
>>>>>>>>>> commits: One for the API changes and one to change the odp-linux
>>>>>>>>>> pool implementation to reflect these new API changes. Are you
>>>>>>>>>> suggesting that we ignore the API changes?
>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>> Okay
>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>> Okay.
>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>> It's better not to change headroom implementation in this set but
>>>>>>>>>>>>> just implement the capability == return the current headroom as
>>>>>>>>>>>>> the max. Other parts of the code assume that init time headroom
>>>>>>>>>>>>> is constant:
>>>>>>>>>>>>>
>>>>>>>>>>>>> return CONFIG_PACKET_HEADROOM + (head - base);
>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>> This is not hint, but minimum. Implementation may round up, but
>>>>>>>>>>>>>> must not round down. When implementation has fixed headroom, it
>>>>>>>>>>>>>> should set max capability to that.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Simply:
>>>>>>>>>>>>>> "Minimum headroom size in bytes. Every newly allocated packet
>>>>>>>>>>>>>> must have at least this much headroom. Must not exceed
>>>>>>>>>>>>>> max_headroom packet pool capability."
>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>> This should refer to the new headroom pool parameter: "This is
>>>>>>>>>>>>>>> the maximum value for packet pool headroom param ..." So, it's
>>>>>>>>>>>>>>> the maximum application may ask, implementation may still round
>>>>>>>>>>>>>>> up from there.
>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>> We should not change this field since that's not required for
>>>>>>>>>>>>>>>> the new feature to be functional.
>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>> In that case the naming "default_headroom" makes more sense
>>>>>>>>>>>>>>>>> than "min_headroom". Maybe we can leave it as "min_headroom"
>>>>>>>>>>>>>>>>> in this release.
>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>> Okay
>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>> Just to elaborate, it's always valid for an application to
>>>>>>>>>>>>>>>>>>> request 0 headroom, so there's no minimum in that sense.
>>>>>>>>>>>>>>>>>>> Whether that results in packets having 0 headroom is up to
>>>>>>>>>>>>>>>>>>> the implementation since implementations are free to round
>>>>>>>>>>>>>>>>>>> up application requests to something larger. So I agree
>>>>>>>>>>>>>>>>>>> with Petri that this particular wording change probably
>>>>>>>>>>>>>>>>>>> isn't needed. It simply says that in the absence of any
>>>>>>>>>>>>>>>>>>> specific requests from the application, this is the minimum
>>>>>>>>>>>>>>>>>>> amount of headroom that applications can expect to find in
>>>>>>>>>>>>>>>>>>> every newly received or allocated packet.
>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>> Ok, but odp-linux really has no built-in maximum for this
>>>>>>>>>>>>>>>>>>>> so we shouldn't impose one artificially. The effective
>>>>>>>>>>>>>>>>>>>> max_headroom would be something like
>>>>>>>>>>>>>>>>>>>> `CONFIG_PACKET_MAX_SEG_LEN - 1`.
>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>> We had a requirement from a customer for 256B headroom
>>>>>>>>>>>>>>>>>>>>> and I am not aware of any request bigger than this hence
>>>>>>>>>>>>>>>>>>>>> this magic number.
>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>> The maximum headroom supported will be equal to the
>>>>>>>>>>>>>>>>>>>>>> maximum segment size supported by the platform and we
>>>>>>>>>>>>>>>>>>>>>> can discuss in the ARCH call and if required we can
>>>>>>>>>>>>>>>>>>>>>> indicate either zero or UINT32_MAX as value when the
>>>>>>>>>>>>>>>>>>>>>> implementation does not have any limitation.
>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>> Trying to specify a headroom >
>>>>>>>>>>>>>>>>>>>>>>> `CONFIG_PACKET_MAX_HEADROOM` should fail the call, as
>>>>>>>>>>>>>>>>>>>>>>> noted earlier.
>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>> Any particular reason for this "magic number"?
>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> If we have defined min and max values, then values
>>>>>>>>>>>>>>>>>>>>>>>>> outside this range should result in the
>>>>>>>>>>>>>>>>>>>>>>>>> `odp_pool_create()` call failing, not being silently
>>>>>>>>>>>>>>>>>>>>>>>>> ignored. Such is the case for other limits so
>>>>>>>>>>>>>>>>>>>>>>>>> headroom should be no different.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> However, as noted previously, I don't think a
>>>>>>>>>>>>>>>>>>>>>>>>> `min_headroom` is particularly meaningful/useful so
>>>>>>>>>>>>>>>>>>>>>>>>> this should be reworded. We should definitely include
>>>>>>>>>>>>>>>>>>>>>>>>> the use case that a `min_headroom` of 0 indicates
>>>>>>>>>>>>>>>>>>>>>>>>> that the application does not require headroom so the
>>>>>>>>>>>>>>>>>>>>>>>>> implementation may omit this to conserve storage.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> It may also be appropriate to revisit the question of
>>>>>>>>>>>>>>>>>>>>>>>>> headroom and packet alignment. The current
>>>>>>>>>>>>>>>>>>>>>>>>> linux-generic pool allocation structure ensures that
>>>>>>>>>>>>>>>>>>>>>>>>> the IP header will be misaligned due to the presence
>>>>>>>>>>>>>>>>>>>>>>>>> of a 14-byte Ethernet header, meaning
>>>>>>>>>>>>>>>>>>>>>>>>> `odp_packet_l3_ptr()` will return an address that is
>>>>>>>>>>>>>>>>>>>>>>>>> 2-byte rather than 4-byte aligned because we're now
>>>>>>>>>>>>>>>>>>>>>>>>> aligning `data[offset]` rather than `data[0]`. I
>>>>>>>>>>>>>>>>>>>>>>>>> realize this is not directly related to this PR, but
>>>>>>>>>>>>>>>>>>>>>>>>> since we're making changes in this area something we
>>>>>>>>>>>>>>>>>>>>>>>>> should make clear.
>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> Do we have to worry about implementations that have
>>>>>>>>>>>>>>>>>>>>>>>>>> no fixed `max_headroom` or must these
>>>>>>>>>>>>>>>>>>>>>>>>>> implementations make up a number or use something
>>>>>>>>>>>>>>>>>>>>>>>>>> like `UINT32_MAX` here? The effective maximum
>>>>>>>>>>>>>>>>>>>>>>>>>> headroom is going to be the `odp_pool_capability()`
>>>>>>>>>>>>>>>>>>>>>>>>>> `pkt.max_seg_len` field, which uses the 0 = limited
>>>>>>>>>>>>>>>>>>>>>>>>>> only by available memory convention.
>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>> Given how we intend to use this. `default_headroom`
>>>>>>>>>>>>>>>>>>>>>>>>>>> would seem to be a better name here than
>>>>>>>>>>>>>>>>>>>>>>>>>>> `min_headroom`. The actual minimum headroom is
>>>>>>>>>>>>>>>>>>>>>>>>>>> always going to be zero. Every implementation must
>>>>>>>>>>>>>>>>>>>>>>>>>>> support zero headroom since
>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_packet_push_head(pkt,
>>>>>>>>>>>>>>>>>>>>>>>>>>> odp_packet_headroom(pkt))` always results in
>>>>>>>>>>>>>>>>>>>>>>>>>>> `odp_packet_headroom(pkt)` returning zero anyway.
https://github.com/Linaro/odp/pull/152#discussion_r137130148
updated_at 2017-09-06 12:56:35
