From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55235) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOnqJ-0004mk-S8 for qemu-devel@nongnu.org; Tue, 02 Sep 2014 09:09:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOnqE-0005IM-Pn for qemu-devel@nongnu.org; Tue, 02 Sep 2014 09:09:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38328) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOnqE-0005Hi-Iq for qemu-devel@nongnu.org; Tue, 02 Sep 2014 09:09:46 -0400 Message-ID: <5405C18F.8080503@redhat.com> Date: Tue, 02 Sep 2014 15:09:35 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1409661695-32038-1-git-send-email-cnanakos@grnet.gr> In-Reply-To: <1409661695-32038-1-git-send-email-cnanakos@grnet.gr> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] block/archipelago: Use QEMU atomic builtins List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chrysostomos Nanakos , qemu-devel@nongnu.org Cc: kwolf@redhat.com, stefanha@redhat.com Il 02/09/2014 14:41, Chrysostomos Nanakos ha scritto: > Replace __sync builtins with ones provided by QEMU > for atomic operations. > > Special thanks goes to Paolo Bonzini for his refactoring > suggestion in order to use the already existing atomic builtins > interface. > > Signed-off-by: Chrysostomos Nanakos > --- > block/archipelago.c | 76 ++++++++++++++++----------------------------------- > 1 file changed, 23 insertions(+), 53 deletions(-) > > diff --git a/block/archipelago.c b/block/archipelago.c > index 34f72dc..22a7daa 100644 > --- a/block/archipelago.c > +++ b/block/archipelago.c > @@ -57,6 +57,7 @@ > #include "qapi/qmp/qint.h" > #include "qapi/qmp/qstring.h" > #include "qapi/qmp/qjson.h" > +#include "qemu/atomic.h" > > #include > #include > @@ -214,7 +215,7 @@ static void xseg_request_handler(void *state) > > xseg_put_request(s->xseg, req, s->srcport); > > - if ((__sync_add_and_fetch(&segreq->ref, -1)) == 0) { > + if (atomic_fetch_dec(&segreq->ref) == 1) { > if (!segreq->failed) { > reqdata->aio_cb->ret = segreq->count; > archipelago_finish_aiocb(reqdata); > @@ -233,7 +234,7 @@ static void xseg_request_handler(void *state) > segreq->count += req->serviced; > xseg_put_request(s->xseg, req, s->srcport); > > - if ((__sync_add_and_fetch(&segreq->ref, -1)) == 0) { > + if (atomic_fetch_dec(&segreq->ref) == 1) { > if (!segreq->failed) { > reqdata->aio_cb->ret = segreq->count; > archipelago_finish_aiocb(reqdata); > @@ -824,78 +825,47 @@ static int archipelago_aio_segmented_rw(BDRVArchipelagoState *s, > ArchipelagoAIOCB *aio_cb, > int op) > { > - int i, ret, segments_nr, last_segment_size; > + int ret, segments_nr; > + size_t pos = 0; > 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; > + atomic_mb_set(&segreq->ref, segments_nr); > > - 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); > + while (segments_nr > 1) { > + 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; > + segments_nr--; > } > - > - 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; > } > > Thanks for picking up the patch! Paolo