Re: [PATCH v4 06/11] mm: thp: check pmd migration entry in common path

[Zi Yan]

Kirill A. Shutemov wrote:
> On Fri, Mar 24, 2017 at 11:09:25AM -0500, Zi Yan wrote:
>> Kirill A. Shutemov wrote:
>>> On Mon, Mar 13, 2017 at 11:45:02AM -0400, Zi Yan wrote:
>>> Again. That's doesn't look right..
>> It will be changed:
>>
>>  	ptl = pmd_lock(mm, pmd);
>> +retry_locked:
>> +	if (unlikely(!pmd_present(*pmd))) {
>> +		if (likely(!(flags & FOLL_MIGRATION))) {
>> +			spin_unlock(ptl);
>> +			return no_page_table(vma, flags);
>> +		}
>> +		pmd_migration_entry_wait(mm, pmd);
>> +		goto retry_locked;
> 
> Nope. pmd_migration_entry_wait() unlocks the ptl.

Right. This chunk is wrong. pmd_migrtion_entry_wait() actually locks
pmd, then unlocks it and waits on the page if it is suitable.

An simple fix could be:

+retry_locked:
 	ptl = pmd_lock(mm, pmd);
+	if (unlikely(!pmd_present(*pmd))) {
+	        spin_unlock(ptl);
+		if (likely(!(flags & FOLL_MIGRATION)))
+			return no_page_table(vma, flags);
+		pmd_migration_entry_wait(mm, pmd);
+		goto retry_locked;
+       }

Or is it better to change pmd_migration_entry_wait() to
void pmd_migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
spinlock_t *ptl)? So that if ptl is NULL, then it takes the pmd lock and
unlocks it; if ptl is specified, it only unlocks it. This can avoid the
redundant unlock and lock in the code above, when
pmd_migration_entry_wait() is called.

Thanks.

--
Best Regards,
Yan Zi