On 11/03/2015 10:21 AM, Nicolas Morey-Chaisemartin wrote: > > On 11/02/2015 06:11 PM, Bill Fischofer wrote: >> Enqueueing to ordered queues requires two locks (source and target >> queues). Since any queue can be either source or target, queues do not >> have a natural locking hierarchy. Create one by using the address of >> the qentry as the lock order. >> >> This addresses the aspect of Bug >> https://bugs.linaro.org/show_bug.cgi?id=1879 >> relating to deadlock in unicore systems. >> >> Suggested-by: Nicolas Morey-Chaisemartin <nmo...@kalray.eu> >> Signed-off-by: Bill Fischofer <bill.fischo...@linaro.org> >> --- >> platform/linux-generic/odp_queue.c | 39 >> +++++++++++++++++++++++++++++--------- >> 1 file changed, 30 insertions(+), 9 deletions(-) >> >> diff --git a/platform/linux-generic/odp_queue.c >> b/platform/linux-generic/odp_queue.c >> index a27af0b..1c15e17 100644 >> --- a/platform/linux-generic/odp_queue.c >> +++ b/platform/linux-generic/odp_queue.c >> @@ -48,6 +48,28 @@ typedef struct queue_table_t { >> static queue_table_t *queue_tbl; >> >> >> +static inline void get_qe_locks(queue_entry_t *qe1, queue_entry_t *qe2) >> +{ >> + /* Special case: enq to self */ >> + if (qe1 == qe2) { >> + LOCK(&qe1->s.lock); >> + return; >> + } >> + >> + /* Since any queue can be either a source or target, queues do not >> have >> + * a natural locking hierarchy. Create one by using the qentry address >> + * as the ordering mechanism. >> + */ >> + >> + if (qe1 < qe2) { >> + LOCK(&qe1->s.lock); >> + LOCK(&qe2->s.lock); >> + } else { >> + LOCK(&qe2->s.lock); >> + LOCK(&qe1->s.lock); >> + } >> +} >> + >> queue_entry_t *get_qentry(uint32_t queue_id) >> { >> return &queue_tbl->queue[queue_id]; >> @@ -370,14 +392,11 @@ int queue_enq(queue_entry_t *queue, odp_buffer_hdr_t >> *buf_hdr, int sustain) >> >> /* Need two locks for enq operations from ordered queues */ >> if (origin_qe) { >> - LOCK(&origin_qe->s.lock); >> - while (!LOCK_TRY(&queue->s.lock)) { >> - UNLOCK(&origin_qe->s.lock); >> - LOCK(&origin_qe->s.lock); >> - } >> + get_qe_locks(origin_qe, queue); >> if (odp_unlikely(origin_qe->s.status < QUEUE_STATUS_READY)) { >> UNLOCK(&queue->s.lock); >> - UNLOCK(&origin_qe->s.lock); >> + if (origin_qe != queue) > This test is unneeded as we are already in a if(origin_qe) branch Agreed. This patch is actually fixing 2 issues (if I read it right): * Actual deadlock issues if qe == origin_qe * Linux scheduling issues
It would be nice to split the patch or update the log accordingly. Other than that, it looks good. Nicolas _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
