Hello Fabiano, On Fri, 28 Feb 2025 at 20:23, Fabiano Rosas <faro...@suse.de> wrote: > You forgot to split patch 2. We cannot have a commit that revamps the > channel discovery logic and enables a new feature at the same > time. Changing the channel discovery affects all the migration > use-cases, that change cannot be smuggled along with multifd+postcopy > enablement.
=== >> Continue with this patch and fix the stuff I mentioned. You can ignore >> the first two paragraphs of that reply. >> >> https://lore.kernel.org/r/87y0y4tf5q....@suse.de >> >> I still think we need to test that preempt + multifd scenario, but it >> should be easy to write a test for that once the series is in more of a >> final shape. === * I thought that major overhaul of the channel discovery part by moving it to channel.c was to be done subsequently, no? As we discussed, we need to touch the channel discovery parts in 'migration_ioc_process_incoming' because, we need to identify the Postcopy channel when its connection comes in. So the Patch-2 does minimal changes that are _required_ to enable the two (multifd & postcopy) features together. * Whereas, moving channel discovery parts out to connections.c could be done separately with its own reasoning that - we are moving it outside to connection.c because it fits better there. > Similarly, the multifd+postcopy enablement is a new feature that needs > to be tested and reasoned upon in isolation, it cannot bring along a > bunch of previously existing code that was shuffled around. We need to > be able to understand clearly what is done _in preparation for_ the > feature and what is done _as part of_ the feature. ... > Not to mention bisect and backporting. Many people will be looking at > this code in the future without any knowledge of migration, but as part > of a bisect section or when searching for missing backports in the > distros. * I think we (you, me, Peter) are all looking at things differently. - In my view Patch-2 is the minimal change _required_ to enable multifd & postcopy. In your view we are _revamping_ channel discovery parts while _sneaking_ in a feature of enabling multifd & postcopy together. - In my view Patch-5 in this series is an isolated change because it adds a new migration command to allow multifd threads sync from source side. But Peter thinks without that 'flush and sync' Patch-2 is incomplete, so we should merge it back there. * I've also shared my view about not trying to do everything in this one series. I don't know how do we move forward from here. I'm also not sure what the final _acceptable_ patch-set should look like. I thought a tested working patch-set is a good start. At this point I think I'll just follow your lead. > I also suggested to move that logic into channel.c. The code is now > well-contained enough that we don't need to be reading it every time > someone is going over the migration flow, it becomes just a helper > function. * How exactly do we want to divide patch-2 to move channel discovery parts to channel.c? === if (!migration_has_main_and_multifd_channels()) { } ... } else { channel = CH_MAIN; } === Do we move this entire if - else - else block to channel.c from migration_ioc_process_incoming() ? > > === > > 67/67 qemu:qtest+qtest-x86_64 / qtest-x86_64/migration-test > > OK 147.84s 81 subtests passed > > I see postcopy/multifd/plain hanging from time to time. Probably due to > the changes in patch 5. Please take a look. * Okay. Does it hang indefinitely or for brief moments? Thank you. --- - Prasad
