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
