On Thu, Sep 19, 2024 at 11:05:28AM -0300, Fabiano Rosas wrote:
> Peter Maydell <peter.mayd...@linaro.org> writes:
>
> > On Thu, 19 Sept 2024 at 12:59, Peter Xu <pet...@redhat.com> wrote:
> >>
> >> On Thu, Sep 19, 2024 at 10:08:25AM +0100, Peter Maydell wrote:
> >> > Thanks for looking at the issues with the migration tests.
> >> > This run went through first time without my needing to retry any
> >> > jobs, so fingers crossed that we have at least improved the reliability.
> >> > (I have a feeling there's still something funny with the k8s runners,
> >> > but that's not migration-test specific, it's just that test tends
> >> > to be the longest running and so most likely to be affected.)
> >>
> >> Kudos all go to Fabiano for debugging the hard problem.
> >>
> >> And yes, please let either of us know if it fails again, we can either keep
> >> looking, or still can disable it when necessary (if it takes long to
> >> debug).
> >
> > On the subject of potential races in the migration code,
> > there's a couple of outstanding Coverity issues that might
> > be worth looking at. If they're false-positives let me know
> > and I can reclassify them in Coverity.
> >
> > CID 1527402: In migrate_fd_cleanup() Coverity thinks there's
> > a race because we read s->to_dst_file in the "if (s->to_dst_file)"
> > check without holding the qemu_file_lock. This might be a
> > false-positive because the race Coverity identifies happens
> > if two threads both call migrate_fd_cleanup() at the same
> > time, which is probably not permitted. (But OTOH taking a
> > mutex gets you for free any necessary memory barriers...)
>
> Yes, we shouldn't rely on mental gymnastics to prove that there's no
> concurrent access.
>
> @peterx, that RH bug you showed me could very well be caused by this
> race, except that I don't see how fd_cleanup could race with
> itself. Just having the lock would probably save us time even thinking
> about it.
I can send a patch for this one.
>
> >
> > CID 1527413: In postcopy_pause_incoming() we read
> > mis->postcopy_qemufile_dst without holding the
> > postcopy_prio_thread_mutex which we use to protect the write
> > to that field, so Coverity thinks there's a race if two
> > threads call this function at once.
>
> At first sight, it seems like a real problem. We did a good pass on
> these races on the source side, but the destination side hasn't been
> investigated yet.
I think it's probably safe and no real race could happen. The rational
being that during postcopy (aka, when ram load thread is running), this
postcopy_qemufile_dst can only be modified by one thread, which is the ram
listen thread itself. The other consumer of it is the fast load thread,
but that thread should only read postcopy_qemufile_dst, not update.
IOW, here the "if (postcopy_qemufile_dst)" is not about saying "whether
anyone else can released it", instead it's about "whether preempt mode is
enabled, and whether we have the fast channel at all".
When it's set, it means preempt is enabled, and it will never become NULL
in that case.
Here I _think_ we can replace it with a migrate_postcopy_preempt() safely,
because AFAICT that must be created when with it enabled. However the
tricky part is the next line, which does the shutdown():
qemu_file_shutdown(mis->postcopy_qemufile_dst);
That's unfortunately not replaceable, and that's so far the only way (and
pretty traditional way... as you pointed out below..) that we can kick one
qemufile read() out. In this specific case, the fast thread normally holds
the mutex meanwhile read() on this qemufile via one qemu_get_be64() of
ram_load_postcopy(), then we kick it out with the shutdown().
IOW both facts need to be true here for the fast load thread on: (1) mutex
held, and (2) block at qemu_get_be64(). So there's no way we can take the
mutex before the shutdown() or we deadlock.. it'll be easier if we have
other way to kick the qemu_get_be64(), e.g. if it could be a generic poll()
then we can have it poll on both the qemufile and another eventfd. But
we're stick with the qemufile API here..
So in short: I don't have a good and simple solution for this one.. but I
am 99.9999% sure it's safe.
>
> Unfortunately, these QEMUFile races are not trivial to fix due to
> several design pain points, such as:
>
> - the QEMUFile pointer validity being sometimes used to imply no error
> has happened before;
>
> - the various shutdown() calls that serve both as a way to kick a read()
> that's stuck, but also to cause some other part of the code to realise
> there has been an error (due to the point above);
>
> - the yank feature which has weird semantics regarding whether it
> operates on an iochannel or qemufile;
>
> - migrate_fd_cancel() that _can_ run concurrently with anything else;
>
> - the need to ensure the other end of migration also reacts to
> error/cancel on this side;
Right. Ultimately it's about relying on shutdown() for kicking qemufile
ops out as of now.. that _may_ not always hold locks. The other example
is qmp_yank() on a migration channel, which invokes the same iochannel
shutdown() via migration_yank_iochannel().
>
> >
> > (The only other migration Coverity issue is CID 1560071,
> > which is the "better to use pstrcpy()" not-really-a-bug
> > we discussed in another thread.)
> >
> > thanks
> > -- PMM
>
--
Peter Xu
