Prasad Pandit <ppan...@redhat.com> writes: > Hi, > > On Tue, 4 Mar 2025 at 20:12, Peter Xu <pet...@redhat.com> wrote: >> IIUC Fabiano is not asking you to drop them, but split them. Split still >> "requires" them to be present, as long as before the enablement patch. > > * Yes, same here; Even I am not suggesting to drop anything. Fabiano > mentioned the following about the changes being done: > > 1. _in preparation for_ the feature (multifd+postcopy enablement) > 2. _as part of_ the feature (multifd+postcopy enablement)
The "channel discovery" part can be considered as a separate patch because in order to integrate it with the new feature you had to introduce a new concept of enum CH_* which is not a trivially verifiable change. That change has the potential to break other use-cases of migration and although the tests are passing we must be cautions about it. Simply having that part as a patch is enough separation that an issue there won't be conflated with the multifd+postcopy feature in general. I also called it "in preparation" for the feature because the channel discovery part is clearly a preexisting issue, in the sense that had we had proper magic numbers in all channels or some other mechanism to diffrentiate channels, we wouldn't need this patch. That's a cleanup you're doing beforehand in order to make the subsequent patches simpler. Changing that code makes sense regardless of whether there is a new feature being introduced. Note that none of this is out of the ordinary, you'll find such discussions in any thread on this community. It may feel arbitrary to you because that's tacit knowledge we gathered along the years. > 3. Concerns about git bisect and backporting of patches, not > missing patches when backporting. Yes, whenever you can frame a code change as a pure refactoring, you should do that. If, say, you had to change 100 lines in order to implement your feature, but 90 of those lines are just preserving the existing behavior, that is a good candidate to be a separate patch. Reviewers will be more inclined to approve the patch, since reading code and making sure it doesn't change behavior is easier than also accounting for the new behavior simultaneously. Maintainers moving patches around, merging, testing, etc will have an easier time when solving git conflicts because the separate code is more easily compared with the old one. Backporters have similar issues with git, but also face questions about the safety of bringing a patch as a prerequisite into older QEMU versions. Imagine someone wants to enable multifd+postcopy in QEMU 9.2, it'll be easier for them to have code more clearly defined as patches. Another scenario could be if the changes to the channel discovery actually fix a bug that we're currently not aware of. That would show up in a bisect session and we wouldn't want to apply a patch that fixes a bug if the same patch also enables a feature. > > * I am thinking: > 1. The _required_ changes are the _as part of_ the feature changes. > 2. The refactoring of ram_save_target_page and moving of > MULTIFD_MAGIC/VERSION macros to multifd.h patch can be termed as _in > preparation for_ the feature. Of these > - Refactoring of 'ram_save_target_page' function patch is > already pulled upstream. > -> > https://gitlab.com/qemu-project/qemu/-/commit/bc38dc2f5f350310724fd7d4f0a09f8c3a4811fa > - Similarly moving of MULTIFD_ macros patch can be pulled. It > has not changed for many revisions. > 3. The _essential_ changes are further refinement of the feature > (multifd+postcopy enablement). > > * IMO, we can split patches as: > - One patch for -> _required_ OR _as part of_ the feature changes > - One patch for -> MULTIFD_ macros as _in preparation for_ the > feature change > - One patch for -> _essential_ changes : flush and sync related > - Two patches for qtest cases related changes. > This division also seems helpful for git bisect and backporting > related concerns. > > * Currently we are divided over what constitutes: _required_ OR _as > part of_ the feature changes. > - Hence the request for individual opinions towards that. > > * If we want to merge _required_/_as part of_ the feature and > _essential_ changes together in one patch - I'm okay. > * If we want to split the _required_ / _as part of_ the feature patch > further into two, we need to define exactly how we do that division. - > [???] > We need an extra patch that reads: migration: Refactor channel discovery mechanism The various logical migration channels don't have a standardized way of advertising themselves and their connections may be seen out of order by the migration destination. When a new connection arrives, the incoming migration currently make use of heuristics to determine which channel it belongs to. The next few patches will need to change how the multifd and postcopy capabilities interact and that affects the channel discovery heuristic. Refactor the channel discovery heuristic to make it less opaque and simplify the subsequent patches. <some description of the new code which might be pertinent> --- You'd move all of the channel discovery code into this patch. Some of it will be unreacheable because multifd is not yet allowed with postcopy, but that's fine. You can mention it on the commit message. About moving the code out of migration.c, it was a suggestion that you're free to push back. Ideally, doing the work would be faster than arguing against it on the mailing list. But that's fine. About the hang in the test. It doesn't reproduce often, but once it does, it hangs forever (although I haven't waited that long). > * I have shared my thoughts above, I won't hold on to it unduly. We > need to find a way to move forward. I'm willing to go with either way > we decide. > > Thank you. > --- > - Prasad
