Re: [PATCH] mm: Improve PageAnon() to check the whole PAGE_MAPPING_FLAGS

["lixinhai.lxh@gmail.com" <lixinhai.lxh@gmail.com>]

On 2019-11-15 at 21:25 Michal Hocko wrote:
>On Fri 15-11-19 21:09:55, lixinhai.lxh@gmail.com wrote:
>> On 2019-11-15 at 20:50 David Hildenbrand wrote:
>> >On 15.11.19 06:59, lixinhai.lxh@gmail.com wrote:
>> >> On 2019-11-15 at 11:18 Li Xinhai wrote:
>> >>> PageAnon() just checking on PAGE_MAPPING_ANON bit would cause page,
>> >>> with PageKsm as true, been wrongly considered as PageAnon. Now,
>> >>> checking the whole PAGE_MAPPING_FLAGS to avoid this error.
>> >>>
>> >>> Reported from:
>> >>> https://lore.kernel.org/linux-mm/20191113000651.20677-1-rcampbell@nvidia.com/
>> >>>
>> >>> Reported-by: Ralph Campbell <rcampbell@nvidia.com>
>> >>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> >>> Cc: Michal Hocko <mhocko@suse.com>
>> >>> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
>> >>> ---
>> >>> include/linux/page-flags.h | 3 ++-
>> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
>> >>> index 1bf83c8..1849fc3 100644
>> >>> --- a/include/linux/page-flags.h
>> >>> +++ b/include/linux/page-flags.h
>> >>> @@ -461,7 +461,8 @@ static __always_inline int PageMappingFlags(struct page *page)
>> >>> static __always_inline int PageAnon(struct page *page)
>> >>> {
>> >>> page = compound_head(page);
>> >>> -	return ((unsigned long)page->mapping & PAGE_MAPPING_ANON) != 0;
>> >>> +	return ((unsigned long)page->mapping & PAGE_MAPPING_FLAGS) ==
>> >>> +	PAGE_MAPPING_ANON;
>> >>> }
>> >>>
>> >>> static __always_inline int __PageMovable(struct page *page)
>> >>> --
>> >>> 1.8.3.1
>> >>>
>> >>
>> >> The current semantics of PageAnon() for both KSM and !KSM are used in many
>> >> places, so can't change it alone without change other code.
>> >> Need skip this patch.
>> >
>> >... I assume that was intended because KSM only merges anonymous pages?
>> >If it was not intended, it would scream for a cleanup.
>> >
>> Yes, I think that was intended to keep the origial version of PageAnon() when add the
>> PageKsm(). So all places where used to have PageAnon() didn't requre change, but other
>> places for new code we have some thing like (PageAnon() && !PageKsm()) check, or do
>> checking in correct sequence.
>>
>> One thing I am not quite sure is about couting of anonymous page, in case page is KSM
>> or !KSM, hope those old codes has been correctly handled. 
>
>I am not really sure I understand what do you like to achieve here. Yes
>there are different checks representing different classes of pages. In
>this particular case the ordering of the check was just wrong. That is
>trivial to be fixed. PageAnon works as intented AFAIK. If that is not
>the case then please be explicit why. 
I was thinking about make changes so to using PageAnon() for Anonymous&!KSM and 
PageKsm() for Anonymous&KSM, then don't require consider sequence. Now, I realized 
that PageAnon() is intended for cover KSM and !KSM cases and it seems no big benefits 
to change the semantics of PageAnon().

>--
>Michal Hocko
>SUSE Labs