On 3.09.2024 17:01, Fabiano Rosas wrote:
"Maciej S. Szmigiero" <m...@maciej.szmigiero.name> writes:
On 30.08.2024 20:13, Fabiano Rosas wrote:
"Maciej S. Szmigiero" <m...@maciej.szmigiero.name> writes:
From: "Maciej S. Szmigiero" <maciej.szmigi...@oracle.com>
This is necessary for multifd_send() to be able to be called
from multiple threads.
Signed-off-by: Maciej S. Szmigiero <maciej.szmigi...@oracle.com>
---
migration/multifd.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index d5a8e5a9c9b5..b25789dde0b3 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
(..)
+
+ /* We wait here, until at least one channel is ready */
+ qemu_sem_wait(&multifd_send_state->channels_ready);
+
+ while (true) {
+ int i_next;
+
if (multifd_send_should_exit()) {
return false;
}
+
+ i = qatomic_load_acquire(&next_channel);
+ i_next = (i + 1) % migrate_multifd_channels();
+ if (qatomic_cmpxchg(&next_channel, i, i_next) != i) {
+ continue;
+ }
Say channel 'i' is the only one that's idle. What's stopping the other
thread(s) to race at this point and loop around to the same index?
See the reply below.
+
p = &multifd_send_state->params[i];
/*
* Lockless read to p->pending_job is safe, because only multifd
* sender thread can clear it.
*/
if (qatomic_read(&p->pending_job) == false) {
With the cmpxchg your other patch adds here, then the race I mentioned
above should be harmless. But we'd need to bring that code into this
patch.
You're right - the sender code with this patch alone isn't thread safe
yet but this commit is only literally about "converting
multifd_send()::next_channel to atomic".
At the time of this patch there aren't any multifd_send() calls from
multiple threads, and the commit that introduces such possible call
site (multifd_queue_device_state()) also modifies multifd_send()
to be fully thread safe by introducing p->pending_job_preparing.
In general this would be a bad practice because this commit can end up
being moved around due to backporting or bisecting. It would be better
if it were complete from the start. It also affects backporting due to
ambiguity on where the Fixes tag should point to if someone eventually
finds a bug.
I already asked you to extract the other code into a separate patch, so
it's not that bad. If you prefer to keep both changes separate for
clarity, please note on the commit message that the next patch is
necessary for correctness.
If someone picks parts of a patch set or reorders commits then I guess
in many cases things can break indeed.
But it looks like I will be able to move code changes around to have
multifd_send() already thread safe by the time of this commit so I
will do that.
Thanks,
Maciej
