Petri Savolainen(psavol) replied on github web page:
include/odp/api/spec/pool.h
@@ -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:
Sorry, forgot to mention the rationale for "headroom" vs "min_headroom". Plain
"headroom" is inline with the other parameter names in the struct
(num/len/max_len/seg_len/uarea_size). Those describe in documentation text how
implementation may round up things, but the variable names are not min_len,
min_seg_len, min_uarea_size, etc
For example, seg_len spec is very close to headroom spec. Application requires
at least 'seg_len' bytes but implementation is free to round up:
"Minimum number of packet data bytes that are stored
in the first segment of a packet. The maximum value
is defined by pool capability pkt.max_seg_len.
Use 0 for default. "
uint32_t seg_len;
So, please rename to "headroom".
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> 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_r139083233
updated_at 2017-09-15 07:44:19
