Petri Savolainen(psavol) replied on github web page: platform/linux-generic/odp_pool.c line 8 @@ -348,6 +348,10 @@ static odp_pool_t pool_create(const char *name, odp_pool_param_t *params, break; case ODP_POOL_PACKET: + if (params->pkt.min_headroom > CONFIG_PACKET_HEADROOM) { + ODP_ERR("Packet headroom size not supported."); + return ODP_POOL_INVALID; + } headroom = CONFIG_PACKET_HEADROOM;
Comment: 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_r138852946 updated_at 2017-09-14 10:19:29