Richard Henderson <richard.hender...@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.
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) {
- paolo:
PostcopyState postcopy_state_get(void)
{
- return qatomic_mb_read(&incoming_postcopy_state);
+ return qatomic_load_acquire(&incoming_postcopy_state);
}
> You're using an atomic 8-byte operation on a host that doesn't support
> it. Did you use qatomic_read__nocheck instead of qatomic_read to try
> and get around a build failure on i686? The check is there for a
> reason...
No, I am changing all ram_counters values to atomic. Almost all of them
move from [u]int64_t to Stat64. Notice that I don't care about 63 to
64bits, and anyways I think it was an error that they were int64_t on
the frist place (blame the old days qapi whet it didn't have unsigned
types).
But it don't exist a stat64_set() function. The most similar thing that
appears here is stat64_init(), but it is cheating about not being atomic
at all.
Almost all ram_counters values are ok with stat64_add() and stat64_get()
operations. But some of them, we need to reset them to zero (or
someother value, but that would not be complicated).
(*) And here is where it comes the call sentence from the 1st paragraph,
see how the stat64_get() gets implemented for the !CONFIG_ATOMIC64, I
didn't even try to write a stat64_set() on my own.
This is one example of the use that I had:
if (qatomic_read__nocheck(&ram_counters.dirty_pages_rate) &&
transferred > 10000) {
- s->expected_downtime = ram_counters.remaining / bandwidth;
+ s->expected_downtime =
+ qatomic_read__nocheck(&ram_counters.dirty_bytes_last_sync) /
+ bandwidth;
}
qemu_file_reset_rate_limit(s->to_dst_file);
diff --git a/migration/ram.c b/migration/ram.c
index 7400abf5e1..7bbaf8cd86 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1224,7 +1224,8 @@ static void migration_bitmap_sync(RAMState *rs)
RAMBLOCK_FOREACH_NOT_IGNORED(block) {
ramblock_sync_dirty_bitmap(rs, block);
}
- ram_counters.remaining = ram_bytes_remaining();
+ qatomic_set__nocheck(&ram_counters.dirty_bytes_last_sync,
+ ram_bytes_remaining());
:
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:
./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.
And:
./ram.c:382: uint64_t num_dirty_pages_period;
./ram.c:746: rs->num_dirty_pages_period = 0;
./ram.c:1095: rs->num_dirty_pages_period += new_dirty_pages;
./ram.c:1133: rs->num_dirty_pages_period * 1000 /
./ram.c:1184: uint64_t bytes_dirty_period = rs->num_dirty_pages_period *
TARGET_PAGE_SIZE;
./ram.c:1232: trace_migration_bitmap_sync_end(rs->num_dirty_pages_period);
./ram.c:1246: rs->num_dirty_pages_period = 0;
The problem here is that we reset the value every second, but for
everything else it is an stat64.
Thanks, Juan.
