Petri Savolainen(psavol) replied on github web page:

platform/linux-generic/odp_pool.c
line 28
@@ -296,7 +282,9 @@ static void init_buffers(pool_t *pool)
                memset(buf_hdr, 0, (uintptr_t)data - (uintptr_t)buf_hdr);
 
                /* Initialize buffer metadata */
-               buf_hdr->index = i;
+               buf_hdr->index.u32    = 0;
+               buf_hdr->index.pool   = pool->pool_idx;
+               buf_hdr->index.buffer = i;


Comment:
It's there to ensure that all bits are zero also when someone would modify the 
bitfield from two to three fields later on. Similarly to memset() zero is used 
for struct inits.

> Petri Savolainen(psavol) wrote:
> There's no need for abs(). Since it's all uint32_t variables, wrap a round is 
> handled already.
> An example in 8bits:
> 0xff - 0xfd = 0x02
> 0x00 - 0xfe = 0x02
> 0x01 - 0xff = 0x02
> 0x02 - 0x00 = 0x02
> 
> This passes both gcc and clang, and is used already in the other ring 
> implementation see ring_deq_multi().


>> Petri Savolainen(psavol) wrote:
>> I prefer style with blank line in the end of a typedef, since it's easier to 
>> spot the type name (as it's not mixed into struct field names). Checkpatch 
>> passes so this style should be OK.


>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Does this mean that sizes larger than 32 have no added performance benefit?


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

Reply via email to