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

[David Hildenbrand <david@redhat.com>]

On 06.11.19 18:48, Alexander Duyck wrote:
> On Wed, 2019-11-06 at 17:54 +0100, Michal Hocko wrote:
>> On Wed 06-11-19 08:35:43, Alexander Duyck wrote:
>>> On Wed, 2019-11-06 at 15:09 +0100, David Hildenbrand wrote:
>>>>> Am 06.11.2019 um 13:16 schrieb Michal Hocko <mhocko@kernel.org>:
>>>>>
>>>>> I didn't have time to read through newer versions of this patch series
>>>>> but I remember there were concerns about this functionality being pulled
>>>>> into the page allocator previously both by me and Mel [1][2]. Have those been
>>>>> addressed? I do not see an ack from Mel or any other MM people. Is there
>>>>> really a consensus that we want something like that living in the
>>>>> allocator?
>>>>
>>>> I don‘t think there is. The discussion is still ongoing (although quiet,
>>>> Nitesh is working on a new version AFAIK). I think we should not rush
>>>> this.
>>>
>>> How much time is needed to get a review? I waited 2 weeks since posting
>>> v12 and the only comments I got on the code were from Andrew. Most of this
>>> hasn't changed much since v10 and that was posted back in mid September. I
>>> have been down to making small tweaks here and there and haven't had any
>>> real critiques on the approach since Mel had the comments about conflicts
>>> with compaction which I addressed by allowing compaction to punt the
>>> reporter out so that it could split and splice the lists as it walked
>>> through them.
>>
>> Well, people are busy and MM community is not a large one. I cannot
>> really help you much other than keep poking those people and give
>> reasonable arguments so they decide to ack your patch.
> 
> I get that. But v10 was posted in mid September. Back then we had a
> discussion about addressing what Mel had mentioned and I had mentioned
> then that I had addressed it by allowing compaction to essentially reset
> the reporter to get it out of the list so compaction could do this split
> and splice tumbling logic.
> 
>> I definitely do not intent to nack this work, I just have maintainability
>> concerns and considering there is an alternative approach that does not
>> require to touch page allocator internals and which we need to compare
>> against then I do not really think there is any need to push something
>> in right away. Or is there any pressing reason to have this merged right
>> now?
> 
> The alternative approach doesn't touch the page allocator, however it
> still has essentially the same changes to __free_one_page. I suspect the

Nitesh is working on Michals suggestion to use page isolation instead 
AFAIK - which avoids this.

> performance issue seen is mostly due to the fact that because it doesn't
> touch the page allocator it is taking the zone lock and probing the page
> for each set bit to see if the page is still free. As such the performance
> regression seen gets worse the lower the order used for reporting.
> 
> Also I suspect Nitesh's patches are also in need of further review. I have
> provided feedback however my focus ended up being on more the kernel
> panics and 30% performance regression rather than debating architecture.

Please don't take this personally, but I really dislike you taking about 
Niteshs RFCs (!) and pushing for your approach (although it was you that 
was late to the party!) in that way. If there are problems then please 
collaborate and fix instead of using the same wrong arguments over and 
over again.

a) hotplug/sparse zones: I explained a couple of times why we can ignore 
that. There was never a reply from you, yet you keep coming up with 
that. I don't enjoy talking to a wall.

b) Locking optimizations: Come on, these are premature optimizations and 
nothing to dictate your design. *nobody* but you cares about that in an 
initial approach we get upstream. We can always optimize that.

c) Kernel panics: Come on, we are talking about complicated RFCs here 
with moving design decisions. We want do discuss *design* and 
*architecture* here, not *implementation details*.

d) Performance: We want to see a design that fits into the whole 
architecture cleanly, is maintainable, and provides a benefit. Of 
course, performance is relevant, but it certainly should not dictate our 
design of a *virtualization specific optimization feature*. Performance 
is not everything, otherwise please feel free and rewrite the kernel in 
ASM and claim it is better because it is faster.

Again, I do value your review and feedback, but I absolutely do not 
enjoy the way you are trying to push your series here, sorry.

Yes, if we end up finding out that there is real value in your approach, 
nothing speaks against considering it. But please don't try to hurry and 
push your series in that way. Please give everybody to time to evaluate.

-- 

Thanks,

David / dhildenb