From c4b22207b569fe3df52c70cc4ef438b69f980c78 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 15 Nov 2017 13:38:57 +0100 Subject: [PATCH] util/async: use atomic_mb_set in qemu_bh_cancel RH-Author: Stefan Hajnoczi Message-id: <20171115133857.13217-2-stefanha@redhat.com> Patchwork-id: 77681 O-Subject: [RHV-7.4.z qemu-kvm-rhev PATCH 1/1] util/async: use atomic_mb_set in qemu_bh_cancel Bugzilla: 1513362 RH-Acked-by: Paolo Bonzini RH-Acked-by: Laurent Vivier RH-Acked-by: John Snow From: Sergio Lopez Commit b7a745d added a qemu_bh_cancel call to the completion function as an optimization to prevent it from unnecessarily rescheduling itself. This completion function is scheduled from worker_thread, after setting the state of a ThreadPoolElement to THREAD_DONE. This was considered to be safe, as the completion function restarts the loop just after the call to qemu_bh_cancel. But, as this loop lacks a HW memory barrier, the read of req->state may actually happen _before_ the call, seeing it still as THREAD_QUEUED, and ending the completion function without having processed a pending TPE linked at pool->head: worker thread | I/O thread ------------------------------------------------------------------------ | speculatively read req->state req->state = THREAD_DONE; | qemu_bh_schedule(p->completion_bh) | bh->scheduled = 1; | | qemu_bh_cancel(p->completion_bh) | bh->scheduled = 0; | if (req->state == THREAD_DONE) | // sees THREAD_QUEUED The source of the misunderstanding was that qemu_bh_cancel is now being used by the _consumer_ rather than the producer, and therefore now needs to have acquire semantics just like e.g. aio_bh_poll. In some situations, if there are no other independent requests in the same aio context that could eventually trigger the scheduling of the completion function, the omitted TPE and all operations pending on it will get stuck forever. [Added Sergio's updated wording about the HW memory barrier. --Stefan] Signed-off-by: Sergio Lopez Message-id: 20171108063447.2842-1-slp@redhat.com Signed-off-by: Stefan Hajnoczi (cherry picked from commit ef6dada8b44e1e7c4bec5c1115903af9af415b50) Signed-off-by: Stefan Hajnoczi Signed-off-by: Miroslav Rezanina --- util/async.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/async.c b/util/async.c index 355af73..0e1bd87 100644 --- a/util/async.c +++ b/util/async.c @@ -174,7 +174,7 @@ void qemu_bh_schedule(QEMUBH *bh) */ void qemu_bh_cancel(QEMUBH *bh) { - bh->scheduled = 0; + atomic_mb_set(&bh->scheduled, 0); } /* This func is async.The bottom half will do the delete action at the finial -- 1.8.3.1