From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59555) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOP3F-000453-C9 for qemu-devel@nongnu.org; Mon, 01 Sep 2014 06:41:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOP37-0003jf-Qt for qemu-devel@nongnu.org; Mon, 01 Sep 2014 06:41:33 -0400 Received: from averel.grnet-hq.admin.grnet.gr ([195.251.29.3]:14821) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOP37-0003jR-FZ for qemu-devel@nongnu.org; Mon, 01 Sep 2014 06:41:25 -0400 Message-ID: <54044CE0.4070500@grnet.gr> Date: Mon, 01 Sep 2014 13:39:28 +0300 From: Chrysostomos Nanakos MIME-Version: 1.0 References: <1409561921-4049-1-git-send-email-cnanakos@grnet.gr> <1409561921-4049-2-git-send-email-cnanakos@grnet.gr> <54043FC6.9090206@redhat.com> <540446B3.8010907@grnet.gr> <54044B85.7040608@redhat.com> In-Reply-To: <54044B85.7040608@redhat.com> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v1 2/2] block/archipelago: Use QEMU atomic builtins List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: kwolf@redhat.com, stefanha@redhat.com On 09/01/2014 01:33 PM, Paolo Bonzini wrote: > Il 01/09/2014 12:13, Chrysostomos Nanakos ha scritto: >>>> - if ((__sync_add_and_fetch(&segreq->ref, -segments_nr + i)) >>>> == 0) { >>>> + if ((atomic_add_fetch(&segreq->ref, -segments_nr + i)) == 0) { >> >> What about this one? It seems easier to me to read the above and >> understand what is going on. >> >> By any means, it's up to you to accept patch 1 :) > Yeah, you win on this one. :) But this code could use some > refactoring... > > Also, there is also a missing memory barrier before the > first archipelago_submit_request call, and setting "failed" does > not need an atomic operation. Have to check the refactoring code thoroughly but at a first glance seems to be fine. I'll come back with a new patch for block/archipelago.c then. Thanks very much for your time and effort! Regards, Chrysostomos. > Something like this (untested): > > diff --git a/block/archipelago.c b/block/archipelago.c > index 34f72dc..c71898a 100644 > --- a/block/archipelago.c > +++ b/block/archipelago.c > @@ -824,76 +824,47 @@ static int archipelago_aio_segmented_rw(BDRVArchipelagoState *s, > ArchipelagoAIOCB *aio_cb, > int op) > { > - int i, ret, segments_nr, last_segment_size; > + int i, ret, segments_nr; > + size_t pos; > ArchipelagoSegmentedRequest *segreq; > > - segreq = g_new(ArchipelagoSegmentedRequest, 1); > + segreq = g_new0(ArchipelagoSegmentedRequest, 1); > > if (op == ARCHIP_OP_FLUSH) { > segments_nr = 1; > - segreq->ref = segments_nr; > - segreq->total = count; > - segreq->count = 0; > - segreq->failed = 0; > - ret = archipelago_submit_request(s, 0, count, offset, aio_cb, > - segreq, ARCHIP_OP_FLUSH); > - if (ret < 0) { > - goto err_exit; > - } > - return 0; > + } else { > + segments_nr = (int)(count / MAX_REQUEST_SIZE) + \ > + ((count % MAX_REQUEST_SIZE) ? 1 : 0); > } > > - segments_nr = (int)(count / MAX_REQUEST_SIZE) + \ > - ((count % MAX_REQUEST_SIZE) ? 1 : 0); > - last_segment_size = (int)(count % MAX_REQUEST_SIZE); > - > - segreq->ref = segments_nr; > segreq->total = count; > - segreq->count = 0; > - segreq->failed = 0; > - > - for (i = 0; i < segments_nr - 1; i++) { > - ret = archipelago_submit_request(s, i * MAX_REQUEST_SIZE, > - MAX_REQUEST_SIZE, > - offset + i * MAX_REQUEST_SIZE, > - aio_cb, segreq, op); > - > + atomic_mb_set(&segreq->ref, segments_nr); > + > + pos = 0; > + for (; segments_nr > 1; segments_nr--) { > + ret = archipelago_submit_request(s, pos, > + MAX_REQUEST_SIZE, > + offset + pos, > + aio_cb, segreq, op); > if (ret < 0) { > goto err_exit; > } > + count -= MAX_REQUEST_SIZE; > + pos += MAX_REQUEST_SIZE; > } > > - if ((segments_nr > 1) && last_segment_size) { > - ret = archipelago_submit_request(s, i * MAX_REQUEST_SIZE, > - last_segment_size, > - offset + i * MAX_REQUEST_SIZE, > - aio_cb, segreq, op); > - } else if ((segments_nr > 1) && !last_segment_size) { > - ret = archipelago_submit_request(s, i * MAX_REQUEST_SIZE, > - MAX_REQUEST_SIZE, > - offset + i * MAX_REQUEST_SIZE, > - aio_cb, segreq, op); > - } else if (segments_nr == 1) { > - ret = archipelago_submit_request(s, 0, count, offset, aio_cb, > - segreq, op); > - } > - > + ret = archipelago_submit_request(s, pos, count, > + offset + pos, > + aio_cb, segreq, op); > if (ret < 0) { > goto err_exit; > } > - > return 0; > > err_exit: > - __sync_add_and_fetch(&segreq->failed, 1); > - if (segments_nr == 1) { > - if (__sync_add_and_fetch(&segreq->ref, -1) == 0) { > - g_free(segreq); > - } > - } else { > - if ((__sync_add_and_fetch(&segreq->ref, -segments_nr + i)) == 0) { > - g_free(segreq); > - } > + segreq->failed = 1; > + if (atomic_fetch_sub(&segreq->ref, segments_nr) == segments_nr) { > + g_free(segreq); > } > > return ret; >