On Tue, Apr 27, 2021 at 1:42 PM Peter Xu peterx@redhat.com wrote:
On Tue, Apr 27, 2021 at 01:29:14PM -0700, Axel Rasmussen wrote:
On Tue, Apr 27, 2021 at 11:03 AM Peter Xu peterx@redhat.com wrote:
On Tue, Apr 27, 2021 at 09:57:16AM -0700, Axel Rasmussen wrote:
I'd prefer to keep them separate, as they are not tiny patches (they are roughly +200/-150 each). And, they really are quite independent - at least in the sense that I can reorder them via rebase with no conflicts, and the code builds at each commit in either orientation. I think this implies they're easier to review separately, rather than squashed.
I don't have a strong feeling about the order. I slightly prefer swapping them compared to this v4 series: first introduce minor faults, then introduce CONTINUE.
Since Peter also has no strong opinion, and Hugh it sounds like you prefer it the other way around, I'll swap them as we had in some previous version of this series: first introduce minor faults, then introduce CONTINUE.
Yes I have no strong opinion, but that's probably the least I prefer. :-)
Because you'll declare UFFD_FEATURE_MINOR_SHMEM and enable this feature without the feature being completely implemented (without UFFDIO_CONTINUE, it's not complete since no one will be able to resolve that minor fault).
Not a big deal anyway, but since we're at it... Basically I think three things to do for minor shmem support:
(1) UFFDIO_CONTINUE (resolving path) (2) Handle fault path for shmem minor fault (faulting path) (3) Enablement of UFFD_FEATURE_MINOR_SHMEM (from which point, user can detect and enable it)
I have no preference on how you'd like to merge these steps (right now you did 1 first, then 2+3 later; or as Hugh suggested do 1+2+3 together), but I'd still hope item 3 should always be the last, if possible...
In that case, I'll split the patch which adds the faulting path in two: add the faulting path hook and registration mode, and then in a separate commit advertise the feature flag as available.
Then I'll order them like so, which I think is the order Hugh finds more natural:
- MInor fault registration / faulting path
- CONTINUE ioctl to resolve the faults
- Advertise the feature as supported
Sound okay?
Good to me, thanks Axel.
Okay.