From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55588) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOOdo-0007hJ-JN for qemu-devel@nongnu.org; Mon, 01 Sep 2014 06:15:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XOOdg-0004GU-N7 for qemu-devel@nongnu.org; Mon, 01 Sep 2014 06:15:16 -0400 Received: from averel.grnet-hq.admin.grnet.gr ([195.251.29.3]:13819) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XOOdg-0004Fl-GE for qemu-devel@nongnu.org; Mon, 01 Sep 2014 06:15:08 -0400 Message-ID: <540446B3.8010907@grnet.gr> Date: Mon, 01 Sep 2014 13:13:07 +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> In-Reply-To: <54043FC6.9090206@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 12:43 PM, Paolo Bonzini wrote: > Il 01/09/2014 10:58, Chrysostomos Nanakos ha scritto: >> Replace __sync builtins with the ones provided by QEMU >> for atomic operations. >> >> Signed-off-by: Chrysostomos Nanakos >> --- >> block/archipelago.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/block/archipelago.c b/block/archipelago.c >> index 34f72dc..fa8cd29 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_add_fetch(&segreq->ref, -1)) == 0) { > Why not just use "== 1" and avoid patch 1? :) > > (Also, you could use atomic_fetch_dec). Nice catch! > >> 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_add_fetch(&segreq->ref, -1)) == 0) { >> if (!segreq->failed) { >> reqdata->aio_cb->ret = segreq->count; >> archipelago_finish_aiocb(reqdata); >> @@ -885,13 +886,13 @@ static int archipelago_aio_segmented_rw(BDRVArchipelagoState *s, >> return 0; >> >> err_exit: >> - __sync_add_and_fetch(&segreq->failed, 1); >> + atomic_add_fetch(&segreq->failed, 1); > You can use atomic_inc here. Yes atomic_inc seems to be a lot better. Thanks! > > Paolo > >> if (segments_nr == 1) { >> - if (__sync_add_and_fetch(&segreq->ref, -1) == 0) { >> + if (atomic_add_fetch(&segreq->ref, -1) == 0) { >> g_free(segreq); >> } >> } else { >> - 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 :) Regards, Chrysostomos. >> g_free(segreq); >> } >> } >>