On Wed, Jun 21, 2017 at 12:05:07AM -0700, Hugh Dickins wrote: > On Mon, 19 Jun 2017, Willy Tarreau wrote: > > > From: Hugh Dickins <hughd@xxxxxxxxxx> > > > > commit 1be7107fbe18eed3e319a6c3e83c78254b693acb upstream. > > Some of these suggested adjustments below are just what comparing mine > and yours showed up, and I'm being anal in passing them on e.g. I do > like your blank line in mm.h, but Michal chose to leave it out, and > I think that the closer we keep these sources to each other, > the less trouble we shall have patching on top in future. I totally agree, that's what I generally focus on as well. > Which is particularly true in expand_upwards() and expand_downwards() > (and you're thinking of backporting Helge's TASK_SIZE enhancement > on top of that, though I don't think it's strictly necessary for a > stable tree). I thought it was a fix for a corner case on PARISC, so just in case I'd rather stick as close as possible to mainline : at least we want to ensure the same bugs are met everywhere so that we can benefit from developers' help when issues are met. > Your patch is not wrong there: though odd to be trying > anon_vma_prepare() twice in expand_downwards(), Ah crap, the second one is a leftover from initial code that I missed. > and tiresome to have to unlock at each error exit. Oh I'm seeing that you could move it later, I wasn't sure about this one. Thanks. I think I did the same stuff in the 3.16 backport. > But I'd already decided in one of our > internal trees just to factor in some of Konstantin's change, that > made the ordering much more sensible there, and the two more like > each other; so recommend that 3.10 do the same, keeping it closer > to the final 4.12 code. But you may have different priorities and > disagree with that: just suggesting. No, I perfectly agree with you. As I mentionned, my patches were proposals based on what I understood from the code, I'm really glad to receive your help and fixes here! > And there is the possibility that we shall want another patch or > two on top there. I've left a question as to whether we should be > comparing anon_vmas. And there's a potential (but I think ignorable) > locking issue, in the case of an architecture that supports both > VM_GROWSUP and VM_GROWSDOWN: if they expand towards each other at the > same instant, they could gobble up the gap between them (they almost > certainly have different anon_vmas, so the anon_vma locking does not > protect against that). When it gets to updating the vma tree, it is > careful to use page_table_lock to maintain the consistency of the > tree in such a case, but maybe we should do that earlier. OK. > Then there's the FOLL_MLOCK thing, and the WARN_ON (phew, remembered > in time that you don't have VM_WARN_ON) - but keep in mind that I > have not even built this tree, let alone tested it. I'll take care of building it, don't worry. > Sorry if I'm being annoying, Willy: you must be heartily sick of > these patches by now! Or, being a longtime longterm maintainer, > perhaps it's all joy for you ;-? No, rest assured it's never a full joy :-) But it's much better when I get help from the people who know how this stuff works than when I have to invent the backport by myself! Thanks a lot, I'll include your patch and will test it again. And yes, I intend to merge Helge's fix once it lands into mainline (maybe it is right now, I didn't check) and possibly other ones you might be working on depending on various feedback. Willy