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

line 95
@@ -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;
        capa->plain.nonblocking = ODP_BLOCKING;
        capa->sched.max_num     = capa->max_queues;
+       capa->sched.max_size    = CONFIG_QUEUE_SIZE;

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

Reply via email to