On Fri, Jan 08, 2021 at 11:34:08AM -0800, Linus Torvalds wrote: > On Fri, Jan 8, 2021 at 9:15 AM Will Deacon <will@xxxxxxxxxx> wrote: > > > > The big difference in this version is that I have reworked it based on > > Kirill's patch which he posted as a follow-up to the original. However, > > I can't tell where we've landed on that -- Linus seemed to like it, but > > Hugh was less enthusiastic. > > Yeah, I like it, but I have to admit that it had a disturbingly high > number of small details wrong for several versions. I hope you picked > up the final version of the code. I picked the version from here: https://lore.kernel.org/r/20201229132819.najtavneutnf7ajp@box and actually, I just noticed that willy spotted a typo in a comment, so I'll fix that locally as well as adding the above to a 'Link:' tag for reference. > At the same time, I do think that the "disturbingly high number of > issues" was primarily exactly _because_ the old code was so > incomprehensible, and I think the end result is much cleaner, so I > still like it. > > >I think that my subsequent patches are an > > awful lot cleaner after the rework > > Yeah, I think that's a side effect of "now the code really makes a lot > more sense". Your subsequent patches 2-3 certainly are much simpler > now, although I'd be inclined to add an argument to "do_set_pte()" > that has the "write" and "pretault" bits in it, instead of having to > modify the 'vmf' structure. I played with a few different ways of doing this, but I can't say I prefer them over what I ended up posting. Having a bunch of 'bool' arguments makes the callers hard to read and brings into question what exactly vmf->flags is for. I also tried adding a separate 'address' parameter so that vmf->address is always the real faulting address, but 'address' is the thing to use for the pte (i.e. prefault is when 'address != vmf->address'). That wasn't too bad, but it made the normal finish_fault() case look weird. So I think I'll leave it as-is and see if anybody wants to change it later on. Will
