On Mon, Jan 21, 2019 at 08:54:46AM -0500, Jerome Glisse wrote: > On Mon, Jan 21, 2019 at 03:57:11PM +0800, Peter Xu wrote: > > change_protection() was used by either the NUMA or mprotect() code, > > there's one parameter for each of the callers (dirty_accountable and > > prot_numa). Further, these parameters are passed along the calls: > > > > - change_protection_range() > > - change_p4d_range() > > - change_pud_range() > > - change_pmd_range() > > - ... > > > > Now we introduce a flag for change_protect() and all these helpers to > > replace these parameters. Then we can avoid passing multiple parameters > > multiple times along the way. > > > > More importantly, it'll greatly simplify the work if we want to > > introduce any new parameters to change_protection(). In the follow up > > patches, a new parameter for userfaultfd write protection will be > > introduced. > > > > No functional change at all. > > There is one change i could spot and also something that looks wrong. > > > > > Signed-off-by: Peter Xu <peterx@xxxxxxxxxx> > > --- > > [...] > > > @@ -428,8 +431,7 @@ mprotect_fixup(struct vm_area_struct *vma, struct vm_area_struct **pprev, > > dirty_accountable = vma_wants_writenotify(vma, vma->vm_page_prot); > > vma_set_page_prot(vma); > > > > - change_protection(vma, start, end, vma->vm_page_prot, > > - dirty_accountable, 0); > > + change_protection(vma, start, end, vma->vm_page_prot, MM_CP_DIRTY_ACCT); > > Here you unconditionaly see the DIRTY_ACCT flag instead it should be > something like: > > s/dirty_accountable/cp_flags > if (vma_wants_writenotify(vma, vma->vm_page_prot)) > cp_flags = MM_CP_DIRTY_ACCT; > else > cp_flags = 0; > > change_protection(vma, start, end, vma->vm_page_prot, cp_flags); > > Or any equivalent construct. Oops, thanks for spotting this... it was definitely wrong. I'll fix. > > > /* > > * Private VM_LOCKED VMA becoming writable: trigger COW to avoid major > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c > > index 005291b9b62f..23d4bbd117ee 100644 > > --- a/mm/userfaultfd.c > > +++ b/mm/userfaultfd.c > > @@ -674,7 +674,7 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start, > > newprot = vm_get_page_prot(dst_vma->vm_flags); > > > > change_protection(dst_vma, start, start + len, newprot, > > - !enable_wp, 0); > > + enable_wp ? 0 : MM_CP_DIRTY_ACCT); > > We had a discussion in the past on that, i have not look at other > patches but this seems wrong to me. MM_CP_DIRTY_ACCT is an > optimization to keep a pte with write permission if it is dirty > while my understanding is that you want to set write flag for pte > unconditionaly. > > So maybe this patch that adds flag should be earlier in the serie > so that you can add a flag to do that before introducing the UFD > mwriteprotect_range() function. I agree. I'm going to move the UFFDIO_WRITEPROTECT patch to the last so I'll rearrange this part too so these lines will be removed in my next version. Thanks! -- Peter Xu
