Re: [PATCH 1/3 v6] mm(v4.1): New pfn_mkwrite same as page_mkwrite for VM_PFNMAP

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Tue, Apr 07, 2015 at 04:37:07PM +0300, Boaz Harrosh wrote:
> On 04/07/2015 04:26 PM, Kirill A. Shutemov wrote:
> > On Tue, Apr 07, 2015 at 04:17:00PM +0300, Kirill A. Shutemov wrote:
> >> On Tue, Apr 07, 2015 at 03:57:32PM +0300, Boaz Harrosh wrote:
> >>> +/*
> >>> + * Handle write page faults for VM_MIXEDMAP or VM_PFNMAP for a VM_SHARED
> >>> + * mapping
> >>> + */
> >>> +static int wp_pfn_shared(struct mm_struct *mm,
> >>> +			struct vm_area_struct *vma, unsigned long address,
> >>> +			pte_t *page_table, spinlock_t *ptl, pte_t orig_pte,
> >>> +			pmd_t *pmd)
> >>> +{
> >>> +	if (vma->vm_ops && vma->vm_ops->pfn_mkwrite) {
> >>> +		struct vm_fault vmf = {
> >>> +			.page = NULL,
> >>> +			.pgoff = linear_page_index(vma, address),
> >>> +			.virtual_address = (void __user *)(address & PAGE_MASK),
> >>> +			.flags = FAULT_FLAG_WRITE | FAULT_FLAG_MKWRITE,
> >>> +		};
> >>> +		int ret;
> >>> +
> >>> +		pte_unmap_unlock(page_table, ptl);
> >>> +		ret = vma->vm_ops->pfn_mkwrite(vma, &vmf);
> >>> +		if (ret & VM_FAULT_ERROR)
> >>> +			return ret;
> >>> +		page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
> >>> +		/* Did pfn_mkwrite already fixed up the pte */
> > 
> > Oh. I guess you've missunderstood why we need pte_same() check below.
> > It's not about ->pfn_mkwrite() changing the pte (generatlly, it should
> > not). It's requited to address race with parallel page fault to the pte.
> > 
> >>> +		if (!pte_same(*page_table, orig_pte)) {
> >>> +			pte_unmap_unlock(page_table, ptl);
> >>> +			return ret;
> >>
> >> This should be "return 0;", shouldn't it?
> >>
> >> VM_FAULT_NOPAGE would imply you've installed new pte, but you did not.
> 
> Changing this to "return 0" would be very scary for me. Because I'm running
> with this code for 1/2 a year now. And it is stable. You see since the original
> code it was always doing just that pte_unmap_unlock && return ret. (See the patch
> based on 4.0)
> 
> I did not understand if you want that I keep it "return ret".

I think "return 0;" is right way to go. It matches wp_page_shared()
behaviour.

> I gather that you would like the comment changed, about the changed pte.
> Both here and at Documentation/.../locking.
> 
> I'll send a new patch just tell me if you want the reurn thing
> 
> Thank you
> Boaz
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>