qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/3] block: make BlockBackend->quiesce_counter atomic


From: Hanna Czenczek
Subject: Re: [PATCH 1/3] block: make BlockBackend->quiesce_counter atomic
Date: Fri, 3 Mar 2023 16:29:54 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1

On 27.02.23 21:57, Stefan Hajnoczi wrote:
The main loop thread increments/decrements BlockBackend->quiesce_counter
when drained sections begin/end. The counter is read in the I/O code
path. Therefore this field is used to communicate between threads
without a lock.

Use qatomic_set()/qatomic_read() to make it clear that this field is
accessed by multiple threads.

Acquire/release are not necessary because the BlockBackend->in_flight
counter already uses sequentially consistent accesses and running I/O
requests hold that counter when blk_wait_while_drained() is called.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
  block/block-backend.c | 18 +++++++++++-------
  1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 278b04ce69..f00bf2ab35 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c

[...]

@@ -2568,7 +2568,9 @@ static void blk_root_drained_begin(BdrvChild *child)
      BlockBackend *blk = child->opaque;
      ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
- if (++blk->quiesce_counter == 1) {
+    int new_counter = qatomic_read(&blk->quiesce_counter) + 1;
+    qatomic_set(&blk->quiesce_counter, new_counter);
+    if (new_counter == 1) {
          if (blk->dev_ops && blk->dev_ops->drained_begin) {
              blk->dev_ops->drained_begin(blk->dev_opaque);
          }

[...]

@@ -2597,12 +2599,14 @@ static bool blk_root_drained_poll(BdrvChild *child)

[...]

      assert(blk->public.throttle_group_member.io_limits_disabled);
      qatomic_dec(&blk->public.throttle_group_member.io_limits_disabled);
- if (--blk->quiesce_counter == 0) {
+    int new_counter = qatomic_read(&blk->quiesce_counter) - 1;
+    qatomic_set(&blk->quiesce_counter, new_counter);

I don’t quite understand why you decided not to use simple atomic increments/decrements with just SeqCst in these places.  Maybe it is fine this way, but it isn’t trivial to see.  As far as I understand, these aren’t hot paths, so I don’t think we’d lose performance by using fully atomic operations here.

Hanna




reply via email to

[Prev in Thread] Current Thread [Next in Thread]