| Message ID | 1446085955-14366-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | Accepted |
| Commit | 5e2f55d76685c88773e3c939898ee0ab994a047b |
| Headers | show |
Merged to api-next. Will merge to master soon. Maxim. On 10/29/2015 05:32, Bill Fischofer wrote: > During odp_schedule_release_ordered() processing in order elements > on a reorder queue should be processed even if they are marked sustain > since no further elements with the current order will be enqueued. > > This fixes Bug https://bugs.linaro.org/show_bug.cgi?id=1824 > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/include/odp_queue_internal.h | 5 +++-- > platform/linux-generic/odp_queue.c | 18 +++++++++++------- > 2 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/platform/linux-generic/include/odp_queue_internal.h b/platform/linux-generic/include/odp_queue_internal.h > index c322e6a..6322948 100644 > --- a/platform/linux-generic/include/odp_queue_internal.h > +++ b/platform/linux-generic/include/odp_queue_internal.h > @@ -291,7 +291,8 @@ static inline int reorder_deq(queue_entry_t *queue, > static inline void reorder_complete(queue_entry_t *origin_qe, > odp_buffer_hdr_t **reorder_buf_return, > odp_buffer_hdr_t **placeholder_buf, > - int placeholder_append) > + int placeholder_append, > + int order_released) > { > odp_buffer_hdr_t *reorder_buf = origin_qe->s.reorder_head; > odp_buffer_hdr_t *next_buf; > @@ -311,7 +312,7 @@ static inline void reorder_complete(queue_entry_t *origin_qe, > > reorder_buf = next_buf; > order_release(origin_qe, 1); > - } else if (reorder_buf->flags.sustain) { > + } else if (!order_released && reorder_buf->flags.sustain) { > reorder_buf = next_buf; > } else { > *reorder_buf_return = origin_qe->s.reorder_head; > diff --git a/platform/linux-generic/odp_queue.c b/platform/linux-generic/odp_queue.c > index a7022cf..a27af0b 100644 > --- a/platform/linux-generic/odp_queue.c > +++ b/platform/linux-generic/odp_queue.c > @@ -478,7 +478,8 @@ int queue_enq(queue_entry_t *queue, odp_buffer_hdr_t *buf_hdr, int sustain) > * other queues, appending placeholder bufs as needed. > */ > UNLOCK(&queue->s.lock); > - reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1); > + reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, > + 1, 0); > UNLOCK(&origin_qe->s.lock); > > if (reorder_buf) > @@ -844,7 +845,7 @@ int queue_pktout_enq(queue_entry_t *queue, odp_buffer_hdr_t *buf_hdr, > order_release(origin_qe, release_count + placeholder_count); > > /* Now handle sends to other queues that are ready to go */ > - reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1); > + reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1, 0); > > /* We're fully done with the origin_qe at last */ > UNLOCK(&origin_qe->s.lock); > @@ -921,13 +922,16 @@ int release_order(queue_entry_t *origin_qe, uint64_t order, > if (order <= origin_qe->s.order_out) { > order_release(origin_qe, 1); > > - /* Check if this release allows us to unblock waiters. > - * At the point of this call, the reorder list may contain > - * zero or more placeholders that need to be freed, followed > - * by zero or one complete reorder buffer chain. > + /* Check if this release allows us to unblock waiters. At the > + * point of this call, the reorder list may contain zero or > + * more placeholders that need to be freed, followed by zero > + * or one complete reorder buffer chain. Note that since we > + * are releasing order, we know no further enqs for this order > + * can occur, so ignore the sustain bit to clear out our > + * element(s) on the reorder queue > */ > reorder_complete(origin_qe, &reorder_buf, > - &placeholder_buf_hdr, 0); > + &placeholder_buf_hdr, 0, 1); > > /* Now safe to unlock */ > UNLOCK(&origin_qe->s.lock);
Bill,
I merged but looks like it was not tested enough. Usually it passes,
but in some cases it fails:
1. scheduler.c:577 - bctx->sequence == seq
2. scheduler.c:577 - bctx->sequence == seq
I can reproduce issue with that script:
while true; do ./test/validation/scheduler/scheduler_main 2>&1 |grep
FAIL; done
Test: scheduler_test_mq_mt_prio_o ...FAILED
Test: scheduler_test_mq_mt_prio_o ...FAILED
Test: scheduler_test_multi_mq_mt_prio_o ...FAILED
Test: scheduler_test_mq_mt_prio_o ...FAILED
Test: scheduler_test_mq_mt_prio_o ...FAILED
Test: scheduler_test_multi_mq_mt_prio_o ...FAILED
Test: scheduler_test_multi_mq_mt_prio_o ...FAILED
Test: scheduler_test_multi_mq_mt_prio_o ...FAILED
Test: scheduler_test_mq_mt_prio_o ...FAILED
Maxim.
On 10/30/2015 10:14, Maxim Uvarov wrote:
> Merged to api-next.
> Will merge to master soon.
>
> Maxim.
>
> On 10/29/2015 05:32, Bill Fischofer wrote:
>> During odp_schedule_release_ordered() processing in order elements
>> on a reorder queue should be processed even if they are marked sustain
>> since no further elements with the current order will be enqueued.
>>
>> This fixes Bug https://bugs.linaro.org/show_bug.cgi?id=1824
>>
>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
>> ---
>> platform/linux-generic/include/odp_queue_internal.h | 5 +++--
>> platform/linux-generic/odp_queue.c | 18
>> +++++++++++-------
>> 2 files changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/platform/linux-generic/include/odp_queue_internal.h
>> b/platform/linux-generic/include/odp_queue_internal.h
>> index c322e6a..6322948 100644
>> --- a/platform/linux-generic/include/odp_queue_internal.h
>> +++ b/platform/linux-generic/include/odp_queue_internal.h
>> @@ -291,7 +291,8 @@ static inline int reorder_deq(queue_entry_t *queue,
>> static inline void reorder_complete(queue_entry_t *origin_qe,
>> odp_buffer_hdr_t **reorder_buf_return,
>> odp_buffer_hdr_t **placeholder_buf,
>> - int placeholder_append)
>> + int placeholder_append,
>> + int order_released)
>> {
>> odp_buffer_hdr_t *reorder_buf = origin_qe->s.reorder_head;
>> odp_buffer_hdr_t *next_buf;
>> @@ -311,7 +312,7 @@ static inline void reorder_complete(queue_entry_t
>> *origin_qe,
>> reorder_buf = next_buf;
>> order_release(origin_qe, 1);
>> - } else if (reorder_buf->flags.sustain) {
>> + } else if (!order_released && reorder_buf->flags.sustain) {
>> reorder_buf = next_buf;
>> } else {
>> *reorder_buf_return = origin_qe->s.reorder_head;
>> diff --git a/platform/linux-generic/odp_queue.c
>> b/platform/linux-generic/odp_queue.c
>> index a7022cf..a27af0b 100644
>> --- a/platform/linux-generic/odp_queue.c
>> +++ b/platform/linux-generic/odp_queue.c
>> @@ -478,7 +478,8 @@ int queue_enq(queue_entry_t *queue,
>> odp_buffer_hdr_t *buf_hdr, int sustain)
>> * other queues, appending placeholder bufs as needed.
>> */
>> UNLOCK(&queue->s.lock);
>> - reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1);
>> + reorder_complete(origin_qe, &reorder_buf, &placeholder_buf,
>> + 1, 0);
>> UNLOCK(&origin_qe->s.lock);
>> if (reorder_buf)
>> @@ -844,7 +845,7 @@ int queue_pktout_enq(queue_entry_t *queue,
>> odp_buffer_hdr_t *buf_hdr,
>> order_release(origin_qe, release_count + placeholder_count);
>> /* Now handle sends to other queues that are ready to go */
>> - reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1);
>> + reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1, 0);
>> /* We're fully done with the origin_qe at last */
>> UNLOCK(&origin_qe->s.lock);
>> @@ -921,13 +922,16 @@ int release_order(queue_entry_t *origin_qe,
>> uint64_t order,
>> if (order <= origin_qe->s.order_out) {
>> order_release(origin_qe, 1);
>> - /* Check if this release allows us to unblock waiters.
>> - * At the point of this call, the reorder list may contain
>> - * zero or more placeholders that need to be freed, followed
>> - * by zero or one complete reorder buffer chain.
>> + /* Check if this release allows us to unblock waiters. At the
>> + * point of this call, the reorder list may contain zero or
>> + * more placeholders that need to be freed, followed by zero
>> + * or one complete reorder buffer chain. Note that since we
>> + * are releasing order, we know no further enqs for this order
>> + * can occur, so ignore the sustain bit to clear out our
>> + * element(s) on the reorder queue
>> */
>> reorder_complete(origin_qe, &reorder_buf,
>> - &placeholder_buf_hdr, 0);
>> + &placeholder_buf_hdr, 0, 1);
>> /* Now safe to unlock */
>> UNLOCK(&origin_qe->s.lock);
>
Thanks. I believe it should still be merged as it's much more reliable than before, and the CUnit test is more thorough, which is how we found the base problem. If there are still issues they should be addressed as another bug item. On Fri, Oct 30, 2015 at 2:53 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Bill, > > I merged but looks like it was not tested enough. Usually it passes, but > in some cases it fails: > 1. scheduler.c:577 - bctx->sequence == seq > 2. scheduler.c:577 - bctx->sequence == seq > > I can reproduce issue with that script: > while true; do ./test/validation/scheduler/scheduler_main 2>&1 |grep FAIL; > done > > Test: scheduler_test_mq_mt_prio_o ...FAILED > Test: scheduler_test_mq_mt_prio_o ...FAILED > Test: scheduler_test_multi_mq_mt_prio_o ...FAILED > Test: scheduler_test_mq_mt_prio_o ...FAILED > Test: scheduler_test_mq_mt_prio_o ...FAILED > Test: scheduler_test_multi_mq_mt_prio_o ...FAILED > Test: scheduler_test_multi_mq_mt_prio_o ...FAILED > Test: scheduler_test_multi_mq_mt_prio_o ...FAILED > Test: scheduler_test_mq_mt_prio_o ...FAILED > > Maxim. > > > > On 10/30/2015 10:14, Maxim Uvarov wrote: > >> Merged to api-next. >> Will merge to master soon. >> >> Maxim. >> >> On 10/29/2015 05:32, Bill Fischofer wrote: >> >>> During odp_schedule_release_ordered() processing in order elements >>> on a reorder queue should be processed even if they are marked sustain >>> since no further elements with the current order will be enqueued. >>> >>> This fixes Bug https://bugs.linaro.org/show_bug.cgi?id=1824 >>> >>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>> --- >>> platform/linux-generic/include/odp_queue_internal.h | 5 +++-- >>> platform/linux-generic/odp_queue.c | 18 >>> +++++++++++------- >>> 2 files changed, 14 insertions(+), 9 deletions(-) >>> >>> diff --git a/platform/linux-generic/include/odp_queue_internal.h >>> b/platform/linux-generic/include/odp_queue_internal.h >>> index c322e6a..6322948 100644 >>> --- a/platform/linux-generic/include/odp_queue_internal.h >>> +++ b/platform/linux-generic/include/odp_queue_internal.h >>> @@ -291,7 +291,8 @@ static inline int reorder_deq(queue_entry_t *queue, >>> static inline void reorder_complete(queue_entry_t *origin_qe, >>> odp_buffer_hdr_t **reorder_buf_return, >>> odp_buffer_hdr_t **placeholder_buf, >>> - int placeholder_append) >>> + int placeholder_append, >>> + int order_released) >>> { >>> odp_buffer_hdr_t *reorder_buf = origin_qe->s.reorder_head; >>> odp_buffer_hdr_t *next_buf; >>> @@ -311,7 +312,7 @@ static inline void reorder_complete(queue_entry_t >>> *origin_qe, >>> reorder_buf = next_buf; >>> order_release(origin_qe, 1); >>> - } else if (reorder_buf->flags.sustain) { >>> + } else if (!order_released && reorder_buf->flags.sustain) { >>> reorder_buf = next_buf; >>> } else { >>> *reorder_buf_return = origin_qe->s.reorder_head; >>> diff --git a/platform/linux-generic/odp_queue.c >>> b/platform/linux-generic/odp_queue.c >>> index a7022cf..a27af0b 100644 >>> --- a/platform/linux-generic/odp_queue.c >>> +++ b/platform/linux-generic/odp_queue.c >>> @@ -478,7 +478,8 @@ int queue_enq(queue_entry_t *queue, odp_buffer_hdr_t >>> *buf_hdr, int sustain) >>> * other queues, appending placeholder bufs as needed. >>> */ >>> UNLOCK(&queue->s.lock); >>> - reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1); >>> + reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, >>> + 1, 0); >>> UNLOCK(&origin_qe->s.lock); >>> if (reorder_buf) >>> @@ -844,7 +845,7 @@ int queue_pktout_enq(queue_entry_t *queue, >>> odp_buffer_hdr_t *buf_hdr, >>> order_release(origin_qe, release_count + placeholder_count); >>> /* Now handle sends to other queues that are ready to go */ >>> - reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1); >>> + reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1, 0); >>> /* We're fully done with the origin_qe at last */ >>> UNLOCK(&origin_qe->s.lock); >>> @@ -921,13 +922,16 @@ int release_order(queue_entry_t *origin_qe, >>> uint64_t order, >>> if (order <= origin_qe->s.order_out) { >>> order_release(origin_qe, 1); >>> - /* Check if this release allows us to unblock waiters. >>> - * At the point of this call, the reorder list may contain >>> - * zero or more placeholders that need to be freed, followed >>> - * by zero or one complete reorder buffer chain. >>> + /* Check if this release allows us to unblock waiters. At the >>> + * point of this call, the reorder list may contain zero or >>> + * more placeholders that need to be freed, followed by zero >>> + * or one complete reorder buffer chain. Note that since we >>> + * are releasing order, we know no further enqs for this order >>> + * can occur, so ignore the sustain bit to clear out our >>> + * element(s) on the reorder queue >>> */ >>> reorder_complete(origin_qe, &reorder_buf, >>> - &placeholder_buf_hdr, 0); >>> + &placeholder_buf_hdr, 0, 1); >>> /* Now safe to unlock */ >>> UNLOCK(&origin_qe->s.lock); >>> >> >> >
On 10/30/2015 14:14, Bill Fischofer wrote: > Thanks. I believe it should still be merged as it's much more > reliable than before, and the CUnit test is more thorough, which is > how we found the base problem. If there are still issues they should > be addressed as another bug item. > For now CI pass for master. I guess if I will include that patches to master test will start failing. Maxim. > On Fri, Oct 30, 2015 at 2:53 AM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > Bill, > > I merged but looks like it was not tested enough. Usually it > passes, but in some cases it fails: > 1. scheduler.c:577 - bctx->sequence == seq > 2. scheduler.c:577 - bctx->sequence == seq > > I can reproduce issue with that script: > while true; do ./test/validation/scheduler/scheduler_main 2>&1 > |grep FAIL; done > > Test: scheduler_test_mq_mt_prio_o ...FAILED > Test: scheduler_test_mq_mt_prio_o ...FAILED > Test: scheduler_test_multi_mq_mt_prio_o ...FAILED > Test: scheduler_test_mq_mt_prio_o ...FAILED > Test: scheduler_test_mq_mt_prio_o ...FAILED > Test: scheduler_test_multi_mq_mt_prio_o ...FAILED > Test: scheduler_test_multi_mq_mt_prio_o ...FAILED > Test: scheduler_test_multi_mq_mt_prio_o ...FAILED > Test: scheduler_test_mq_mt_prio_o ...FAILED > > Maxim. > > > > On 10/30/2015 10:14, Maxim Uvarov wrote: > > Merged to api-next. > Will merge to master soon. > > Maxim. > > On 10/29/2015 05:32, Bill Fischofer wrote: > > During odp_schedule_release_ordered() processing in order > elements > on a reorder queue should be processed even if they are > marked sustain > since no further elements with the current order will be > enqueued. > > This fixes Bug https://bugs.linaro.org/show_bug.cgi?id=1824 > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>> > --- > platform/linux-generic/include/odp_queue_internal.h | 5 +++-- > platform/linux-generic/odp_queue.c | 18 +++++++++++------- > 2 files changed, 14 insertions(+), 9 deletions(-) > > diff --git > a/platform/linux-generic/include/odp_queue_internal.h > b/platform/linux-generic/include/odp_queue_internal.h > index c322e6a..6322948 100644 > --- a/platform/linux-generic/include/odp_queue_internal.h > +++ b/platform/linux-generic/include/odp_queue_internal.h > @@ -291,7 +291,8 @@ static inline int > reorder_deq(queue_entry_t *queue, > static inline void reorder_complete(queue_entry_t > *origin_qe, > odp_buffer_hdr_t **reorder_buf_return, > odp_buffer_hdr_t **placeholder_buf, > - int placeholder_append) > + int placeholder_append, > + int order_released) > { > odp_buffer_hdr_t *reorder_buf = > origin_qe->s.reorder_head; > odp_buffer_hdr_t *next_buf; > @@ -311,7 +312,7 @@ static inline void > reorder_complete(queue_entry_t *origin_qe, > reorder_buf = next_buf; > order_release(origin_qe, 1); > - } else if (reorder_buf->flags.sustain) { > + } else if (!order_released && > reorder_buf->flags.sustain) { > reorder_buf = next_buf; > } else { > *reorder_buf_return = origin_qe->s.reorder_head; > diff --git a/platform/linux-generic/odp_queue.c > b/platform/linux-generic/odp_queue.c > index a7022cf..a27af0b 100644 > --- a/platform/linux-generic/odp_queue.c > +++ b/platform/linux-generic/odp_queue.c > @@ -478,7 +478,8 @@ int queue_enq(queue_entry_t *queue, > odp_buffer_hdr_t *buf_hdr, int sustain) > * other queues, appending placeholder bufs as > needed. > */ > UNLOCK(&queue->s.lock); > - reorder_complete(origin_qe, &reorder_buf, > &placeholder_buf, 1); > + reorder_complete(origin_qe, &reorder_buf, > &placeholder_buf, > + 1, 0); > UNLOCK(&origin_qe->s.lock); > if (reorder_buf) > @@ -844,7 +845,7 @@ int queue_pktout_enq(queue_entry_t > *queue, odp_buffer_hdr_t *buf_hdr, > order_release(origin_qe, release_count + > placeholder_count); > /* Now handle sends to other queues that are ready > to go */ > - reorder_complete(origin_qe, &reorder_buf, > &placeholder_buf, 1); > + reorder_complete(origin_qe, &reorder_buf, > &placeholder_buf, 1, 0); > /* We're fully done with the origin_qe at last */ > UNLOCK(&origin_qe->s.lock); > @@ -921,13 +922,16 @@ int release_order(queue_entry_t > *origin_qe, uint64_t order, > if (order <= origin_qe->s.order_out) { > order_release(origin_qe, 1); > - /* Check if this release allows us to unblock > waiters. > - * At the point of this call, the reorder list > may contain > - * zero or more placeholders that need to be > freed, followed > - * by zero or one complete reorder buffer chain. > + /* Check if this release allows us to unblock > waiters. At the > + * point of this call, the reorder list may > contain zero or > + * more placeholders that need to be freed, > followed by zero > + * or one complete reorder buffer chain. Note > that since we > + * are releasing order, we know no further enqs > for this order > + * can occur, so ignore the sustain bit to clear > out our > + * element(s) on the reorder queue > */ > reorder_complete(origin_qe, &reorder_buf, > - &placeholder_buf_hdr, 0); > + &placeholder_buf_hdr, 0, 1); > /* Now safe to unlock */ > UNLOCK(&origin_qe->s.lock); > > > >
I've confirmed I can reproduce the failure, but it obviously doesn't happen very often. I'll open a bug for it and investigate further as normal priority. Thanks. On Fri, Oct 30, 2015 at 6:14 AM, Bill Fischofer <bill.fischofer@linaro.org> wrote: > Thanks. I believe it should still be merged as it's much more reliable > than before, and the CUnit test is more thorough, which is how we found the > base problem. If there are still issues they should be addressed as > another bug item. > > On Fri, Oct 30, 2015 at 2:53 AM, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > >> Bill, >> >> I merged but looks like it was not tested enough. Usually it passes, but >> in some cases it fails: >> 1. scheduler.c:577 - bctx->sequence == seq >> 2. scheduler.c:577 - bctx->sequence == seq >> >> I can reproduce issue with that script: >> while true; do ./test/validation/scheduler/scheduler_main 2>&1 |grep >> FAIL; done >> >> Test: scheduler_test_mq_mt_prio_o ...FAILED >> Test: scheduler_test_mq_mt_prio_o ...FAILED >> Test: scheduler_test_multi_mq_mt_prio_o ...FAILED >> Test: scheduler_test_mq_mt_prio_o ...FAILED >> Test: scheduler_test_mq_mt_prio_o ...FAILED >> Test: scheduler_test_multi_mq_mt_prio_o ...FAILED >> Test: scheduler_test_multi_mq_mt_prio_o ...FAILED >> Test: scheduler_test_multi_mq_mt_prio_o ...FAILED >> Test: scheduler_test_mq_mt_prio_o ...FAILED >> >> Maxim. >> >> >> >> On 10/30/2015 10:14, Maxim Uvarov wrote: >> >>> Merged to api-next. >>> Will merge to master soon. >>> >>> Maxim. >>> >>> On 10/29/2015 05:32, Bill Fischofer wrote: >>> >>>> During odp_schedule_release_ordered() processing in order elements >>>> on a reorder queue should be processed even if they are marked sustain >>>> since no further elements with the current order will be enqueued. >>>> >>>> This fixes Bug https://bugs.linaro.org/show_bug.cgi?id=1824 >>>> >>>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> >>>> --- >>>> platform/linux-generic/include/odp_queue_internal.h | 5 +++-- >>>> platform/linux-generic/odp_queue.c | 18 >>>> +++++++++++------- >>>> 2 files changed, 14 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/platform/linux-generic/include/odp_queue_internal.h >>>> b/platform/linux-generic/include/odp_queue_internal.h >>>> index c322e6a..6322948 100644 >>>> --- a/platform/linux-generic/include/odp_queue_internal.h >>>> +++ b/platform/linux-generic/include/odp_queue_internal.h >>>> @@ -291,7 +291,8 @@ static inline int reorder_deq(queue_entry_t *queue, >>>> static inline void reorder_complete(queue_entry_t *origin_qe, >>>> odp_buffer_hdr_t **reorder_buf_return, >>>> odp_buffer_hdr_t **placeholder_buf, >>>> - int placeholder_append) >>>> + int placeholder_append, >>>> + int order_released) >>>> { >>>> odp_buffer_hdr_t *reorder_buf = origin_qe->s.reorder_head; >>>> odp_buffer_hdr_t *next_buf; >>>> @@ -311,7 +312,7 @@ static inline void reorder_complete(queue_entry_t >>>> *origin_qe, >>>> reorder_buf = next_buf; >>>> order_release(origin_qe, 1); >>>> - } else if (reorder_buf->flags.sustain) { >>>> + } else if (!order_released && reorder_buf->flags.sustain) { >>>> reorder_buf = next_buf; >>>> } else { >>>> *reorder_buf_return = origin_qe->s.reorder_head; >>>> diff --git a/platform/linux-generic/odp_queue.c >>>> b/platform/linux-generic/odp_queue.c >>>> index a7022cf..a27af0b 100644 >>>> --- a/platform/linux-generic/odp_queue.c >>>> +++ b/platform/linux-generic/odp_queue.c >>>> @@ -478,7 +478,8 @@ int queue_enq(queue_entry_t *queue, >>>> odp_buffer_hdr_t *buf_hdr, int sustain) >>>> * other queues, appending placeholder bufs as needed. >>>> */ >>>> UNLOCK(&queue->s.lock); >>>> - reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1); >>>> + reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, >>>> + 1, 0); >>>> UNLOCK(&origin_qe->s.lock); >>>> if (reorder_buf) >>>> @@ -844,7 +845,7 @@ int queue_pktout_enq(queue_entry_t *queue, >>>> odp_buffer_hdr_t *buf_hdr, >>>> order_release(origin_qe, release_count + placeholder_count); >>>> /* Now handle sends to other queues that are ready to go */ >>>> - reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1); >>>> + reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1, 0); >>>> /* We're fully done with the origin_qe at last */ >>>> UNLOCK(&origin_qe->s.lock); >>>> @@ -921,13 +922,16 @@ int release_order(queue_entry_t *origin_qe, >>>> uint64_t order, >>>> if (order <= origin_qe->s.order_out) { >>>> order_release(origin_qe, 1); >>>> - /* Check if this release allows us to unblock waiters. >>>> - * At the point of this call, the reorder list may contain >>>> - * zero or more placeholders that need to be freed, followed >>>> - * by zero or one complete reorder buffer chain. >>>> + /* Check if this release allows us to unblock waiters. At the >>>> + * point of this call, the reorder list may contain zero or >>>> + * more placeholders that need to be freed, followed by zero >>>> + * or one complete reorder buffer chain. Note that since we >>>> + * are releasing order, we know no further enqs for this order >>>> + * can occur, so ignore the sustain bit to clear out our >>>> + * element(s) on the reorder queue >>>> */ >>>> reorder_complete(origin_qe, &reorder_buf, >>>> - &placeholder_buf_hdr, 0); >>>> + &placeholder_buf_hdr, 0, 1); >>>> /* Now safe to unlock */ >>>> UNLOCK(&origin_qe->s.lock); >>>> >>> >>> >> >
diff --git a/platform/linux-generic/include/odp_queue_internal.h b/platform/linux-generic/include/odp_queue_internal.h index c322e6a..6322948 100644 --- a/platform/linux-generic/include/odp_queue_internal.h +++ b/platform/linux-generic/include/odp_queue_internal.h @@ -291,7 +291,8 @@ static inline int reorder_deq(queue_entry_t *queue, static inline void reorder_complete(queue_entry_t *origin_qe, odp_buffer_hdr_t **reorder_buf_return, odp_buffer_hdr_t **placeholder_buf, - int placeholder_append) + int placeholder_append, + int order_released) { odp_buffer_hdr_t *reorder_buf = origin_qe->s.reorder_head; odp_buffer_hdr_t *next_buf; @@ -311,7 +312,7 @@ static inline void reorder_complete(queue_entry_t *origin_qe, reorder_buf = next_buf; order_release(origin_qe, 1); - } else if (reorder_buf->flags.sustain) { + } else if (!order_released && reorder_buf->flags.sustain) { reorder_buf = next_buf; } else { *reorder_buf_return = origin_qe->s.reorder_head; diff --git a/platform/linux-generic/odp_queue.c b/platform/linux-generic/odp_queue.c index a7022cf..a27af0b 100644 --- a/platform/linux-generic/odp_queue.c +++ b/platform/linux-generic/odp_queue.c @@ -478,7 +478,8 @@ int queue_enq(queue_entry_t *queue, odp_buffer_hdr_t *buf_hdr, int sustain) * other queues, appending placeholder bufs as needed. */ UNLOCK(&queue->s.lock); - reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1); + reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, + 1, 0); UNLOCK(&origin_qe->s.lock); if (reorder_buf) @@ -844,7 +845,7 @@ int queue_pktout_enq(queue_entry_t *queue, odp_buffer_hdr_t *buf_hdr, order_release(origin_qe, release_count + placeholder_count); /* Now handle sends to other queues that are ready to go */ - reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1); + reorder_complete(origin_qe, &reorder_buf, &placeholder_buf, 1, 0); /* We're fully done with the origin_qe at last */ UNLOCK(&origin_qe->s.lock); @@ -921,13 +922,16 @@ int release_order(queue_entry_t *origin_qe, uint64_t order, if (order <= origin_qe->s.order_out) { order_release(origin_qe, 1); - /* Check if this release allows us to unblock waiters. - * At the point of this call, the reorder list may contain - * zero or more placeholders that need to be freed, followed - * by zero or one complete reorder buffer chain. + /* Check if this release allows us to unblock waiters. At the + * point of this call, the reorder list may contain zero or + * more placeholders that need to be freed, followed by zero + * or one complete reorder buffer chain. Note that since we + * are releasing order, we know no further enqs for this order + * can occur, so ignore the sustain bit to clear out our + * element(s) on the reorder queue */ reorder_complete(origin_qe, &reorder_buf, - &placeholder_buf_hdr, 0); + &placeholder_buf_hdr, 0, 1); /* Now safe to unlock */ UNLOCK(&origin_qe->s.lock);
During odp_schedule_release_ordered() processing in order elements on a reorder queue should be processed even if they are marked sustain since no further elements with the current order will be enqueued. This fixes Bug https://bugs.linaro.org/show_bug.cgi?id=1824 Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- platform/linux-generic/include/odp_queue_internal.h | 5 +++-- platform/linux-generic/odp_queue.c | 18 +++++++++++------- 2 files changed, 14 insertions(+), 9 deletions(-)