Re: [PULL 00/20] Migration 20230420 patches

[Richard Henderson <richard.henderson@linaro.org>]

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:
>>> The following changes since commit 2d82c32b2ceaca3dc3da5e36e10976f34bfcb598:
>>>     Open 8.1 development tree (2023-04-20 10:05:25 +0100)
>>> are available in the Git repository at:
>>>     https://gitlab.com/juan.quintela/qemu.git
>>> tags/migration-20230420-pull-request
>>> for you to fetch changes up to
>>> cdf07846e6fe07a2e20c93eed5902114dc1d3dcf:
>>>     migration: Pass migrate_caps_check() the old and new caps
>>> (2023-04-20 15:10:58 +0200)
>>> ----------------------------------------------------------------
>>> Migration Pull request
>>> This series include everything reviewed for migration:
>>> - fix for disk stop/start (eric)
>>> - detect filesystem of hostmem (peter)
>>> - rename qatomic_mb_read (paolo)
>>> - whitespace cleanup (李皆俊)
>>>     I hope copy and paste work for the name O:-)
>>> - atomic_counters series (juan)
>>> - two first patches of capabilities (juan)
>>> Please apply,
>>
>> Fails CI:
>> https://gitlab.com/qemu-project/qemu/-/jobs/4159279870#L2896
>>
>> /usr/lib/gcc-cross/mipsel-linux-gnu/10/../../../../mipsel-linux-gnu/bin/ld:
>> libcommon.fa.p/migration_migration.c.o: undefined reference to symbol
>> '__atomic_load_8@@LIBATOMIC_1.0'
> 
> 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":
> 
> $ 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.)

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.


> 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...

> - paolo:
> 
>   PostcopyState  postcopy_state_get(void)
>   {
> -    return qatomic_mb_read(&incoming_postcopy_state);
> +    return qatomic_load_acquire(&incoming_postcopy_state);

... because this one was already atomic, with different barriers.

> 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.

> ./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.


r~