Subject: Re: [PATCH V2] mm/mprotect: Add a cond_resched() inside
 change_pmd_range()
To: Michal Hocko <mhocko@kernel.org>,
        Anshuman Khandual <khandual@linux.vnet.ibm.com>
References: <20171214111426.25912-1-khandual@linux.vnet.ibm.com>
 <20171214112928.GH16951@dhcp22.suse.cz>
 <28e54a80-73d9-76aa-31d5-f71375f14b96@linux.vnet.ibm.com>
 <20171214130435.GL16951@dhcp22.suse.cz>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org
From: Anshuman Khandual <khandual@linux.vnet.ibm.com>
Date: Thu, 14 Dec 2017 18:50:41 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.5.1
MIME-Version: 1.0
In-Reply-To: <20171214130435.GL16951@dhcp22.suse.cz>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Message-Id: <cc03168b-dd53-73e7-88fd-717eba6e6ce0@linux.vnet.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1535
Lines: 35

On 12/14/2017 06:34 PM, Michal Hocko wrote:
> On Thu 14-12-17 18:25:54, Anshuman Khandual wrote:
>> On 12/14/2017 04:59 PM, Michal Hocko wrote:
>>> On Thu 14-12-17 16:44:26, Anshuman Khandual wrote:
>>>> diff --git a/mm/mprotect.c b/mm/mprotect.c
>>>> index ec39f73..43c29fa 100644
>>>> --- a/mm/mprotect.c
>>>> +++ b/mm/mprotect.c
>>>> @@ -196,6 +196,7 @@ static inline unsigned long change_pmd_range(struct vm_area_struct *vma,
>>>>  		this_pages = change_pte_range(vma, pmd, addr, next, newprot,
>>>>  				 dirty_accountable, prot_numa);
>>>>  		pages += this_pages;
>>>> +		cond_resched();
>>>>  	} while (pmd++, addr = next, addr != end);
>>>>  
>>>>  	if (mni_start)
>>> this is not exactly what I meant. See how change_huge_pmd does continue.
>>> That's why I mentioned zap_pmd_range which does goto next...
>> I might be still missing something but is this what you meant ?
> yes, except
> 
>> Here we will give cond_resched() cover to the THP backed pages
>> as well.
> but there is still 
> 		if (!is_swap_pmd(*pmd) && !pmd_trans_huge(*pmd) && !pmd_devmap(*pmd)
> 				&& pmd_none_or_clear_bad(pmd))
> 			continue;
> 
> so we won't have scheduling point on pmd holes. Maybe this doesn't
> matter, I haven't checked but why should we handle those differently?

May be because it is not spending much time for those entries which
can really trigger stalls, hence they dont need scheduling points.
In case of zap_pmd_range(), it was spending time either in
__split_huge_pmd() or zap_huge_pmd() hence deserved a scheduling point.