Petri Savolainen(psavol) replied on github web page:
platform/linux-generic/odp_pool.c
line 16
@@ -853,7 +857,8 @@ int odp_pool_capability(odp_pool_capability_t *capa)
capa->pkt.max_pools = ODP_CONFIG_POOLS;
capa->pkt.max_len = CONFIG_PACKET_MAX_SEGS * max_seg_len;
capa->pkt.max_num = CONFIG_POOL_MAX_NUM;
- capa->pkt.min_headroom = CONFIG_PACKET_HEADROOM;
+ capa->pkt.min_headroom = 0;
Comment:
Actually, min_headroom is still CONFIG_PACKET_HEADROOM since we always round up
to that.
> Petri Savolainen(psavol) wrote:
> 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_r138853139
updated_at 2017-09-14 10:20:28
