Petri Savolainen(psavol) replied on github web page:
include/odp/api/spec/pool.h
line 28
@@ -214,6 +221,17 @@ typedef struct odp_pool_param_t {
defined by pool capability pkt.max_uarea_size.
Specify as 0 if no user area is needed. */
uint32_t uarea_size;
+
+ /** Minimum Headroom size in bytes. Minimum number of
+ bytes of space preceding packet data to reserve for
+ use as headroom. Each newly allocated packet from
+ the pool must have at least this much headroom.
+ Must not exceed the implementation defined
+ 'max_headroom' in odp_pool_capability().
+ @note If application does not require headroom it
+ can configure 'min_headroom' as zero and
+ implementation may or may not allocate headroom. */
+ uint32_t min_headroom;
Comment:
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_r138850667
updated_at 2017-09-14 10:08:20
