Bill Fischofer(Bill-Fischofer-Linaro) 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:
Given Petri's other comments, I agree here. Sounds like we just want to make
these additional params available in the spec but ignore them in odp-linux. We
can revisit this in the cloud-dev branch.
> Balasubramanian Manoharan(bala-manoharan) wrote:
> I prefer naming it as min_headroom is coz the value given by application is
> not directly configured and implementation may roundup the value and hence
> the name "min_headroom"
>> Petri Savolainen(psavol) wrote:
>> 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_r138877158
updated_at 2017-09-14 12:23:43
