[Devel] [PATCH rh7] x86: arch_free_page() introduced

[Konstantin Khorenko]

On 11/17/2015 06:31 PM, Stanislav Kinsburskiy wrote:
>
>
> 17.11.2015 16:08, Vladimir Davydov пишет:
>> On Tue, Nov 17, 2015 at 03:42:46PM +0100, Stanislav Kinsburskiy wrote:
>>>
>>> 17.11.2015 15:20, Vladimir Davydov пишет:
>>>> On Thu, Nov 12, 2015 at 10:01:14PM +0400, Stanislav Kinsburskiy wrote:
>>>>
>>>>> @@ -0,0 +1,23 @@
>>>>> +#ifndef _ASM_X86_FREE_PAGE_H
>>>>> +#define _ASM_X86_FREE_PAGE_H
>>>>> +
>>>>> +#ifdef __KERNEL__
>>>>> +
>>>>> +#ifndef __ASSEMBLY__
>>>>> +
>>>>> +#include <linux/jump_label.h>
>>>>> +
>>>>> +#define HAVE_ARCH_FREE_PAGE
>>>>> +
>>>>> +extern struct static_key zero_free_pages;
>>>>> +extern void do_zero_pages(struct page *page, int order);
>>>>> +
>>>>> +static __always_inline void arch_free_page(struct page *page, int order)
>>>>> +{
>>>>> +	if (static_key_false(&zero_free_pages))
>>>>> +		do_zero_pages(page, order);
>>>>> +}
>>>> There is no point in making this feature arch-dependant now, as you use
>>>> jump labels, which are arch-independent. The only reason why I had to
>>>> overwrite arch_free_page in PCS6 is that I had to write some asm code to
>>>> implement functionality close to that provided by jump labels, which are
>>>> absent in RH6-based kernels. Please move it to mm/page_alloc.c.
>>>>
>>>> Thanks.
>>> If we will make this function generic, then we have to declare
>>> "zero_free_pages" as generic (which is more or less ok)  and define
>>> do_zero_pages for the case, when HAVE_ARCH_FREE_PAGE is not defined. Does
>>> the goal worth it?
>>> If yes, then why?
>>>
>> I don't think I get your point. All I want you to do is something like
>> this:
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index f70c5f4da2a2..12126f212f3b 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -730,8 +730,11 @@ static bool free_pages_prepare(struct page *page, unsigned int order)
>>
>>    	if (PageAnon(page))
>>    		page->mapping = NULL;
>> -	for (i = 0; i < (1 << order); i++)
>> +	for (i = 0; i < (1 << order); i++) {
>>    		bad += free_pages_check(page + i);
>> +		if (static_key_false(&zero_free_pages))
>> +			clear_highpage(page + i);
>> +	}
>>    	if (bad)
>>    		return false;
>>
>> IMO this is better than messing around arch code.
>
> Ok, I got your point.
> I'm trying to follow a little bit different strategy, which is aimed to
> impact generic code as less, as possible. And from this point of view
> using already defined arch_free_page callback looks better to me. But I
> don't insist and can make it your way.
> Should I?

Stas, i like the idea not touching generic code if possible,
but don't like the idea of using arch_free_page() for zeroing purposes, because
* it's really arch independent
* does not suit the logic of name (arch_free_page() <-> zero pages)
* it can be defined one day for x86 as well and is already defined for
   powerpc which we might want to support one day.

=> please rework the patch. If you wish to leave your idea of heaving in different files - i'm ok,
but create them in arch independent dirs and call another (new) function near arch_free_page().

Or go with Vladimir's solution - if you got persuaded. :)

Thank you.