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

platform/linux-generic/odp_queue.c
line 420
@@ -584,8 +556,9 @@ static int queue_init(queue_entry_t *queue, const char 
*name,
        queue->s.pktin = PKTIN_INVALID;
        queue->s.pktout = PKTOUT_INVALID;
 
-       queue->s.head = NULL;
-       queue->s.tail = NULL;
+       ring_st_init(&queue->s.ring_st,
+                    queue_tbl->ring_data[queue->s.index].data,
+                    CONFIG_QUEUE_SIZE);


Comment:
Must use `CONFIG_QUEUE_SIZE - 1` here, as noted earlier, if we're not going to 
use the user-supplied queue size.

> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Given its name, this looks like an extraneous statement that should be 
> deleted. Renaming this to something like `prefetch_dequeued_bufs()` would 
> make the intent clearer here.


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

Reply via email to