block/archipelago: Use QEMU atomic builtins

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 <cnanakos@grnet.gr>
---
 block/archipelago.c |   76 ++++++++++++++++-----------------------------------
 1 file changed, 23 insertions(+), 53 deletions(-)

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 <cnanakos@grnet.gr>
> ---
>  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 <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_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

On Tue, Sep 02, 2014 at 03:41:34PM +0300, Chrysostomos Nanakos wrote:
> 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 <cnanakos@grnet.gr>
> ---
>  block/archipelago.c |   76 ++++++++++++++++-----------------------------------
>  1 file changed, 23 insertions(+), 53 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan