Re: [PULL 00/20] Migration 20230420 patches

[Juan Quintela]

Richard Henderson <richard.henderson@linaro.org> wrote:
> On 4/22/23 10:21, Juan Quintela wrote:
>> Richard Henderson <richard.henderson@linaro.org> wrote:
>>> On 4/20/23 14:17, Juan Quintela wrote:

>> Hi Richard
>> First of all, I have no doubt that you know better that me in this
>> regard (*).
>> Once told that, it looks like one case of "my toolchain is better
>> than
>> yours":

Quotes were here for a reason O:-)

>> $ ls qemu-system-mips
>> qemu-system-mips        qemu-system-mips64el.p/ qemu-system-mipsel.p/
>> qemu-system-mips64      qemu-system-mips64.p/   qemu-system-mips.p/
>> qemu-system-mips64el    qemu-system-mipsel
>> This is Fedora37 with updates.
>
> I'm sure it's not true that "my toolchain is better", because mips32
> simply does not have the ability.  (And of course mips64 does, but
> that's a different test.)

It was a kind of apology to say that I had really compiled mipsel.  I
compile everything that has a crosscompiler in fedora:

TARGET_DIRS=aarch64-softmmu alpha-softmmu arm-softmmu avr-softmmu
cris-softmmu hppa-softmmu i386-softmmu loongarch64-softmmu m68k-softmmu
microblazeel-softmmu microblaze-softmmu mips64el-softmmu mips64-softmmu
mipsel-softmmu mips-softmmu nios2-softmmu or1k-softmmu ppc64-softmmu
ppc-softmmu riscv32-softmmu riscv64-softmmu rx-softmmu s390x-softmmu
sh4eb-softmmu sh4-softmmu sparc64-softmmu sparc-softmmu tricore-softmmu
x86_64-softmmu xtensaeb-softmmu xtensa-softmmu aarch64_be-linux-user
aarch64-linux-user alpha-linux-user armeb-linux-user arm-linux-user
cris-linux-user hexagon-linux-user hppa-linux-user i386-linux-user
loongarch64-linux-user m68k-linux-user microblazeel-linux-user
microblaze-linux-user mips64el-linux-user mips64-linux-user
mipsel-linux-user mips-linux-user mipsn32el-linux-user
mipsn32-linux-user nios2-linux-user or1k-linux-user ppc64le-linux-user
ppc64-linux-user ppc-linux-user riscv32-linux-user riscv64-linux-user
s390x-linux-user sh4eb-linux-user sh4-linux-user sparc32plus-linux-user
sparc64-linux-user sparc-linux-user x86_64-linux-user
xtensaeb-linux-user xtensa-linux-user

And I still get this "build" failures.

> I'll note that mips32 and armv6 (that is, *not* debian's armv7 based
> armhf distro) are the only hosts we have that don't have an atomic
> 8-byte operation.

This is the kind of trouble that I don'k now what to do.  I am pretty
sure that nobody is goigng to migrate a host that has so much RAM than
needs a 64bit counter in that two architectures (or any 32 architectures
for what is worth).

A couple of minutes after sending the 1st email, I considederd sending
another one saying "my toolchain lies better than yours".

I moved the atomic operations that do the buildcheck and run make again:

$ rm -f qemu-system-mips*
$ time make

[....]

[2/5] Linking target qemu-system-mipsel
[3/5] Linking target qemu-system-mips
[4/5] Linking target qemu-system-mips64el
[5/5] Linking target qemu-system-mips64

So clearly my toolchain is lying O:-)

>> There are two posibilities here that came to mind, in order of
>> probability;
>> - myself with:
>> -    if (ram_counters.dirty_pages_rate && transferred > 10000) {
>> +    if (qatomic_read__nocheck(&ram_counters.dirty_pages_rate) &&
>> +        transferred > 10000) {
>
> I think it's this one...

O:-)

>> and why I used qatomic_*__nocheck() instead of the proper operations?
>> Because reading this:
>> #define qatomic_read__nocheck(ptr) \
>>      __atomic_load_n(ptr, __ATOMIC_RELAXED)
>> #define qatomic_read(ptr)                              \
>>      ({                                                 \
>>      qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
>>      qatomic_read__nocheck(ptr);                        \
>>      })
>> #define qatomic_set__nocheck(ptr, i) \
>>      __atomic_store_n(ptr, i, __ATOMIC_RELAXED)
>> #define qatomic_set(ptr, i)  do {                      \
>>      qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
>>      qatomic_set__nocheck(ptr, i);                      \
>> } while(0)
>> I was complely sure that we will never get the qemu_build_assert().
>> I know, I know.
>
> :-)
>
>> And now that I have explained myself, what is the correct way of doing
>> this?
>> I declared the value as:
>> +    aligned_uint64_t dirty_bytes_last_sync;
>> -    int64_t remaining;
>> I just want to make sure that *all* ram_counters are atomic and then
>> I
>> can use them from any thread.  All the counters that use stat64 already
>> are.  But for this two to work, I would need to have a way to set and
>> old value.
>> And once that we are here, I would like ta have:
>> stat64_inc(): just add 1, I know, I can create a macro.
>> and
>> stat64_reset(): as its name says, it returns the value to zero.
>> I still miss a couple of stats in migration, where I need to reset
>> them
>> to zero from time to time:
>
> How critical are the statistics?  Are they integral to the algorithm
> or are they merely for diagnostics and user display?  What happens
> they're not atomic and we do race?
>
> If we really need atomicity, then the only answer is a mutex or spinlock.

I think we can extend Stat64 operations with at least stat64_reset()
operations.  What I don't want is that half of the counters need to be
updated with one spinlock and the other half with atomic operations, it
makes difficult to explain.

If I have the stat64_reset() operation, then stat64_set() becomes
stat64_reset() + stat64_add().  I put a wrapper and call it a day.  As
said, this one is not so speed critical.

Yes, other counters are speed critical, updated once for each transmited
page.  But others are only updated every time that we try to finish
migration or each iteration.  The two ones given trouble are in the last
kind.

>> ./ram.c:380:    uint64_t bytes_xfer_prev;
>> ./ram.c:747:    rs->bytes_xfer_prev = stat64_get(&ram_counters.transferred);
>> ./ram.c:1183:        stat64_get(&ram_counters.transferred) - 
>> rs->bytes_xfer_prev;
>> ./ram.c:1247:        rs->bytes_xfer_prev = 
>> stat64_get(&ram_counters.transferred);
>> You can clame that this operation happens always on the migration
>> thread, but I have found that it is more difficult to document which
>> ones are atomic and which not, that make all of them atomic.  This
>> variable are get/set once a second, so performance is not one of the
>> issues.
>
> For access once per second, it sounds like a spinlock would be fine.

Ortogonality is more important to me that speed here.  I mill wait if
someone (hint, hint) cames with an implementaton of stat64_clear() that
also works for non ATOMIC64 machines O:-)

Later, Juan.