Re: [PATCH] mm, trivial: Simplify swap related code in try_to_unmap_one()

[Huang\, Ying]

Michal Hocko <mhocko@xxxxxxxxxx> writes:

> On Tue 31-03-20 16:46:13, Huang, Ying wrote:
>> From: Huang Ying <ying.huang@xxxxxxxxx>
>> 
>> Because PageSwapCache() will always return false if PageSwapBacked() returns
>> false, and PageSwapBacked() will be check for MADV_FREE pages in
>> try_to_unmap_one().  The swap related code in try_to_unmap_one() can be
>> simplified to improve the readability.
>
> My understanding is that this is a sanity check to let us know if
> something breaks. Do we really want to get rid of it? Maybe it is not
> really useful but if that is the case then the changelog should reflect
> this fact.

Now the definition of PageSwapCache() is,

static __always_inline int PageSwapCache(struct page *page)
{
#ifdef CONFIG_THP_SWAP
	page = compound_head(page);
#endif
	return PageSwapBacked(page) && test_bit(PG_swapcache, &page->flags);
}

So, if PageSwapBacked() returns false, PageSwapCache() will always
return false.  The original checking,

-			if (unlikely(PageSwapBacked(page) != PageSwapCache(page))) {

is equivalent to

-			if (unlikely(PageSwapBacked(page) && !PageSwapCache(page))) {

Then what is the check !PageSwapBacked() && PageSwapCache() for?  To
prevent someone to change the definition of PageSwapCache() in the
future to break this?

Best Regards,
Huang, Ying