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

platform/linux-generic/include/odp_buffer_internal.h
line 17
@@ -41,11 +41,19 @@ typedef struct seg_entry_t {
        uint32_t  len;
 } seg_entry_t;
 
+typedef union buffer_index_t {
+       uint32_t u32;
+
+       struct {
+               uint32_t pool   :8;
+               uint32_t buffer :24;


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

Reply via email to