Balasubramanian Manoharan(bala-manoharan) 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:
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_r138871330
updated_at 2017-09-14 11:56:03
