* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.