On 5 April 2017 at 14:50, Maxim Uvarov <maxim.uva...@linaro.org> wrote:
> On 04/05/17 06:57, Honnappa Nagarahalli wrote:
>> This can go into master/api-next as an independent patch. Agree?
>
> agree. If we accept implementation where events can be 'delayed'
Probably all platforms with HW queues.
> than it
> looks like we missed some api to sync queues.
When would those API's be used?
>
> But I do not see why we need this patch. On the same cpu test queue 1
> event and after that dequeue 1 event:
>
> for (i = 0; i < QUEUE_ROUNDS; i++) {
> ev = odp_buffer_to_event(buf);
>
> if (odp_queue_enq(queue, ev)) {
> LOG_ERR(" [%i] Queue enqueue failed.\n", thr);
> odp_buffer_free(buf);
> return -1;
> }
>
> ev = odp_queue_deq(queue);
>
> buf = odp_buffer_from_event(ev);
>
> if (!odp_buffer_is_valid(buf)) {
> LOG_ERR(" [%i] Queue empty.\n", thr);
> return -1;
> }
> }
>
> Where this exactly event can be delayed?
In the memory system.
>
> If other threads do the same - then all do enqueue 1 event first and
> then dequeue one event. I can understand problem with queueing on one
> cpu and dequeuing on other cpu. But on the same cpu is has to always
> work. Isn't it?
No.
>
> Maxim.
>
>>
>> On 4 April 2017 at 21:22, Brian Brooks <brian.bro...@arm.com> wrote:
>>> On 04/04 17:26:12, Bill Fischofer wrote:
>>>> On Tue, Apr 4, 2017 at 3:37 PM, Brian Brooks <brian.bro...@arm.com> wrote:
>>>>> On 04/04 21:59:15, Maxim Uvarov wrote:
>>>>>> On 04/04/17 21:47, Brian Brooks wrote:
>>>>>>> Signed-off-by: Ola Liljedahl <ola.liljed...@arm.com>
>>>>>>> Reviewed-by: Brian Brooks <brian.bro...@arm.com>
>>>>>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
>>>>>>> Reviewed-by: Kevin Wang <kevin.w...@arm.com>
>>>>>>> ---
>>>>>>> test/common_plat/performance/odp_scheduling.c | 12 ++++++++++--
>>>>>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/test/common_plat/performance/odp_scheduling.c
>>>>>>> b/test/common_plat/performance/odp_scheduling.c
>>>>>>> index c74a0713..38e76257 100644
>>>>>>> --- a/test/common_plat/performance/odp_scheduling.c
>>>>>>> +++ b/test/common_plat/performance/odp_scheduling.c
>>>>>>> @@ -273,7 +273,7 @@ static int test_plain_queue(int thr, test_globals_t
>>>>>>> *globals)
>>>>>>> test_message_t *t_msg;
>>>>>>> odp_queue_t queue;
>>>>>>> uint64_t c1, c2, cycles;
>>>>>>> - int i;
>>>>>>> + int i, j;
>>>>>>>
>>>>>>> /* Alloc test message */
>>>>>>> buf = odp_buffer_alloc(globals->pool);
>>>>>>> @@ -307,7 +307,15 @@ static int test_plain_queue(int thr,
>>>>>>> test_globals_t *globals)
>>>>>>> return -1;
>>>>>>> }
>>>>>>>
>>>>>>> - ev = odp_queue_deq(queue);
>>>>>>> + /* When enqueue and dequeue are decoupled (e.g. not using a
>>>>>>> + * common lock), an enqueued event may not be immediately
>>>>>>> + * visible to dequeue. So we just try again for a while. */
>>>>>>> + for (j = 0; j < 100; j++) {
>>>>>>
>>>>>> where 100 number comes from?
>>>>>
>>>>> It is the retry count. Perhaps it could be a bit lower, or a bit higher,
>>>>> but
>>>>> it works well.
>>>>
>>>> Actually, it's incorrect. What happens if all 100 retries fail? You'll
>>>> call odp_buffer_from_event() for ODP_EVENT_INVALID, which is
>>>> undefined.
>>>
>>> Incorrect? :) The point is that an event may not be immediately available
>>> to dequeue after it has been enqueued. This is due to the way that a
>>> concurrent
>>> ring buffer behaves in a multi-threaded environment. The approach here is
>>> just to retry the dequeue a couple times (100 times actually) before moving
>>> on to the rest of code. Perhaps 100 times is too many times, but some amount
>>> of retry is needed.
>>>
>>> If this is not desirable, then I think it would be more accurate to consider
>>> odp_queue_enq() / odp_queue_deq() as async APIs -or- MT-unsafe (must be
>>> called
>>> from one thread at a time in order to ensure the behavior that an event is
>>> immediately available for dequeue once it has been enqueued).
>>>
>>>>>
>>>>>> Maxim.
>>>>>>
>>>>>>> + ev = odp_queue_deq(queue);
>>>>>>> + if (ev != ODP_EVENT_INVALID)
>>>>>>> + break;
>>>>>>> + odp_cpu_pause();
>>>>>>> + }
>>>>>>>
>>>>>>> buf = odp_buffer_from_event(ev);
>>>>>>>
>>>>>>>
>>>>>>
>
