Petri Savolainen(psavol) replied on github web page:
platform/linux-generic/odp_pool.c
line 8
@@ -348,6 +348,10 @@ 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("Packet headroom size not supported.");
+ return ODP_POOL_INVALID;
+ }
headroom = CONFIG_PACKET_HEADROOM;
Comment:
Bala's implementation is correct. Currently, it's much more simpler to always
use the default headroom. I'd rather optimize odp-linux for simplicity and
performance than for saving memory. When memory usage is a worry, user can
rebuild with smaller CONFIG_PACKET_HEADROOM. Also due to data align / cacheline
round ups there will be always some headroom - so the potential memory saving
in question would be 1 or 2 cachelines per packet, which is not much.
> Petri Savolainen(psavol) wrote:
> The parameter name can be just "headroom".
>
> Text can be compressed a bit and made inline with documentation of the other
> params:
>
> "Minimum headroom size in bytes. Each newly allocated
> packet from the pool will have at least this much headroom.
> The maximum value is defined by pool capability pkt.max_headroom.
> Use zero if headroom is not needed."
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> To change this to:
>>
>> ```
>> headroom = params->pkt.min_headroom;
>> ```
>>
>> To reflect the API change being proposed in this PR all that's needed is a
>> one line update to the `packet_init()` routine in `odp_packet_internal.h`:
>>
>> Change
>>
>> ```
>> pkt_hdr->headroom = CONFIG_PACKET_HEADROOM;
>> ```
>> to
>>
>> ```
>> pkt_hdr->headroom = pkt_hdr->buf_hdr.pool_ptr->headroom;
>> ```
>>
>> However since `pool_ptr` is currently defined as `void *` a few more simple
>> infrastructure changes are needed to undo this "shortcut".
>>
>> 1. `pool_ptr` should be of type `pool_t *`, not `void *` in
>> `odp_buffer_internal.h`
>>
>> 2. `odp_buffer_internal.h` needs to `#include <odp_pool_internal.h>` to get
>> the definition of `pool_t`
>>
>> 3. Since there is now a circular dependency between `odp_buffer_internal.h`
>> and `odp_pool_internal.h` the `odp_forward_typedefs_internal.h` file has to
>> have the added line: `typedef struct pool_t pool_t;` added to it.
>>
>> With these changes pools will properly use the headroom requested in by the
>> new `min_headroom` field, as intended.
>>
>> However, as @psavol is in the process or reworking the packet internals
>> again I'll leave it to him as to whether he'd rather see this change before
>> or after he makes his other changes. An alternate approach would be for
>> pools to define an internal inline API that returns the configured headroom
>> for this pool so `packet_init()` doesn't have to rummage for it.
>>
>> My point remains is that if we're creating a reference implementation that
>> reference implementation should reflect how we intend the APIs to be used.
>> If we're now going to allow applications to specify headroom size on
>> `odp_pool_create()` then odp-linux should honor those requests.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Typo: If (should be capitalized) and require, not required.
>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>> 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.
>>>>> Balasubramanian Manoharan(bala-manoharan) 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.
>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>> setting headroom = CONFIG_PACKET_HEADROOM does not violate the API
>>>>>>> requirement and it also fits well with our linux-generic
>>>>>>> implementation.
>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>> Okay
>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) 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
>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) 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?
>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>> Okay
>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) wrote:
>>>>>>>>>>>>>>>> Okay.
>>>>>>>>>>>>>>>>> Petri Savolainen(psavol) 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);
>>>>>>>>>>>>>>>>>> Petri Savolainen(psavol) 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."
>>>>>>>>>>>>>>>>>>> Petri Savolainen(psavol) 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.
>>>>>>>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>>>>>>>>>>>>> We should not change this field since that's not required
>>>>>>>>>>>>>>>>>>>> for the new feature to be functional.
>>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) 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.
>>>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) 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`.
>>>>>>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) 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.
>>>>>>>>>>>>>>>>>>>>>>>>>> Balasubramanian Manoharan(bala-manoharan) 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_r138852946
updated_at 2017-09-14 10:19:29
