Re: [PATCH v5 04/10] hugetlb/userfaultfd: Unshare all pmds for hugetlbfs when register wp

[Mike Kravetz]

On 2/12/21 1:18 PM, Peter Xu wrote:
> On Fri, Feb 12, 2021 at 10:11:39AM -0800, Mike Kravetz wrote:
>> On 2/10/21 1:21 PM, Axel Rasmussen wrote:
>>> From: Peter Xu <peterx@xxxxxxxxxx>
>>> diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h
>>> index b8200782dede..ff50c8528113 100644
>>> --- a/include/linux/mmu_notifier.h
>>> +++ b/include/linux/mmu_notifier.h
>>> @@ -51,6 +51,7 @@ enum mmu_notifier_event {
>>>  	MMU_NOTIFY_SOFT_DIRTY,
>>>  	MMU_NOTIFY_RELEASE,
>>>  	MMU_NOTIFY_MIGRATE,
>>> +	MMU_NOTIFY_HUGETLB_UNSHARE,
>>
>> I don't claim to know much about mmu notifiers.  Currently, we use other
>> event notifiers such as MMU_NOTIFY_CLEAR.  I guess we do 'clear' page table
>> entries if we unshare.  More than happy to have a MMU_NOTIFY_HUGETLB_UNSHARE
>> event, but will consumers of the notifications know what this new event type
>> means?  And, if we introduce this should we use this other places where
>> huge_pmd_unshare is called?
> 
> Yeah AFAICT that is a new feature to mmu notifiers and it's not really used a
> lot by consumers yet.  Hmm... is there really any consumer at all? I simply
> grepped MMU_NOTIFY_UNMAP and see no hook took special care of that.  So it's
> some extra information that the upper layer would like to deliever to the
> notifiers, it's just not vastly used so far.
> 
> So far I didn't worry too much on that either.  MMU_NOTIFY_HUGETLB_UNSHARE is
> introduced here simply because I tried to make it explicit, then it's easy to
> be overwritten one day if we think huge pmd unshare is not worth a standalone
> flag but reuse some other common one.  But I think at least I owe one
> documentation of that new enum. :)
> 
> I'm not extremely willing to touch the rest callers of huge pmd unshare yet,
> unless I've a solid reason.  E.g., one day maybe one mmu notifier hook would
> start to monitor some events, then that's a better place imho to change them.
> Otherwise any of my future change could be vague, imho.
> 
> For this patch - how about I simply go back to use MMU_NOTIFIER_CLEAR instead?

I'm good with the new MMU_NOTIFY_HUGETLB_UNSHARE and agree with your reasoning
for adding it.  I really did not know enough about usage which caused me to
question.
-- 
Mike Kravetz