Re: [PATCH 1/5] mm: tracking shared dirty pages

[Hugh Dickins]

On Tue, 27 Jun 2006, Peter Zijlstra wrote:
> @@ -796,6 +797,44 @@ struct shrinker;
>  extern struct shrinker *set_shrinker(int, shrinker_t);
>  extern void remove_shrinker(struct shrinker *shrinker);
>  
> +#define VM_NOTIFY_NO_PROT	0x01
> +#define VM_NOTIFY_NO_MKWRITE	0x02
> +
> +/*
> + * Some shared mappigns will want the pages marked read-only
> + * to track write events. If so, we'll downgrade vm_page_prot
> + * to the private version (using protection_map[] without the
> + * VM_SHARED bit).
> + */
> +static inline int vma_wants_writenotify(struct vm_area_struct *vma, int flags)
> +{
> +	unsigned int vm_flags = vma->vm_flags;
> +
> +	/* If it was private or non-writable, the write bit is already clear */
> +	if ((vm_flags & (VM_WRITE|VM_SHARED)) != ((VM_WRITE|VM_SHARED)))
> +		return 0;
> +
> +	/* The backer wishes to know when pages are first written to? */
> +	if (!(flags & VM_NOTIFY_NO_MKWRITE) &&
> +			vma->vm_ops && vma->vm_ops->page_mkwrite)
> +		return 1;
> +
> +	/* The open routine did something to the protections already? */
> +	if (!(flags & VM_NOTIFY_NO_PROT) &&
> +			pgprot_val(vma->vm_page_prot) !=
> +			pgprot_val(protection_map[vm_flags &
> +				(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]))
> +		return 0;

Sorry to be such a bore, but this is far from an improvement.

Try to resist adding flags to condition how a function behaves:
there are a million places where it's necessary or accepted, but
avoid it if you reasonably can.  And negative flags are particularly
hard to understand ("SKIP" would have been easier than "NO").

Just separate out the pgprot_val check from vma_wants_writenotify,
making that additional test in the case of do_mmap_pgoff.  Or if
you prefer, go back to how you had it before, with mprotect_fixup
making sure that that test will succeed.

In the case of page_mkclean, I see no need for vma_wants_writenotify
at all: you're overdesigning for some imaginary use of page_mkclean.
Just apply to the VM_SHARED vmas, with page_mkclean_one saying
	if (!pte_dirty(*pte) && !pte_write(*pte))
		goto unlock;

Hugh
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/