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

line 92
@@ -143,8 +150,10 @@ static int queue_capability(odp_queue_capability_t *capa)
        capa->max_sched_groups  = sched_fn->num_grps();
        capa->sched_prios       = odp_schedule_num_prio();
        capa->plain.max_num     = capa->max_queues;
+       capa->plain.max_size    = CONFIG_QUEUE_SIZE;

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).
updated_at 2018-02-22 09:32:40

Reply via email to