Re: [PATCH -mm -v4 04/21] mm, THP, swap: Support PMD swap mapping in swapcache_free_cluster()

From: "Huang\, Ying" <ying.huang@intel.com>
To: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, <linux-mm@kvack.org>,
	<linux-kernel@vger.kernel.org>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Michal Hocko <mhocko@suse.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	Shaohua Li <shli@kernel.org>, Hugh Dickins <hughd@google.com>,
	Minchan Kim <minchan@kernel.org>, Rik van Riel <riel@redhat.com>,
	Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>,
	Zi Yan <zi.yan@cs.rutgers.edu>,
	Daniel Jordan <daniel.m.jordan@oracle.com>
Subject: Re: [PATCH -mm -v4 04/21] mm, THP, swap: Support PMD swap mapping in swapcache_free_cluster()
Date: Wed, 11 Jul 2018 09:08:44 +0800	[thread overview]
Message-ID: <87lgaih943.fsf@yhuang-dev.intel.com> (raw)
In-Reply-To: <444d0718-8b89-5ef1-15c8-1bbbc6cb1bf3@linux.intel.com> (Dave Hansen's message of "Tue, 10 Jul 2018 06:54:21 -0700")

Dave Hansen <dave.hansen@linux.intel.com> writes:

> On 07/09/2018 11:53 PM, Huang, Ying wrote:
>> Dave Hansen <dave.hansen@linux.intel.com> writes:
>>>> +#ifdef CONFIG_THP_SWAP
>>>> +static inline int cluster_swapcount(struct swap_cluster_info *ci)
>>>> +{
>>>> +	if (!ci || !cluster_is_huge(ci))
>>>> +		return 0;
>>>> +
>>>> +	return cluster_count(ci) - SWAPFILE_CLUSTER;
>>>> +}
>>>> +#else
>>>> +#define cluster_swapcount(ci)			0
>>>> +#endif
>>>
>>> Dumb questions, round 2:  On a CONFIG_THP_SWAP=n build, presumably,
>>> cluster_is_huge()=0 always, so cluster_swapout() always returns 0.  Right?
>>>
>>> So, why the #ifdef?
>> 
>> #ifdef here is to reduce the code size for !CONFIG_THP_SWAP.
>
> I'd just remove the !CONFIG_THP_SWAP version entirely.

Sure.  Unless there are some build errors after some other refactoring.

>>>> @@ -1288,24 +1301,30 @@ static void swapcache_free_cluster(swp_entry_t entry)
>>>>  
>>>>  	ci = lock_cluster(si, offset);
>>>>  	VM_BUG_ON(!cluster_is_huge(ci));
>>>> +	VM_BUG_ON(!is_cluster_offset(offset));
>>>> +	VM_BUG_ON(cluster_count(ci) < SWAPFILE_CLUSTER);
>>>>  	map = si->swap_map + offset;
>>>> -	for (i = 0; i < SWAPFILE_CLUSTER; i++) {
>>>> -		val = map[i];
>>>> -		VM_BUG_ON(!(val & SWAP_HAS_CACHE));
>>>> -		if (val == SWAP_HAS_CACHE)
>>>> -			free_entries++;
>>>> +	if (!cluster_swapcount(ci)) {
>>>> +		for (i = 0; i < SWAPFILE_CLUSTER; i++) {
>>>> +			val = map[i];
>>>> +			VM_BUG_ON(!(val & SWAP_HAS_CACHE));
>>>> +			if (val == SWAP_HAS_CACHE)
>>>> +				free_entries++;
>>>> +		}
>>>> +		if (free_entries != SWAPFILE_CLUSTER)
>>>> +			cluster_clear_huge(ci);
>>>>  	}
>>>
>>> Also, I'll point out that cluster_swapcount() continues the horrific
>>> naming of cluster_couunt(), not saying what the count is *of*.  The
>>> return value doesn't help much:
>>>
>>> 	return cluster_count(ci) - SWAPFILE_CLUSTER;
>> 
>> We have page_swapcount() for page, swp_swapcount() for swap entry.
>> cluster_swapcount() tries to mimic them for swap cluster.  But I am not
>> good at naming in general.  What's your suggestion?
>
> I don't have a suggestion because I haven't the foggiest idea what it is
> doing. :)
>
> Is it the number of instantiated swap cache pages that are referring to
> this cluster?  Is it just huge pages?  Huge and small?  One refcount per
> huge page, or 512?

page_swapcount() and swp_swapcount() for a normal swap entry is the
number of PTE swap mapping to the normal swap entry.

cluster_swapcount() for a huge swap entry (or huge swap cluster) is the
number of PMD swap mapping to the huge swap entry.

Originally, cluster_count is the reference count of the swap entries in
the swap cluster (that is, how many entries are in use).  Now, it is the
sum of the reference count of the swap entries in the swap cluster and
the number of PMD swap mapping to the huge swap entry.

I need to add comments for this at least.

Best Regards,
Huang, Ying