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

platform/linux-generic/odp_queue.c
@@ -263,7 +275,7 @@ static int queue_destroy(odp_queue_t handle)
                ODP_ERR("queue \"%s\" already destroyed\n", queue->s.name);
                return -1;
        }
-       if (queue->s.head != NULL) {
+       if (ring_st_is_empty(&queue->s.ring_st) == 0) {


Comment:
`if (!ring_st_is_empty(&queue->s.ring_st))` seems more natural here.

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

Reply via email to