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

[Stanislav Kinsburskiy]

18.11.2015 14:46, 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.
Ok, sure.