From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36671) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eC6uF-0005Co-L4 for qemu-devel@nongnu.org; Tue, 07 Nov 2017 11:39:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eC6uB-0007HI-5O for qemu-devel@nongnu.org; Tue, 07 Nov 2017 11:39:19 -0500 References: <20171107150937.23188-1-slp@redhat.com> From: Paolo Bonzini Message-ID: <389cf59d-01df-6269-1933-027b736cd2eb@redhat.com> Date: Tue, 7 Nov 2017 17:38:57 +0100 MIME-Version: 1.0 In-Reply-To: <20171107150937.23188-1-slp@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] util/async: use atomic_mb_set in qemu_bh_cancel List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergio Lopez , qemu-devel@nongnu.org Cc: qemu-block@nongnu.org, famz@redhat.com, stefanha@redhat.com On 07/11/2017 16:09, Sergio Lopez wrote: > 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, under certain access > patterns and scheduling conditions, the loop may wrongly use a > pre-fetched elem->state value, reading it as THREAD_QUEUED, and ending > the completion function without having processed a pending TPE linked at > pool->head. The commit message probably should include an example of the scenario: I/O thread worker thread -------------------------------------------------------------------------------- speculatively read req->state req->state = THREAD_DONE; qemu_bh_schedule(pool->completion_bh) bh->scheduled = 1; qemu_bh_cancel(pool->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. Paolo > 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. > > Signed-off-by: Sergio Lopez > --- > util/async.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/util/async.c b/util/async.c > index 355af73ee7..0e1bd8780a 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 >