[Qemu-devel] [RFC PATCH] migration: do not use atomic__nocheck() functions directly

[Qemu-devel] [RFC PATCH] migration: do not use atomic__nocheck() functions directly

[Philippe Mathieu-Daudé]

(incorrectly use in 3be98be4e9f)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
currently on ppc32 the linking fails:

  CC      migration/postcopy-ram.o
...
  LINK    microblaze-softmmu/qemu-system-microblaze
../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_end':
migration/postcopy-ram.c:717: undefined reference to `__atomic_fetch_add_8'
migration/postcopy-ram.c:738: undefined reference to `__atomic_fetch_add_8'
../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_begin':
migration/postcopy-ram.c:651: undefined reference to `__atomic_exchange_8'
migration/postcopy-ram.c:652: undefined reference to `__atomic_exchange_8'
migration/postcopy-ram.c:661: undefined reference to `__atomic_exchange_8'
collect2: error: ld returned 1 exit status
Makefile:193: recipe for target 'qemu-system-microblaze' failed
make[1]: *** [qemu-system-microblaze] Error 1

with this patch the compilation fails:

  CC      migration/postcopy-ram.o
In file included from include/qemu/osdep.h:36:0,
                 from migration/postcopy-ram.c:19:
migration/postcopy-ram.c: In function 'mark_postcopy_blocktime_begin':
include/qemu/compiler.h:86:30: error: static assertion failed: "not expecting: sizeof(*&dc->last_begin) > ATOMIC_REG_SIZE"
 #define QEMU_BUILD_BUG_ON(x) _Static_assert(!(x), "not expecting: " #x)
                              ^
include/qemu/atomic.h:183:5: note: in expansion of macro 'QEMU_BUILD_BUG_ON'
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE);      \
     ^
migration/postcopy-ram.c:651:5: note: in expansion of macro 'atomic_xchg'
     atomic_xchg(&dc->last_begin, now_ms);
     ^

 migration/postcopy-ram.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 7814da5b4b..6ecc1aa820 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -648,17 +648,17 @@ static void mark_postcopy_blocktime_begin(uintptr_t addr, uint32_t ptid,
         atomic_inc(&dc->smp_cpus_down);
     }
 
-    atomic_xchg__nocheck(&dc->last_begin, now_ms);
-    atomic_xchg__nocheck(&dc->page_fault_vcpu_time[cpu], now_ms);
-    atomic_xchg__nocheck(&dc->vcpu_addr[cpu], addr);
+    atomic_xchg(&dc->last_begin, now_ms);
+    atomic_xchg(&dc->page_fault_vcpu_time[cpu], now_ms);
+    atomic_xchg(&dc->vcpu_addr[cpu], addr);
 
     /* check it here, not at the begining of the function,
      * due to, check could accur early than bitmap_set in
      * qemu_ufd_copy_ioctl */
     already_received = ramblock_recv_bitmap_test(rb, (void *)addr);
     if (already_received) {
-        atomic_xchg__nocheck(&dc->vcpu_addr[cpu], 0);
-        atomic_xchg__nocheck(&dc->page_fault_vcpu_time[cpu], 0);
+        atomic_xchg(&dc->vcpu_addr[cpu], 0);
+        atomic_xchg(&dc->page_fault_vcpu_time[cpu], 0);
         atomic_dec(&dc->smp_cpus_down);
     }
     trace_mark_postcopy_blocktime_begin(addr, dc, dc->page_fault_vcpu_time[cpu],
@@ -719,7 +719,7 @@ static void mark_postcopy_blocktime_end(uintptr_t addr)
             read_vcpu_time == 0) {
             continue;
         }
-        atomic_xchg__nocheck(&dc->vcpu_addr[i], 0);
+        atomic_xchg(&dc->vcpu_addr[i], 0);
         vcpu_blocktime = now_ms - read_vcpu_time;
         affected_cpu += 1;
         /* we need to know is that mark_postcopy_end was due to
-- 
2.15.1

Re: [Qemu-devel] [RFC PATCH] migration: do not use atomic__nocheck() functions directly

[Philippe Mathieu-Daudé]

On Fri, Jan 19, 2018 at 11:39 AM, Philippe Mathieu-Daudé
<f4bug@amsat.org> wrote:
> (incorrectly use in 3be98be4e9f)
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> currently on ppc32 the linking fails:

Well armel and armhf hosts are also failing since 3be98be4e9f.

>
>   CC      migration/postcopy-ram.o
> ...
>   LINK    microblaze-softmmu/qemu-system-microblaze
> ../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_end':
> migration/postcopy-ram.c:717: undefined reference to `__atomic_fetch_add_8'
> migration/postcopy-ram.c:738: undefined reference to `__atomic_fetch_add_8'
> ../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_begin':
> migration/postcopy-ram.c:651: undefined reference to `__atomic_exchange_8'
> migration/postcopy-ram.c:652: undefined reference to `__atomic_exchange_8'
> migration/postcopy-ram.c:661: undefined reference to `__atomic_exchange_8'
> collect2: error: ld returned 1 exit status
> Makefile:193: recipe for target 'qemu-system-microblaze' failed
> make[1]: *** [qemu-system-microblaze] Error 1
>
> with this patch the compilation fails:
>
>   CC      migration/postcopy-ram.o
> In file included from include/qemu/osdep.h:36:0,
>                  from migration/postcopy-ram.c:19:
> migration/postcopy-ram.c: In function 'mark_postcopy_blocktime_begin':
> include/qemu/compiler.h:86:30: error: static assertion failed: "not expecting: sizeof(*&dc->last_begin) > ATOMIC_REG_SIZE"
>  #define QEMU_BUILD_BUG_ON(x) _Static_assert(!(x), "not expecting: " #x)
>                               ^
> include/qemu/atomic.h:183:5: note: in expansion of macro 'QEMU_BUILD_BUG_ON'
>      QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE);      \
>      ^
> migration/postcopy-ram.c:651:5: note: in expansion of macro 'atomic_xchg'
>      atomic_xchg(&dc->last_begin, now_ms);
>      ^
>
>  migration/postcopy-ram.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 7814da5b4b..6ecc1aa820 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -648,17 +648,17 @@ static void mark_postcopy_blocktime_begin(uintptr_t addr, uint32_t ptid,
>          atomic_inc(&dc->smp_cpus_down);
>      }
>
> -    atomic_xchg__nocheck(&dc->last_begin, now_ms);
> -    atomic_xchg__nocheck(&dc->page_fault_vcpu_time[cpu], now_ms);
> -    atomic_xchg__nocheck(&dc->vcpu_addr[cpu], addr);
> +    atomic_xchg(&dc->last_begin, now_ms);
> +    atomic_xchg(&dc->page_fault_vcpu_time[cpu], now_ms);
> +    atomic_xchg(&dc->vcpu_addr[cpu], addr);
>
>      /* check it here, not at the begining of the function,
>       * due to, check could accur early than bitmap_set in
>       * qemu_ufd_copy_ioctl */
>      already_received = ramblock_recv_bitmap_test(rb, (void *)addr);
>      if (already_received) {
> -        atomic_xchg__nocheck(&dc->vcpu_addr[cpu], 0);
> -        atomic_xchg__nocheck(&dc->page_fault_vcpu_time[cpu], 0);
> +        atomic_xchg(&dc->vcpu_addr[cpu], 0);
> +        atomic_xchg(&dc->page_fault_vcpu_time[cpu], 0);
>          atomic_dec(&dc->smp_cpus_down);
>      }
>      trace_mark_postcopy_blocktime_begin(addr, dc, dc->page_fault_vcpu_time[cpu],
> @@ -719,7 +719,7 @@ static void mark_postcopy_blocktime_end(uintptr_t addr)
>              read_vcpu_time == 0) {
>              continue;
>          }
> -        atomic_xchg__nocheck(&dc->vcpu_addr[i], 0);
> +        atomic_xchg(&dc->vcpu_addr[i], 0);
>          vcpu_blocktime = now_ms - read_vcpu_time;
>          affected_cpu += 1;
>          /* we need to know is that mark_postcopy_end was due to
> --
> 2.15.1
>

Re: [Qemu-devel] [RFC PATCH] migration: do not use atomic__nocheck() functions directly

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
> (incorrectly use in 3be98be4e9f)
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I'm a bit confused; isnt the only difference between the nocheck
versions that it'll fail at compile time instead of link?

Dave

> ---
> currently on ppc32 the linking fails:
> 
>   CC      migration/postcopy-ram.o
> ...
>   LINK    microblaze-softmmu/qemu-system-microblaze
> ../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_end':
> migration/postcopy-ram.c:717: undefined reference to `__atomic_fetch_add_8'
> migration/postcopy-ram.c:738: undefined reference to `__atomic_fetch_add_8'
> ../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_begin':
> migration/postcopy-ram.c:651: undefined reference to `__atomic_exchange_8'
> migration/postcopy-ram.c:652: undefined reference to `__atomic_exchange_8'
> migration/postcopy-ram.c:661: undefined reference to `__atomic_exchange_8'
> collect2: error: ld returned 1 exit status
> Makefile:193: recipe for target 'qemu-system-microblaze' failed
> make[1]: *** [qemu-system-microblaze] Error 1
> 
> with this patch the compilation fails:
> 
>   CC      migration/postcopy-ram.o
> In file included from include/qemu/osdep.h:36:0,
>                  from migration/postcopy-ram.c:19:
> migration/postcopy-ram.c: In function 'mark_postcopy_blocktime_begin':
> include/qemu/compiler.h:86:30: error: static assertion failed: "not expecting: sizeof(*&dc->last_begin) > ATOMIC_REG_SIZE"
>  #define QEMU_BUILD_BUG_ON(x) _Static_assert(!(x), "not expecting: " #x)
>                               ^
> include/qemu/atomic.h:183:5: note: in expansion of macro 'QEMU_BUILD_BUG_ON'
>      QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE);      \
>      ^
> migration/postcopy-ram.c:651:5: note: in expansion of macro 'atomic_xchg'
>      atomic_xchg(&dc->last_begin, now_ms);
>      ^
> 
>  migration/postcopy-ram.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 7814da5b4b..6ecc1aa820 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -648,17 +648,17 @@ static void mark_postcopy_blocktime_begin(uintptr_t addr, uint32_t ptid,
>          atomic_inc(&dc->smp_cpus_down);
>      }
>  
> -    atomic_xchg__nocheck(&dc->last_begin, now_ms);
> -    atomic_xchg__nocheck(&dc->page_fault_vcpu_time[cpu], now_ms);
> -    atomic_xchg__nocheck(&dc->vcpu_addr[cpu], addr);
> +    atomic_xchg(&dc->last_begin, now_ms);
> +    atomic_xchg(&dc->page_fault_vcpu_time[cpu], now_ms);
> +    atomic_xchg(&dc->vcpu_addr[cpu], addr);
>  
>      /* check it here, not at the begining of the function,
>       * due to, check could accur early than bitmap_set in
>       * qemu_ufd_copy_ioctl */
>      already_received = ramblock_recv_bitmap_test(rb, (void *)addr);
>      if (already_received) {
> -        atomic_xchg__nocheck(&dc->vcpu_addr[cpu], 0);
> -        atomic_xchg__nocheck(&dc->page_fault_vcpu_time[cpu], 0);
> +        atomic_xchg(&dc->vcpu_addr[cpu], 0);
> +        atomic_xchg(&dc->page_fault_vcpu_time[cpu], 0);
>          atomic_dec(&dc->smp_cpus_down);
>      }
>      trace_mark_postcopy_blocktime_begin(addr, dc, dc->page_fault_vcpu_time[cpu],
> @@ -719,7 +719,7 @@ static void mark_postcopy_blocktime_end(uintptr_t addr)
>              read_vcpu_time == 0) {
>              continue;
>          }
> -        atomic_xchg__nocheck(&dc->vcpu_addr[i], 0);
> +        atomic_xchg(&dc->vcpu_addr[i], 0);
>          vcpu_blocktime = now_ms - read_vcpu_time;
>          affected_cpu += 1;
>          /* we need to know is that mark_postcopy_end was due to
> -- 
> 2.15.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH] migration: do not use atomic__nocheck() functions directly

[Philippe Mathieu-Daudé]

On 01/19/2018 03:01 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
>> (incorrectly use in 3be98be4e9f)
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> I'm a bit confused; isnt the only difference between the nocheck
> versions that it'll fail at compile time instead of link?

You are right, this isn't the right approach.

Peter gave a clearer explanation here:
https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg02103.html

'''
because atomic operations on types larger than the host pointer
size are not portable to all architectures. This should probably
use the atomic_cmpxchg(), not __nocheck, because the latter
bypasses the build time assert for this problem and turns a
"fails on any 32-bit host" into "fails obscurely on some
architectures only".
'''

Maybe we should remove the __nocheck() functions and inline them in the
'checked' functions?
There is no performance cost doing this, right?

> 
> Dave
> 
>> ---
>> currently on ppc32 the linking fails:
>>
>>   CC      migration/postcopy-ram.o
>> ...
>>   LINK    microblaze-softmmu/qemu-system-microblaze
>> ../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_end':
>> migration/postcopy-ram.c:717: undefined reference to `__atomic_fetch_add_8'
>> migration/postcopy-ram.c:738: undefined reference to `__atomic_fetch_add_8'
>> ../migration/postcopy-ram.o: In function `mark_postcopy_blocktime_begin':
>> migration/postcopy-ram.c:651: undefined reference to `__atomic_exchange_8'
>> migration/postcopy-ram.c:652: undefined reference to `__atomic_exchange_8'
>> migration/postcopy-ram.c:661: undefined reference to `__atomic_exchange_8'
>> collect2: error: ld returned 1 exit status
>> Makefile:193: recipe for target 'qemu-system-microblaze' failed
>> make[1]: *** [qemu-system-microblaze] Error 1
>>
>> with this patch the compilation fails:
>>
>>   CC      migration/postcopy-ram.o
>> In file included from include/qemu/osdep.h:36:0,
>>                  from migration/postcopy-ram.c:19:
>> migration/postcopy-ram.c: In function 'mark_postcopy_blocktime_begin':
>> include/qemu/compiler.h:86:30: error: static assertion failed: "not expecting: sizeof(*&dc->last_begin) > ATOMIC_REG_SIZE"
>>  #define QEMU_BUILD_BUG_ON(x) _Static_assert(!(x), "not expecting: " #x)
>>                               ^
>> include/qemu/atomic.h:183:5: note: in expansion of macro 'QEMU_BUILD_BUG_ON'
>>      QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE);      \
>>      ^
>> migration/postcopy-ram.c:651:5: note: in expansion of macro 'atomic_xchg'
>>      atomic_xchg(&dc->last_begin, now_ms);
>>      ^
>>
>>  migration/postcopy-ram.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
>> index 7814da5b4b..6ecc1aa820 100644
>> --- a/migration/postcopy-ram.c
>> +++ b/migration/postcopy-ram.c
>> @@ -648,17 +648,17 @@ static void mark_postcopy_blocktime_begin(uintptr_t addr, uint32_t ptid,
>>          atomic_inc(&dc->smp_cpus_down);
>>      }
>>  
>> -    atomic_xchg__nocheck(&dc->last_begin, now_ms);
>> -    atomic_xchg__nocheck(&dc->page_fault_vcpu_time[cpu], now_ms);
>> -    atomic_xchg__nocheck(&dc->vcpu_addr[cpu], addr);
>> +    atomic_xchg(&dc->last_begin, now_ms);
>> +    atomic_xchg(&dc->page_fault_vcpu_time[cpu], now_ms);
>> +    atomic_xchg(&dc->vcpu_addr[cpu], addr);
>>  
>>      /* check it here, not at the begining of the function,
>>       * due to, check could accur early than bitmap_set in
>>       * qemu_ufd_copy_ioctl */
>>      already_received = ramblock_recv_bitmap_test(rb, (void *)addr);
>>      if (already_received) {
>> -        atomic_xchg__nocheck(&dc->vcpu_addr[cpu], 0);
>> -        atomic_xchg__nocheck(&dc->page_fault_vcpu_time[cpu], 0);
>> +        atomic_xchg(&dc->vcpu_addr[cpu], 0);
>> +        atomic_xchg(&dc->page_fault_vcpu_time[cpu], 0);
>>          atomic_dec(&dc->smp_cpus_down);
>>      }
>>      trace_mark_postcopy_blocktime_begin(addr, dc, dc->page_fault_vcpu_time[cpu],
>> @@ -719,7 +719,7 @@ static void mark_postcopy_blocktime_end(uintptr_t addr)
>>              read_vcpu_time == 0) {
>>              continue;
>>          }
>> -        atomic_xchg__nocheck(&dc->vcpu_addr[i], 0);
>> +        atomic_xchg(&dc->vcpu_addr[i], 0);
>>          vcpu_blocktime = now_ms - read_vcpu_time;
>>          affected_cpu += 1;
>>          /* we need to know is that mark_postcopy_end was due to
>> -- 
>> 2.15.1
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

Re: [Qemu-devel] [RFC PATCH] migration: do not use atomic__nocheck() functions directly

[Richard Henderson]

On 01/19/2018 10:20 AM, Philippe Mathieu-Daudé wrote:
> Maybe we should remove the __nocheck() functions and inline them in the
> 'checked' functions?

No, I use them in TCG, properly protected with CONFIG_ATOMIC*.


r~