Re: + mm-introduce-reported-pages.patch added to -mm tree

[David Hildenbrand <david@redhat.com>]

>> "for your own idea" - are you saying Nitesh's approach is my idea? I
>> hope not, otherwise I would get credit for Rik's and Nitesh's work by
>> simply providing review comments.
> 
> Sorry, I was using "your" in the collective sense. I meant Nitesh, Rik,
> MST, yourself, and any other folks are working on the bitmap approach.

I guess I was misreading it then :)

> 
>> Of course it is okay to fight for your own idea.
>>
>>> let me reply to the criticisms of my own patchset before you pile on. I
>>
>> Me (+ Michal): Are these core buddy changes really wanted and required.
>> Can we evaluate the alternatives properly. (Michal even proposed
>> something very similar to Nitesh's approach before even looking into it)
> 
> That is part of my frustration. Before I even had a chance to explain the
> situation you had already jumped in throwing a "I second that" into the
> discussion and insisting on comparisons against Nitesh's patch set which I
> have already provided.

The different prototypes Nitesh provided are the only alternatives we 
have. So, to do the discussion that I want to see for a long time, we 
should evaluate them?

> 
> Nitesh's most recent patch set caused a kernel panic, and when I fixed
> that then it is over 30% worse performance wise than my patch set, and
> then when Nitesh restricted the order to MAX_ORDER - 1 and added some
> logic to check the buddy before taking the lock then it finally became
> comparable.

Yes, they are protoypes, RFCs. You did the right thing to report the 
issues so Nitesh can look into them.

> 
>> You: Please take my patch set, it is better than the alternatives
>> because of X, for X in {RFC quality, sparse zones, locking internals,
>> current performance differences}
> 
> I should have replied to Michal's original question and simply stated that
> Mel had not replied to the patches in the last month and a half. I half
> suspect that is the reason for Andrew applying it. It put some pressure on
> others to provide review feedback, which if nothing else I am grateful
> for.
> 
> You had inserted the need to compare it against Nitesh's patch set. Which
> based on Nitesh's email is likely going to be a little while since he
> cannot give me an ETA.

So I want us (you, me, Michal, Mel, Dave, ...) to discuss the direction 
we want to go. I'd love to do this on a design level, instead of having 
to wait for any patch set. But I guess this is harder to do? And 
especially as you keep mentioning the performance difference, I think we 
should evaluate if this is an unsolvable problem or just an issue in the 
current prototype?

I mean people could have a look at Niteshs older series to see how it 
fundamentally differs to your approach (external tracking). Nitesh might 
have fixed some things in the mean time, and is replacing the "fake 
allocation" by page isolation. But I mean, the general approach should 
be obvious and sufficient for people to make a decision.

> 
>> And all I am requesting is that we do the evaluation, discuss if there
>> are really no alternatives, and sort out fundamental issues with
>> external tracking.
>>
>> Michal asked the very same question again at the beginning of this
>> thread: "Is there really a consensus"
> 
> Yes, but he also said he is not nacking the patch. It seemed like he is

I never NACKed your series either.

> deferring to Mel on this so I will try to work with Mel to address his
> concerns since he had some feedback that I can act on.

Makes perfect sense to me.

> 
> I'll address the comments Mel provided and be submitting a v14 sometime
> soon.
> 
>> Reading the replies, "no".
> 
> I get that you think the bitmap approach is the best approach, but the

No, I think that a simple external tracking is preferable over the core 
buddy modifications we have right now. Kernel panics of fixable 
performance regressions in an RFC are not fundamental issues to me.

> fact is it is still invasive, just to different parts of the mm subsystem.

I'd love to see how it uses the page isolation framework, and only has a 
single hook to queue pages. I don't like the way pages are pulled out of 
the buddy in Niteshs approach currently. What you have is cleaner.

> I would argue that one of my concerns about the hotplug and sparse
> handling is that by skipping those for now is essentially hiding what is
> likely to be some invasive code, likely not too different from what I had
> to deal with with compaction. At this point he adds more data to the zone
> struct than my changes, and I suspect as he progresses that may increase
> further. >
> I do not think it is fair to hold up review and acceptance of this patch
> set for performance comparisons with a patch set with no definite ETA.

Michal asked "Is there really a consensus". A consensus that we want 
something like this, not that we want Nitesh's approach. It's just an 
alternative worth discussing.

And if you are reworking your patch set now with Mel, we might get 
another alternative that everybody is pleased with. Nobody is against 
reviewing your series - that's perfect, it's against picking it up and 
sending it upstream. That's my concern an Michals concern if I am not wrong.

> Ideally we should be able to review and provide feedback on this patch set
> on its own. Since Nitesh's code is in part based on my virio-balloon code

I definitely agree, but this here is *not* a reply to your patch set but 
to "Re: + mm-introduce-reported-pages.patch added to -mm tree" - picking 
it up for testing. Michals question is completely valid. And I think the 
discussion started by Michal and me here is completely valid.

> anyway it would make more sense to replace my code eventually if/when he
> comes up with a better solution. One thing I can do is make sure that my
> code is as non-intrusive as possible so that if/when that time comes
> reverting it would not be too difficult.

-- 

Thanks,

David / dhildenb