Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_config_internal.h
line 5
@@ -144,7 +144,7 @@ extern "C" {
* This controls the burst size on various enqueue, dequeue, etc calls. Large
* burst size improves throughput, but may degrade QoS (increase latency).
*/
-#define CONFIG_BURST_SIZE 16
+#define CONFIG_BURST_SIZE 32
Comment:
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_r169825596
updated_at 2018-02-22 09:32:40
