On 2019-11-15 at 21:25 Michal Hocko wrote: >On Fri 15-11-19 21:09:55, lixinhai.lxh@xxxxxxxxx wrote: >> On 2019-11-15 at 20:50 David Hildenbrand wrote: >> >On 15.11.19 06:59, lixinhai.lxh@xxxxxxxxx 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@xxxxxxxxxx/ >> >>> >> >>> Reported-by: Ralph Campbell <rcampbell@xxxxxxxxxx> >> >>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> >> >>> Cc: Michal Hocko <mhocko@xxxxxxxx> >> >>> Signed-off-by: Li Xinhai <lixinhai.lxh@xxxxxxxxx> >> >>> --- >> >>> 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
