* [Qemu-devel] [PATCH v1 1/2] Extend header file for atomic operations @ 2014-09-01 8:58 Chrysostomos Nanakos 2014-09-01 8:58 ` [Qemu-devel] [PATCH v1 2/2] block/archipelago: Use QEMU atomic builtins Chrysostomos Nanakos 0 siblings, 1 reply; 6+ messages in thread From: Chrysostomos Nanakos @ 2014-09-01 8:58 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, pbonzini, Chrysostomos Nanakos, stefanha Add __sync_*_and_fetch builtins used in several places. Signed-off-by: Chrysostomos Nanakos <cnanakos@grnet.gr> --- include/qemu/atomic.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h index 492bce1..48fc283 100644 --- a/include/qemu/atomic.h +++ b/include/qemu/atomic.h @@ -189,6 +189,10 @@ #define atomic_fetch_sub __sync_fetch_and_sub #define atomic_fetch_and __sync_fetch_and_and #define atomic_fetch_or __sync_fetch_and_or +#define atomic_add_fetch __sync_add_and_fetch +#define atomic_sub_fetch __sync_sub_and_fetch +#define atomic_or_fetch __sync_or_and_fetch +#define atomic_and_fetch __sync_and_and_fetch #define atomic_cmpxchg __sync_val_compare_and_swap /* And even shorter names that return void. */ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v1 2/2] block/archipelago: Use QEMU atomic builtins 2014-09-01 8:58 [Qemu-devel] [PATCH v1 1/2] Extend header file for atomic operations Chrysostomos Nanakos @ 2014-09-01 8:58 ` Chrysostomos Nanakos 2014-09-01 9:43 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Chrysostomos Nanakos @ 2014-09-01 8:58 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, pbonzini, Chrysostomos Nanakos, stefanha Replace __sync builtins with the ones provided by QEMU for atomic operations. Signed-off-by: Chrysostomos Nanakos <cnanakos@grnet.gr> --- 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 <inttypes.h> #include <xseg/xseg.h> @@ -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) { 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); 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) { g_free(segreq); } } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1 2/2] block/archipelago: Use QEMU atomic builtins 2014-09-01 8:58 ` [Qemu-devel] [PATCH v1 2/2] block/archipelago: Use QEMU atomic builtins Chrysostomos Nanakos @ 2014-09-01 9:43 ` Paolo Bonzini 2014-09-01 10:13 ` Chrysostomos Nanakos 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2014-09-01 9:43 UTC (permalink / raw) To: Chrysostomos Nanakos, qemu-devel; +Cc: kwolf, stefanha 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 <cnanakos@grnet.gr> > --- > 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 <inttypes.h> > #include <xseg/xseg.h> > @@ -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). > 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. 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) { > g_free(segreq); > } > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1 2/2] block/archipelago: Use QEMU atomic builtins 2014-09-01 9:43 ` Paolo Bonzini @ 2014-09-01 10:13 ` Chrysostomos Nanakos 2014-09-01 10:33 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Chrysostomos Nanakos @ 2014-09-01 10:13 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel; +Cc: kwolf, stefanha 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 <cnanakos@grnet.gr> >> --- >> 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 <inttypes.h> >> #include <xseg/xseg.h> >> @@ -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); >> } >> } >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1 2/2] block/archipelago: Use QEMU atomic builtins 2014-09-01 10:13 ` Chrysostomos Nanakos @ 2014-09-01 10:33 ` Paolo Bonzini 2014-09-01 10:39 ` Chrysostomos Nanakos 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2014-09-01 10:33 UTC (permalink / raw) To: Chrysostomos Nanakos, qemu-devel; +Cc: kwolf, stefanha 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. 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; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1 2/2] block/archipelago: Use QEMU atomic builtins 2014-09-01 10:33 ` Paolo Bonzini @ 2014-09-01 10:39 ` Chrysostomos Nanakos 0 siblings, 0 replies; 6+ messages in thread From: Chrysostomos Nanakos @ 2014-09-01 10:39 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel; +Cc: kwolf, stefanha 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; > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-01 10:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-01 8:58 [Qemu-devel] [PATCH v1 1/2] Extend header file for atomic operations Chrysostomos Nanakos 2014-09-01 8:58 ` [Qemu-devel] [PATCH v1 2/2] block/archipelago: Use QEMU atomic builtins Chrysostomos Nanakos 2014-09-01 9:43 ` Paolo Bonzini 2014-09-01 10:13 ` Chrysostomos Nanakos 2014-09-01 10:33 ` Paolo Bonzini 2014-09-01 10:39 ` Chrysostomos Nanakos
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).