Re: [Qemu-devel] [PATCH v1 2/2] block/archipelago: Use QEMU atomic builtins

[Chrysostomos Nanakos]

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 <address@hidden>
> > ---
> >   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);
> >           }
> >       }