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

[Chrysostomos Nanakos <cnanakos@grnet.gr>]

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);
>>           }
>>       }
>>