Re: [PATCH 1/4] rmap: add page_wrprotect() function,

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 11 Nov 2008 15:21:38 +0200
Izik Eidus <ieidus@redhat.com> wrote:

> From: Izik Eidus <izike@qumranet.com>
> 
> this function is useful for cases you want to compare page and know
> that its value wont change during you compare it.
> 
> this function is working by walking over the whole rmap of a page
> and mark every pte related to the page as write_protect.
> 
> the odirect_sync paramter is used to notify the caller of
> page_wrprotect() if one pte or more was not marked readonly
> in order to avoid race with odirect.

The grammar is a bit mangled.  Missing apostrophes.  Sentences start
with capital letters.

Yeah, it's a nit, but we don't actually gain anything from deliberately
mangling the language.

>
> ...
>
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -576,6 +576,103 @@ int page_mkclean(struct page *page)
>  }
>  EXPORT_SYMBOL_GPL(page_mkclean);
>  
> +static int page_wrprotect_one(struct page *page, struct vm_area_struct *vma,
> +			      int *odirect_sync)
> +{
> +	struct mm_struct *mm = vma->vm_mm;
> +	unsigned long address;
> +	pte_t *pte;
> +	spinlock_t *ptl;
> +	int ret = 0;
> +
> +	address = vma_address(page, vma);
> +	if (address == -EFAULT)
> +		goto out;
> +
> +	pte = page_check_address(page, mm, address, &ptl, 0);
> +	if (!pte)
> +		goto out;
> +
> +	if (pte_write(*pte)) {
> +		pte_t entry;
> +
> +		if (page_mapcount(page) != page_count(page)) {
> +			*odirect_sync = 0;
> +			goto out_unlock;
> +		}
> +		flush_cache_page(vma, address, pte_pfn(*pte));
> +		entry = ptep_clear_flush_notify(vma, address, pte);
> +		entry = pte_wrprotect(entry);
> +		set_pte_at(mm, address, pte, entry);
> +	}
> +	ret = 1;
> +
> +out_unlock:
> +	pte_unmap_unlock(pte, ptl);
> +out:
> +	return ret;
> +}

OK.  I think.  We need to find a way of provoking Hugh to look at it.

> +static int page_wrprotect_file(struct page *page, int *odirect_sync)
> +{
> +	struct address_space *mapping;
> +	struct prio_tree_iter iter;
> +	struct vm_area_struct *vma;
> +	pgoff_t pgoff = page->index << (PAGE_CACHE_SHIFT - PAGE_SHIFT);
> +	int ret = 0;
> +
> +	mapping = page_mapping(page);

What pins *mapping in memory?  Usually this is done by requiring that
the caller has locked the page.  But no such precondition is documented
here.

> +	if (!mapping)
> +		return ret;
> +
> +	spin_lock(&mapping->i_mmap_lock);
> +
> +	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff)
> +		ret += page_wrprotect_one(page, vma, odirect_sync);
> +
> +	spin_unlock(&mapping->i_mmap_lock);
> +
> +	return ret;
> +}
> +
> +static int page_wrprotect_anon(struct page *page, int *odirect_sync)
> +{
> +	struct vm_area_struct *vma;
> +	struct anon_vma *anon_vma;
> +	int ret = 0;
> +
> +	anon_vma = page_lock_anon_vma(page);
> +	if (!anon_vma)
> +		return ret;
> +
> +	list_for_each_entry(vma, &anon_vma->head, anon_vma_node)
> +		ret += page_wrprotect_one(page, vma, odirect_sync);
> +
> +	page_unlock_anon_vma(anon_vma);
> +
> +	return ret;
> +}
> +
> +/**

This token means "this is a kerneldoc comment".

> + * set all the ptes pointed to a page as read only,
> + * odirect_sync is set to 0 in case we cannot protect against race with odirect
> + * return the number of ptes that were set as read only
> + * (ptes that were read only before this function was called are couned as well)
> + */

But it isn't.

I don't understand this odirect_sync thing.  What race?  Please expand
this comment to make the function of odirect_sync more understandable.

> +int page_wrprotect(struct page *page, int *odirect_sync)
> +{
> +	int ret =0;
> +
> +	*odirect_sync = 1;
> +	if (PageAnon(page))
> +		ret = page_wrprotect_anon(page, odirect_sync);
> +	else
> +		ret = page_wrprotect_file(page, odirect_sync);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(page_wrprotect);

What do you think about making all this new code dependent upon some
CONFIG_ switch which CONFIG_KVM can select?