Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_pool.c
line 28
@@ -296,7 +282,9 @@ static void init_buffers(pool_t *pool)
memset(buf_hdr, 0, (uintptr_t)data - (uintptr_t)buf_hdr);
/* Initialize buffer metadata */
- buf_hdr->index = i;
+ buf_hdr->index.u32 = 0;
+ buf_hdr->index.pool = pool->pool_idx;
+ buf_hdr->index.buffer = i;
Comment:
Since you're initializing `index.pool` and `index.buffer` there's no need to
set `index.u32` here.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> We originally had this index partitioning based on `ODP_CONFIG_POOLS`. Do we
> want to return to that here?
>
> If not, we at least need an `ODP_STATIC_ASSERT()` to ensure that
> `ODP_CONFIG_POOLS < 256` or else bad things will happen here.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> This routine can be optimized to:
>> ```
>> return ring->head == ring->tail;
>> ```
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Your invariant is the queue is empty when `head == tail` therefore the
>>> queue is full when `abs(tail - head) == mask`, so the correct calculation
>>> here is:
>>>
>>> `num = mask - abs(tail - head);`
>>>
>>> The effect is that a queue can only hold `size - 1` elements, otherwise you
>>> cannot distinguish between a full and an empty queue without another bit of
>>> metadata, which is a cost you're trying to avoid.
>>>
>>> This is somewhat problematic if the caller is trying to be "optimal" by
>>> specifying a power of two in the `size` parameter of the
>>> `odp_queue_param_t` passed to `odp_queue_create()`. For this reason we may
>>> wish to return a `max_size` of a power of 2 - 1 in `odp_queue_capability()`
>>> as part of this patch series.
>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>> This only works if `size` is a power of 2. Should be documented as such,
>>>> since this is an internal routine. In this case an `ODP_ASSERT(size ==
>>>> ROUNDUP_POWER2_U32(size))` for this requirement would be a useful
>>>> debugging aid.
>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>> should be `num = abs(tail - head);` to deal with wrap arounds, otherwise
>>>>> may be misinterpreted as overly large since it's `uint32_t`. Note that
>>>>> GCC and clang recognize `abs()` and treat it as a builtin, so there's no
>>>>> actual `stdlib.h` call here.
>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>> Extra blank line should be removed (nit).
https://github.com/Linaro/odp/pull/492#discussion_r169821150
updated_at 2018-02-22 09:32:40
