Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:

platform/linux-generic/odp_queue.c
line 103
@@ -192,6 +201,9 @@ static odp_queue_t queue_create(const char *name,
                param = &default_param;
        }
 
+       if (param->size > CONFIG_QUEUE_SIZE)


Comment:
Change to `if (param->size >= CONFIG_QUEUE_SIZE)` to handle the effective queue 
capacity. The user-supplied `size` should then be set to 
`ROUNDUP_POWER2_U32(size) - 1` for the masking to work properly.

> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Same comment here as for plain queues.


>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> As noted earlier, due to "losing" one entry to distinguish queue empty/full, 
>> this should be returned as `CONFIG_QUEUE_SIZE - 1`, and we also need to 
>> ensure that `CONFIG_QUEUE_SIZE` is itself a power of 2.


>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> 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_r169822191
updated_at 2018-02-22 09:32:40

Reply via email to