LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [patch 0/6] [RFC] MMU Notifiers V3
@ 2008-01-30  2:29 Christoph Lameter
  2008-01-30  2:29 ` [patch 1/6] mmu_notifier: Core code Christoph Lameter
                   ` (5 more replies)
  0 siblings, 6 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30  2:29 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

This is a patchset implementing MMU notifier callbacks based on Andrea's
earlier work. These are needed if Linux pages are referenced from something
else than tracked by the rmaps of the kernel. The known immediate users are

KVM (establishes a refcount to the page. External references called spte)

GRU (simple TLB shootdown without refcount. Has its own pagetable/tlb)

XPmem (uses its own reverse mappings and refcount. Remote ptes, Needs
	to sleep when sending messages)

Issues:

- Feedback from uses of the callbacks for KVM, RDMA, XPmem and GRU
  Early tests with the GRU were successful.

- Pages may be freed before the external mapping are torn down
  through invalidate_range() if no refcount on the page is taken.
  There is the chance that page content may be visible after
  they have been reallocated (mainly an issue for the GRU that
  takes no refcount).

- invalidate_range() callbacks are sometimes called under i_mmap_lock.
  These need to be dealt with or XPmem needs to be able to work around
  these.

- filemap_xip.c does not follow conventions for Rmap callbacks.
  We could depends on XIP support not being active to avoid the issue.

Things that we leave as is:

- RCU quiescent periods are required on registering and unregistering
  notifiers to guarantee visibility to other processors.
  Currently only mmu_notifier_release() does the correct thing.
  It is up to the user to provide RCU quiescent periods for
  register/unregister functions if they are called outside of the
  ->release method.

Andrea's mmu_notifier #4 -> RFC V1

- Merge subsystem rmap based with Linux rmap based approach
- Move Linux rmap based notifiers out of macro
- Try to account for what locks are held while the notifiers are
  called.
- Develop a patch sequence that separates out the different types of
  hooks so that we can review their use.
- Avoid adding include to linux/mm_types.h
- Integrate RCU logic suggested by Peter.

V1->V2:
- Improve RCU support
- Use mmap_sem for mmu_notifier register / unregister
- Drop invalidate_page from COW, mm/fremap.c and mm/rmap.c since we
  already have invalidate_range() callbacks there.
- Clean compile for !MMU_NOTIFIER
- Isolate filemap_xip strangeness into its own diff
- Pass a the flag to invalidate_range to indicate if a spinlock
  is held.
- Add invalidate_all()

V2->V3:
- Further RCU fixes
- Fixes from Andrea to fixup aging and move invalidate_range() in do_wp_page
  and sys_remap_file_pages() after the pte clearing.

-- 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* [patch 1/6] mmu_notifier: Core code
  2008-01-30  2:29 [patch 0/6] [RFC] MMU Notifiers V3 Christoph Lameter
@ 2008-01-30  2:29 ` Christoph Lameter
  2008-01-30 15:37   ` Andrea Arcangeli
  2008-01-30 18:02   ` Robin Holt
  2008-01-30  2:29 ` [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges Christoph Lameter
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30  2:29 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

[-- Attachment #1: mmu_core --]
[-- Type: text/plain, Size: 15337 bytes --]

Core code for mmu notifiers.

Signed-off-by: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Andrea Arcangeli <andrea@qumranet.com>

---
 include/linux/list.h         |   14 ++
 include/linux/mm_types.h     |    6 +
 include/linux/mmu_notifier.h |  210 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/page-flags.h   |   10 ++
 kernel/fork.c                |    2 
 mm/Kconfig                   |    4 
 mm/Makefile                  |    1 
 mm/mmap.c                    |    2 
 mm/mmu_notifier.c            |  101 ++++++++++++++++++++
 9 files changed, 350 insertions(+)

Index: linux-2.6/include/linux/mm_types.h
===================================================================
--- linux-2.6.orig/include/linux/mm_types.h	2008-01-29 16:56:33.000000000 -0800
+++ linux-2.6/include/linux/mm_types.h	2008-01-29 16:56:36.000000000 -0800
@@ -153,6 +153,10 @@ struct vm_area_struct {
 #endif
 };
 
+struct mmu_notifier_head {
+	struct hlist_head head;
+};
+
 struct mm_struct {
 	struct vm_area_struct * mmap;		/* list of VMAs */
 	struct rb_root mm_rb;
@@ -219,6 +223,8 @@ struct mm_struct {
 	/* aio bits */
 	rwlock_t		ioctx_list_lock;
 	struct kioctx		*ioctx_list;
+
+	struct mmu_notifier_head mmu_notifier; /* MMU notifier list */
 };
 
 #endif /* _LINUX_MM_TYPES_H */
Index: linux-2.6/include/linux/mmu_notifier.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/include/linux/mmu_notifier.h	2008-01-29 16:56:36.000000000 -0800
@@ -0,0 +1,210 @@
+#ifndef _LINUX_MMU_NOTIFIER_H
+#define _LINUX_MMU_NOTIFIER_H
+
+/*
+ * MMU motifier
+ *
+ * Notifier functions for hardware and software that establishes external
+ * references to pages of a Linux system. The notifier calls ensure that
+ * the external mappings are removed when the Linux VM removes memory ranges
+ * or individual pages from a process.
+ *
+ * These fall into two classes
+ *
+ * 1. mmu_notifier
+ *
+ * 	These are callbacks registered with an mm_struct. If mappings are
+ * 	removed from an address space then callbacks are performed.
+ * 	Spinlocks must be held in order to the walk reverse maps and the
+ * 	notifications are performed while the spinlock is held.
+ *
+ *
+ * 2. mmu_rmap_notifier
+ *
+ *	Callbacks for subsystems that provide their own rmaps. These
+ *	need to walk their own rmaps for a page. The invalidate_page
+ *	callback is outside of locks so that we are not in a strictly
+ *	atomic context (but we may be in a PF_MEMALLOC context if the
+ *	notifier is called from reclaim code) and are able to sleep.
+ *	Rmap notifiers need an extra page bit and are only available
+ *	on 64 bit platforms. It is up to the subsystem to mark pags
+ *	as PageExternalRmap as needed to trigger the callbacks. Pages
+ *	must be marked dirty if dirty bits are set in the external
+ *	pte.
+ */
+
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/rcupdate.h>
+#include <linux/mm_types.h>
+
+struct mmu_notifier_ops;
+
+struct mmu_notifier {
+	struct hlist_node hlist;
+	const struct mmu_notifier_ops *ops;
+};
+
+struct mmu_notifier_ops {
+	/*
+	 * Note: The mmu_notifier structure must be released with
+	 * call_rcu() since other processors are only guaranteed to
+	 * see the changes after a quiescent period.
+	 */
+	void (*release)(struct mmu_notifier *mn,
+			struct mm_struct *mm);
+
+	int (*age_page)(struct mmu_notifier *mn,
+			struct mm_struct *mm,
+			unsigned long address);
+
+	void (*invalidate_page)(struct mmu_notifier *mn,
+				struct mm_struct *mm,
+				unsigned long address);
+
+	/*
+	 * lock indicates that the function is called under spinlock.
+	 */
+	void (*invalidate_range)(struct mmu_notifier *mn,
+				 struct mm_struct *mm,
+				 unsigned long start, unsigned long end,
+				 int lock);
+};
+
+struct mmu_rmap_notifier_ops;
+
+struct mmu_rmap_notifier {
+	struct hlist_node hlist;
+	const struct mmu_rmap_notifier_ops *ops;
+};
+
+struct mmu_rmap_notifier_ops {
+	/*
+	 * Called with the page lock held after ptes are modified or removed
+	 * so that a subsystem with its own rmap's can remove remote ptes
+	 * mapping a page.
+	 */
+	void (*invalidate_page)(struct mmu_rmap_notifier *mrn,
+						struct page *page);
+};
+
+#ifdef CONFIG_MMU_NOTIFIER
+
+/*
+ * Must hold the mmap_sem for write.
+ *
+ * RCU is used to traverse the list. A quiescent period needs to pass
+ * before the notifier is guaranteed to be visible to all threads
+ */
+extern void __mmu_notifier_register(struct mmu_notifier *mn,
+				  struct mm_struct *mm);
+/* Will acquire mmap_sem for write*/
+extern void mmu_notifier_register(struct mmu_notifier *mn,
+				  struct mm_struct *mm);
+/*
+ * Will acquire mmap_sem for write.
+ *
+ * A quiescent period needs to pass before the mmu_notifier structure
+ * can be released. mmu_notifier_release() will wait for a quiescent period
+ * after calling the ->release callback. So it is safe to call
+ * mmu_notifier_unregister from the ->release function.
+ */
+extern void mmu_notifier_unregister(struct mmu_notifier *mn,
+				    struct mm_struct *mm);
+
+
+extern void mmu_notifier_release(struct mm_struct *mm);
+extern int mmu_notifier_age_page(struct mm_struct *mm,
+				 unsigned long address);
+
+static inline void mmu_notifier_head_init(struct mmu_notifier_head *mnh)
+{
+	INIT_HLIST_HEAD(&mnh->head);
+}
+
+#define mmu_notifier(function, mm, args...)				\
+	do {								\
+		struct mmu_notifier *__mn;				\
+		struct hlist_node *__n;					\
+									\
+		if (unlikely(!hlist_empty(&(mm)->mmu_notifier.head))) { \
+			rcu_read_lock();				\
+			hlist_for_each_entry_rcu(__mn, __n,		\
+					     &(mm)->mmu_notifier.head,	\
+					     hlist)			\
+				if (__mn->ops->function)		\
+					__mn->ops->function(__mn,	\
+							    mm,		\
+							    args);	\
+			rcu_read_unlock();				\
+		}							\
+	} while (0)
+
+extern void mmu_rmap_notifier_register(struct mmu_rmap_notifier *mrn);
+extern void mmu_rmap_notifier_unregister(struct mmu_rmap_notifier *mrn);
+
+extern struct hlist_head mmu_rmap_notifier_list;
+
+#define mmu_rmap_notifier(function, args...)				\
+	do {								\
+		struct mmu_rmap_notifier *__mrn;			\
+		struct hlist_node *__n;					\
+									\
+		rcu_read_lock();					\
+		hlist_for_each_entry_rcu(__mrn, __n,			\
+				&mmu_rmap_notifier_list, 		\
+						hlist)			\
+			if (__mrn->ops->function)			\
+				__mrn->ops->function(__mrn, args);	\
+		rcu_read_unlock();					\
+	} while (0);
+
+#else /* CONFIG_MMU_NOTIFIER */
+
+/*
+ * Notifiers that use the parameters that they were passed so that the
+ * compiler does not complain about unused variables but does proper
+ * parameter checks even if !CONFIG_MMU_NOTIFIER.
+ * Macros generate no code.
+ */
+#define mmu_notifier(function, mm, args...)				\
+	do {								\
+		if (0) {						\
+			struct mmu_notifier *__mn;			\
+									\
+			__mn = (struct mmu_notifier *)(0x00ff);		\
+			__mn->ops->function(__mn, mm, args);		\
+		};							\
+	} while (0)
+
+#define mmu_rmap_notifier(function, args...)				\
+	do {								\
+		if (0) {						\
+			struct mmu_rmap_notifier *__mrn;		\
+									\
+			__mrn = (struct mmu_rmap_notifier *)(0x00ff);	\
+			__mrn->ops->function(__mrn, args);		\
+		}							\
+	} while (0);
+
+static inline void mmu_notifier_register(struct mmu_notifier *mn,
+						struct mm_struct *mm) {}
+static inline void mmu_notifier_unregister(struct mmu_notifier *mn,
+						struct mm_struct *mm) {}
+static inline void mmu_notifier_release(struct mm_struct *mm) {}
+static inline int mmu_notifier_age_page(struct mm_struct *mm,
+				unsigned long address)
+{
+	return 0;
+}
+
+static inline void mmu_notifier_head_init(struct mmu_notifier_head *mmh) {}
+
+static inline void mmu_rmap_notifier_register(struct mmu_rmap_notifier *mrn)
+									{}
+static inline void mmu_rmap_notifier_unregister(struct mmu_rmap_notifier *mrn)
+									{}
+
+#endif /* CONFIG_MMU_NOTIFIER */
+
+#endif /* _LINUX_MMU_NOTIFIER_H */
Index: linux-2.6/include/linux/page-flags.h
===================================================================
--- linux-2.6.orig/include/linux/page-flags.h	2008-01-29 16:56:33.000000000 -0800
+++ linux-2.6/include/linux/page-flags.h	2008-01-29 16:56:36.000000000 -0800
@@ -105,6 +105,7 @@
  * 64 bit  |           FIELDS             | ??????         FLAGS         |
  *         63                            32                              0
  */
+#define PG_external_rmap	30	/* Page has external rmap */
 #define PG_uncached		31	/* Page has been mapped as uncached */
 #endif
 
@@ -260,6 +261,15 @@ static inline void __ClearPageTail(struc
 #define SetPageUncached(page)	set_bit(PG_uncached, &(page)->flags)
 #define ClearPageUncached(page)	clear_bit(PG_uncached, &(page)->flags)
 
+#if defined(CONFIG_MMU_NOTIFIER) && defined(CONFIG_64BIT)
+#define PageExternalRmap(page)	test_bit(PG_external_rmap, &(page)->flags)
+#define SetPageExternalRmap(page) set_bit(PG_external_rmap, &(page)->flags)
+#define ClearPageExternalRmap(page) clear_bit(PG_external_rmap, \
+							&(page)->flags)
+#else
+#define PageExternalRmap(page)	0
+#endif
+
 struct page;	/* forward declaration */
 
 extern void cancel_dirty_page(struct page *page, unsigned int account_size);
Index: linux-2.6/mm/Kconfig
===================================================================
--- linux-2.6.orig/mm/Kconfig	2008-01-29 16:56:33.000000000 -0800
+++ linux-2.6/mm/Kconfig	2008-01-29 16:56:36.000000000 -0800
@@ -193,3 +193,7 @@ config NR_QUICK
 config VIRT_TO_BUS
 	def_bool y
 	depends on !ARCH_NO_VIRT_TO_BUS
+
+config MMU_NOTIFIER
+	def_bool y
+	bool "MMU notifier, for paging KVM/RDMA"
Index: linux-2.6/mm/Makefile
===================================================================
--- linux-2.6.orig/mm/Makefile	2008-01-29 16:56:33.000000000 -0800
+++ linux-2.6/mm/Makefile	2008-01-29 16:56:36.000000000 -0800
@@ -30,4 +30,5 @@ obj-$(CONFIG_FS_XIP) += filemap_xip.o
 obj-$(CONFIG_MIGRATION) += migrate.o
 obj-$(CONFIG_SMP) += allocpercpu.o
 obj-$(CONFIG_QUICKLIST) += quicklist.o
+obj-$(CONFIG_MMU_NOTIFIER) += mmu_notifier.o
 
Index: linux-2.6/mm/mmu_notifier.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/mm/mmu_notifier.c	2008-01-29 16:57:26.000000000 -0800
@@ -0,0 +1,101 @@
+/*
+ *  linux/mm/mmu_notifier.c
+ *
+ *  Copyright (C) 2008  Qumranet, Inc.
+ *  Copyright (C) 2008  SGI
+ *  		Christoph Lameter <clameter@sgi.com>
+ *
+ *  This work is licensed under the terms of the GNU GPL, version 2. See
+ *  the COPYING file in the top-level directory.
+ */
+
+#include <linux/mmu_notifier.h>
+#include <linux/module.h>
+
+void mmu_notifier_release(struct mm_struct *mm)
+{
+	struct mmu_notifier *mn;
+	struct hlist_node *n, *t;
+
+	if (unlikely(!hlist_empty(&mm->mmu_notifier.head))) {
+		rcu_read_lock();
+		hlist_for_each_entry_safe_rcu(mn, n, t,
+					  &mm->mmu_notifier.head, hlist) {
+			hlist_del_rcu(&mn->hlist);
+			if (mn->ops->release)
+				mn->ops->release(mn, mm);
+		}
+		rcu_read_unlock();
+		synchronize_rcu();
+	}
+}
+
+/*
+ * If no young bitflag is supported by the hardware, ->age_page can
+ * unmap the address and return 1 or 0 depending if the mapping previously
+ * existed or not.
+ */
+int mmu_notifier_age_page(struct mm_struct *mm, unsigned long address)
+{
+	struct mmu_notifier *mn;
+	struct hlist_node *n;
+	int young = 0;
+
+	if (unlikely(!hlist_empty(&mm->mmu_notifier.head))) {
+		rcu_read_lock();
+		hlist_for_each_entry_rcu(mn, n,
+					  &mm->mmu_notifier.head, hlist) {
+			if (mn->ops->age_page)
+				young |= mn->ops->age_page(mn, mm, address);
+		}
+		rcu_read_unlock();
+	}
+
+	return young;
+}
+
+/*
+ * Note that all notifiers use RCU. The updates are only guaranteed to be
+ * visible to other processes after a RCU quiescent period!
+ */
+void __mmu_notifier_register(struct mmu_notifier *mn, struct mm_struct *mm)
+{
+	hlist_add_head_rcu(&mn->hlist, &mm->mmu_notifier.head);
+}
+EXPORT_SYMBOL_GPL(__mmu_notifier_register);
+
+void mmu_notifier_register(struct mmu_notifier *mn, struct mm_struct *mm)
+{
+	down_write(&mm->mmap_sem);
+	__mmu_notifier_register(mn, mm);
+	up_write(&mm->mmap_sem);
+}
+EXPORT_SYMBOL_GPL(mmu_notifier_register);
+
+void mmu_notifier_unregister(struct mmu_notifier *mn, struct mm_struct *mm)
+{
+	down_write(&mm->mmap_sem);
+	hlist_del_rcu(&mn->hlist);
+	up_write(&mm->mmap_sem);
+}
+EXPORT_SYMBOL_GPL(mmu_notifier_unregister);
+
+static DEFINE_SPINLOCK(mmu_notifier_list_lock);
+HLIST_HEAD(mmu_rmap_notifier_list);
+
+void mmu_rmap_notifier_register(struct mmu_rmap_notifier *mrn)
+{
+	spin_lock(&mmu_notifier_list_lock);
+	hlist_add_head_rcu(&mrn->hlist, &mmu_rmap_notifier_list);
+	spin_unlock(&mmu_notifier_list_lock);
+}
+EXPORT_SYMBOL(mmu_rmap_notifier_register);
+
+void mmu_rmap_notifier_unregister(struct mmu_rmap_notifier *mrn)
+{
+	spin_lock(&mmu_notifier_list_lock);
+	hlist_del_rcu(&mrn->hlist);
+	spin_unlock(&mmu_notifier_list_lock);
+}
+EXPORT_SYMBOL(mmu_rmap_notifier_unregister);
+
Index: linux-2.6/kernel/fork.c
===================================================================
--- linux-2.6.orig/kernel/fork.c	2008-01-29 16:56:33.000000000 -0800
+++ linux-2.6/kernel/fork.c	2008-01-29 16:56:36.000000000 -0800
@@ -52,6 +52,7 @@
 #include <linux/tty.h>
 #include <linux/proc_fs.h>
 #include <linux/blkdev.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
@@ -360,6 +361,7 @@ static struct mm_struct * mm_init(struct
 
 	if (likely(!mm_alloc_pgd(mm))) {
 		mm->def_flags = 0;
+		mmu_notifier_head_init(&mm->mmu_notifier);
 		return mm;
 	}
 	free_mm(mm);
Index: linux-2.6/mm/mmap.c
===================================================================
--- linux-2.6.orig/mm/mmap.c	2008-01-29 16:56:33.000000000 -0800
+++ linux-2.6/mm/mmap.c	2008-01-29 16:56:36.000000000 -0800
@@ -26,6 +26,7 @@
 #include <linux/mount.h>
 #include <linux/mempolicy.h>
 #include <linux/rmap.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/uaccess.h>
 #include <asm/cacheflush.h>
@@ -2043,6 +2044,7 @@ void exit_mmap(struct mm_struct *mm)
 	vm_unacct_memory(nr_accounted);
 	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, 0);
 	tlb_finish_mmu(tlb, 0, end);
+	mmu_notifier_release(mm);
 
 	/*
 	 * Walk the list again, actually closing and freeing it,
Index: linux-2.6/include/linux/list.h
===================================================================
--- linux-2.6.orig/include/linux/list.h	2008-01-29 16:56:33.000000000 -0800
+++ linux-2.6/include/linux/list.h	2008-01-29 16:56:36.000000000 -0800
@@ -991,6 +991,20 @@ static inline void hlist_add_after_rcu(s
 		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
 	     pos = pos->next)
 
+/**
+ * hlist_for_each_entry_safe_rcu	- iterate over list of given type
+ * @tpos:	the type * to use as a loop cursor.
+ * @pos:	the &struct hlist_node to use as a loop cursor.
+ * @n:		temporary pointer
+ * @head:	the head for your list.
+ * @member:	the name of the hlist_node within the struct.
+ */
+#define hlist_for_each_entry_safe_rcu(tpos, pos, n, head, member)	 \
+	for (pos = (head)->first;					 \
+	     rcu_dereference(pos) && ({ n = pos->next; 1;}) &&		 \
+		({ tpos = hlist_entry(pos, typeof(*tpos), member); 1;}); \
+	     pos = n)
+
 #else
 #warning "don't include kernel headers in userspace"
 #endif /* __KERNEL__ */

-- 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30  2:29 [patch 0/6] [RFC] MMU Notifiers V3 Christoph Lameter
  2008-01-30  2:29 ` [patch 1/6] mmu_notifier: Core code Christoph Lameter
@ 2008-01-30  2:29 ` Christoph Lameter
  2008-01-30  2:29 ` [patch 3/6] mmu_notifier: invalidate_page callbacks for subsystems with rmap Christoph Lameter
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30  2:29 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

[-- Attachment #1: mmu_invalidate_range_callbacks --]
[-- Type: text/plain, Size: 4641 bytes --]

The invalidation of address ranges in a mm_struct needs to be
performed when pages are removed or permissions etc change.
Most of the VM address space changes can use the range invalidate
callback.

invalidate_range() is generally called with mmap_sem held but
no spinlocks are active. If invalidate_range() is called with
locks held then we pass a flag into invalidate_range()

Comments state that mmap_sem must be held for
remap_pfn_range() but various drivers do not seem to do this.

Signed-off-by: Andrea Arcangeli <andrea@qumranet.com>
Signed-off-by: Robin Holt <holt@sgi.com>
Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 mm/fremap.c  |    2 ++
 mm/hugetlb.c |    2 ++
 mm/memory.c  |   11 +++++++++--
 mm/mmap.c    |    1 +
 4 files changed, 14 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/fremap.c
===================================================================
--- linux-2.6.orig/mm/fremap.c	2008-01-29 16:56:33.000000000 -0800
+++ linux-2.6/mm/fremap.c	2008-01-29 16:59:24.000000000 -0800
@@ -15,6 +15,7 @@
 #include <linux/rmap.h>
 #include <linux/module.h>
 #include <linux/syscalls.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/mmu_context.h>
 #include <asm/cacheflush.h>
@@ -212,6 +213,7 @@ asmlinkage long sys_remap_file_pages(uns
 	}
 
 	err = populate_range(mm, vma, start, size, pgoff);
+	mmu_notifier(invalidate_range, mm, start, start + size, 0);
 	if (!err && !(flags & MAP_NONBLOCK)) {
 		if (unlikely(has_write_lock)) {
 			downgrade_write(&mm->mmap_sem);
Index: linux-2.6/mm/memory.c
===================================================================
--- linux-2.6.orig/mm/memory.c	2008-01-29 16:56:33.000000000 -0800
+++ linux-2.6/mm/memory.c	2008-01-29 16:59:24.000000000 -0800
@@ -50,6 +50,7 @@
 #include <linux/delayacct.h>
 #include <linux/init.h>
 #include <linux/writeback.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/pgalloc.h>
 #include <asm/uaccess.h>
@@ -891,6 +892,8 @@ unsigned long zap_page_range(struct vm_a
 	end = unmap_vmas(&tlb, vma, address, end, &nr_accounted, details);
 	if (tlb)
 		tlb_finish_mmu(tlb, address, end);
+	mmu_notifier(invalidate_range, mm, address, end,
+		(details ? (details->i_mmap_lock != NULL)  : 0));
 	return end;
 }
 
@@ -1319,7 +1322,7 @@ int remap_pfn_range(struct vm_area_struc
 {
 	pgd_t *pgd;
 	unsigned long next;
-	unsigned long end = addr + PAGE_ALIGN(size);
+	unsigned long start = addr, end = addr + PAGE_ALIGN(size);
 	struct mm_struct *mm = vma->vm_mm;
 	int err;
 
@@ -1360,6 +1363,7 @@ int remap_pfn_range(struct vm_area_struc
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
+	mmu_notifier(invalidate_range, mm, start, end, 0);
 	return err;
 }
 EXPORT_SYMBOL(remap_pfn_range);
@@ -1443,7 +1447,7 @@ int apply_to_page_range(struct mm_struct
 {
 	pgd_t *pgd;
 	unsigned long next;
-	unsigned long end = addr + size;
+	unsigned long start = addr, end = addr + size;
 	int err;
 
 	BUG_ON(addr >= end);
@@ -1454,6 +1458,7 @@ int apply_to_page_range(struct mm_struct
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
+	mmu_notifier(invalidate_range, mm, start, end, 0);
 	return err;
 }
 EXPORT_SYMBOL_GPL(apply_to_page_range);
@@ -1669,6 +1674,8 @@ gotten:
 		page_cache_release(old_page);
 unlock:
 	pte_unmap_unlock(page_table, ptl);
+	mmu_notifier(invalidate_range, mm, address,
+				address + PAGE_SIZE - 1, 0);
 	if (dirty_page) {
 		if (vma->vm_file)
 			file_update_time(vma->vm_file);
Index: linux-2.6/mm/mmap.c
===================================================================
--- linux-2.6.orig/mm/mmap.c	2008-01-29 16:56:36.000000000 -0800
+++ linux-2.6/mm/mmap.c	2008-01-29 16:58:15.000000000 -0800
@@ -1748,6 +1748,7 @@ static void unmap_region(struct mm_struc
 	free_pgtables(&tlb, vma, prev? prev->vm_end: FIRST_USER_ADDRESS,
 				 next? next->vm_start: 0);
 	tlb_finish_mmu(tlb, start, end);
+	mmu_notifier(invalidate_range, mm, start, end, 0);
 }
 
 /*
Index: linux-2.6/mm/hugetlb.c
===================================================================
--- linux-2.6.orig/mm/hugetlb.c	2008-01-29 16:56:33.000000000 -0800
+++ linux-2.6/mm/hugetlb.c	2008-01-29 16:58:15.000000000 -0800
@@ -14,6 +14,7 @@
 #include <linux/mempolicy.h>
 #include <linux/cpuset.h>
 #include <linux/mutex.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/page.h>
 #include <asm/pgtable.h>
@@ -763,6 +764,7 @@ void __unmap_hugepage_range(struct vm_ar
 	}
 	spin_unlock(&mm->page_table_lock);
 	flush_tlb_range(vma, start, end);
+	mmu_notifier(invalidate_range, mm, start, end, 1);
 	list_for_each_entry_safe(page, tmp, &page_list, lru) {
 		list_del(&page->lru);
 		put_page(page);

-- 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* [patch 3/6] mmu_notifier: invalidate_page callbacks for subsystems with rmap
  2008-01-30  2:29 [patch 0/6] [RFC] MMU Notifiers V3 Christoph Lameter
  2008-01-30  2:29 ` [patch 1/6] mmu_notifier: Core code Christoph Lameter
  2008-01-30  2:29 ` [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges Christoph Lameter
@ 2008-01-30  2:29 ` Christoph Lameter
  2008-01-30 18:03   ` Robin Holt
  2008-01-30  2:29 ` [patch 4/6] MMU notifier: invalidate_page callbacks using Linux rmaps Christoph Lameter
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30  2:29 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

[-- Attachment #1: mmu_invalidate_page_rmap_callbacks --]
[-- Type: text/plain, Size: 1416 bytes --]

Callbacks to remove individual pages if the subsystem has an
rmap capability. The pagelock is held but no spinlocks are held.
The refcount of the page is elevated so that dropping the refcount
in the subsystem will not directly free the page.

The callbacks occur after the Linux rmaps have been walked.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 mm/rmap.c |    6 ++++++
 1 file changed, 6 insertions(+)

Index: linux-2.6/mm/rmap.c
===================================================================
--- linux-2.6.orig/mm/rmap.c	2008-01-25 14:24:19.000000000 -0800
+++ linux-2.6/mm/rmap.c	2008-01-25 14:24:38.000000000 -0800
@@ -49,6 +49,7 @@
 #include <linux/rcupdate.h>
 #include <linux/module.h>
 #include <linux/kallsyms.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/tlbflush.h>
 
@@ -473,6 +474,8 @@ int page_mkclean(struct page *page)
 		struct address_space *mapping = page_mapping(page);
 		if (mapping) {
 			ret = page_mkclean_file(mapping, page);
+			if (unlikely(PageExternalRmap(page)))
+				mmu_rmap_notifier(invalidate_page, page);
 			if (page_test_dirty(page)) {
 				page_clear_dirty(page);
 				ret = 1;
@@ -971,6 +974,9 @@ int try_to_unmap(struct page *page, int 
 	else
 		ret = try_to_unmap_file(page, migration);
 
+	if (unlikely(PageExternalRmap(page)))
+		mmu_rmap_notifier(invalidate_page, page);
+
 	if (!page_mapped(page))
 		ret = SWAP_SUCCESS;
 	return ret;

-- 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* [patch 4/6] MMU notifier: invalidate_page callbacks using Linux rmaps
  2008-01-30  2:29 [patch 0/6] [RFC] MMU Notifiers V3 Christoph Lameter
                   ` (2 preceding siblings ...)
  2008-01-30  2:29 ` [patch 3/6] mmu_notifier: invalidate_page callbacks for subsystems with rmap Christoph Lameter
@ 2008-01-30  2:29 ` Christoph Lameter
  2008-01-30  2:29 ` [patch 5/6] mmu_notifier: Callbacks for xip_filemap.c Christoph Lameter
  2008-01-30  2:29 ` [patch 6/6] mmu_notifier: Add invalidate_all() Christoph Lameter
  5 siblings, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30  2:29 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

[-- Attachment #1: mmu_invalidate_page_callbacks --]
[-- Type: text/plain, Size: 2465 bytes --]

These notifiers here use the Linux rmaps to perform the callbacks.
In order to walk the rmaps locks must be held. Callbacks can therefore
only operate in an atomic context.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 mm/rmap.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Index: linux-2.6/mm/rmap.c
===================================================================
--- linux-2.6.orig/mm/rmap.c	2008-01-29 16:58:25.000000000 -0800
+++ linux-2.6/mm/rmap.c	2008-01-29 16:58:39.000000000 -0800
@@ -285,7 +285,8 @@ static int page_referenced_one(struct pa
 	if (!pte)
 		goto out;
 
-	if (ptep_clear_flush_young(vma, address, pte))
+	if (ptep_clear_flush_young(vma, address, pte) |
+	    mmu_notifier_age_page(mm, address))
 		referenced++;
 
 	/* Pretend the page is referenced if the task has the
@@ -435,6 +436,7 @@ static int page_mkclean_one(struct page 
 
 		flush_cache_page(vma, address, pte_pfn(*pte));
 		entry = ptep_clear_flush(vma, address, pte);
+		mmu_notifier(invalidate_page, mm, address);
 		entry = pte_wrprotect(entry);
 		entry = pte_mkclean(entry);
 		set_pte_at(mm, address, pte, entry);
@@ -680,7 +682,8 @@ static int try_to_unmap_one(struct page 
 	 * skipped over this mm) then we should reactivate it.
 	 */
 	if (!migration && ((vma->vm_flags & VM_LOCKED) ||
-			(ptep_clear_flush_young(vma, address, pte)))) {
+			(ptep_clear_flush_young(vma, address, pte) |
+				mmu_notifier_age_page(mm, address)))) {
 		ret = SWAP_FAIL;
 		goto out_unmap;
 	}
@@ -688,6 +691,7 @@ static int try_to_unmap_one(struct page 
 	/* Nuke the page table entry. */
 	flush_cache_page(vma, address, page_to_pfn(page));
 	pteval = ptep_clear_flush(vma, address, pte);
+	mmu_notifier(invalidate_page, mm, address);
 
 	/* Move the dirty bit to the physical page now the pte is gone. */
 	if (pte_dirty(pteval))
@@ -812,12 +816,14 @@ static void try_to_unmap_cluster(unsigne
 		page = vm_normal_page(vma, address, *pte);
 		BUG_ON(!page || PageAnon(page));
 
-		if (ptep_clear_flush_young(vma, address, pte))
+		if (ptep_clear_flush_young(vma, address, pte) |
+		    mmu_notifier_age_page(mm, address))
 			continue;
 
 		/* Nuke the page table entry. */
 		flush_cache_page(vma, address, pte_pfn(*pte));
 		pteval = ptep_clear_flush(vma, address, pte);
+		mmu_notifier(invalidate_page, mm, address);
 
 		/* If nonlinear, store the file page offset in the pte. */
 		if (page->index != linear_page_index(vma, address))

-- 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* [patch 5/6] mmu_notifier: Callbacks for xip_filemap.c
  2008-01-30  2:29 [patch 0/6] [RFC] MMU Notifiers V3 Christoph Lameter
                   ` (3 preceding siblings ...)
  2008-01-30  2:29 ` [patch 4/6] MMU notifier: invalidate_page callbacks using Linux rmaps Christoph Lameter
@ 2008-01-30  2:29 ` Christoph Lameter
  2008-01-30  2:29 ` [patch 6/6] mmu_notifier: Add invalidate_all() Christoph Lameter
  5 siblings, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30  2:29 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

[-- Attachment #1: mmu_xip --]
[-- Type: text/plain, Size: 1242 bytes --]

Problem for external rmaps: There is no pagelock held on the page.

Signed-off-by: Robin Holt <holt@sgi.com>

---
 mm/filemap_xip.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux-2.6/mm/filemap_xip.c
===================================================================
--- linux-2.6.orig/mm/filemap_xip.c	2008-01-25 19:39:04.000000000 -0800
+++ linux-2.6/mm/filemap_xip.c	2008-01-25 19:39:06.000000000 -0800
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/uio.h>
 #include <linux/rmap.h>
+#include <linux/mmu_notifier.h>
 #include <linux/sched.h>
 #include <asm/tlbflush.h>
 
@@ -183,6 +184,9 @@ __xip_unmap (struct address_space * mapp
 	if (!page)
 		return;
 
+	if (PageExternalRmap(page))
+		mmu_rmap_notifier(invalidate_page, page);
+
 	spin_lock(&mapping->i_mmap_lock);
 	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
 		mm = vma->vm_mm;
@@ -194,6 +198,7 @@ __xip_unmap (struct address_space * mapp
 			/* Nuke the page table entry. */
 			flush_cache_page(vma, address, pte_pfn(*pte));
 			pteval = ptep_clear_flush(vma, address, pte);
+			mmu_notifier(invalidate_page, mm, address);
 			page_remove_rmap(page, vma);
 			dec_mm_counter(mm, file_rss);
 			BUG_ON(pte_dirty(pteval));

-- 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* [patch 6/6] mmu_notifier: Add invalidate_all()
  2008-01-30  2:29 [patch 0/6] [RFC] MMU Notifiers V3 Christoph Lameter
                   ` (4 preceding siblings ...)
  2008-01-30  2:29 ` [patch 5/6] mmu_notifier: Callbacks for xip_filemap.c Christoph Lameter
@ 2008-01-30  2:29 ` Christoph Lameter
  5 siblings, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30  2:29 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

[-- Attachment #1: mmu_all --]
[-- Type: text/plain, Size: 1542 bytes --]

when a task exits we can remove all external pts at once. At that point the
extern mmu may also unregister itself from the mmu notifier chain to avoid
future calls.

Note the complications because of RCU. Other processors may not see that the
notifier was unlinked until a quiescent period has passed!

Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 include/linux/mmu_notifier.h |    4 ++++
 mm/mmap.c                    |    1 +
 2 files changed, 5 insertions(+)

Index: linux-2.6/include/linux/mmu_notifier.h
===================================================================
--- linux-2.6.orig/include/linux/mmu_notifier.h	2008-01-28 14:02:18.000000000 -0800
+++ linux-2.6/include/linux/mmu_notifier.h	2008-01-28 14:15:49.000000000 -0800
@@ -62,6 +62,10 @@ struct mmu_notifier_ops {
 				struct mm_struct *mm,
 				unsigned long address);
 
+	/* Dummy needed because the mmu_notifier() macro requires it */
+	void (*invalidate_all)(struct mmu_notifier *mn, struct mm_struct *mm,
+				int dummy);
+
 	/*
 	 * lock indicates that the function is called under spinlock.
 	 */
Index: linux-2.6/mm/mmap.c
===================================================================
--- linux-2.6.orig/mm/mmap.c	2008-01-28 14:15:49.000000000 -0800
+++ linux-2.6/mm/mmap.c	2008-01-28 14:15:49.000000000 -0800
@@ -2034,6 +2034,7 @@ void exit_mmap(struct mm_struct *mm)
 	unsigned long end;
 
 	/* mm's last user has gone, and its about to be pulled down */
+	mmu_notifier(invalidate_all, mm, 0);
 	arch_exit_mmap(mm);
 
 	lru_add_drain();

-- 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 1/6] mmu_notifier: Core code
  2008-01-30  2:29 ` [patch 1/6] mmu_notifier: Core code Christoph Lameter
@ 2008-01-30 15:37   ` Andrea Arcangeli
  2008-01-30 15:53     ` Jack Steiner
  2008-01-30 17:10     ` Peter Zijlstra
  2008-01-30 18:02   ` Robin Holt
  1 sibling, 2 replies; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-30 15:37 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, Jan 29, 2008 at 06:29:10PM -0800, Christoph Lameter wrote:
> +void mmu_notifier_release(struct mm_struct *mm)
> +{
> +	struct mmu_notifier *mn;
> +	struct hlist_node *n, *t;
> +
> +	if (unlikely(!hlist_empty(&mm->mmu_notifier.head))) {
> +		rcu_read_lock();
> +		hlist_for_each_entry_safe_rcu(mn, n, t,
> +					  &mm->mmu_notifier.head, hlist) {
> +			hlist_del_rcu(&mn->hlist);

This will race and kernel crash against mmu_notifier_register in
SMP. You should resurrect the per-mmu_notifier_head lock in my last
patch (except it can be converted from a rwlock_t to a regular
spinlock_t) and drop the mmap_sem from
mmu_notifier_register/unregister.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 1/6] mmu_notifier: Core code
  2008-01-30 15:37   ` Andrea Arcangeli
@ 2008-01-30 15:53     ` Jack Steiner
  2008-01-30 16:38       ` Andrea Arcangeli
  2008-01-30 19:19       ` Christoph Lameter
  2008-01-30 17:10     ` Peter Zijlstra
  1 sibling, 2 replies; 101+ messages in thread
From: Jack Steiner @ 2008-01-30 15:53 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Christoph Lameter, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Wed, Jan 30, 2008 at 04:37:49PM +0100, Andrea Arcangeli wrote:
> On Tue, Jan 29, 2008 at 06:29:10PM -0800, Christoph Lameter wrote:
> > +void mmu_notifier_release(struct mm_struct *mm)
> > +{
> > +	struct mmu_notifier *mn;
> > +	struct hlist_node *n, *t;
> > +
> > +	if (unlikely(!hlist_empty(&mm->mmu_notifier.head))) {
> > +		rcu_read_lock();
> > +		hlist_for_each_entry_safe_rcu(mn, n, t,
> > +					  &mm->mmu_notifier.head, hlist) {
> > +			hlist_del_rcu(&mn->hlist);
> 
> This will race and kernel crash against mmu_notifier_register in
> SMP. You should resurrect the per-mmu_notifier_head lock in my last
> patch (except it can be converted from a rwlock_t to a regular
> spinlock_t) and drop the mmap_sem from
> mmu_notifier_register/unregister.

Agree.

That will also resolve the problem we discussed yesterday. 
I want to unregister my mmu_notifier when a GRU segment is
unmapped. This would not necessarily be at task termination.

However, the mmap_sem is already held for write by the core
VM at the point I would call the unregister function.
Currently, there is no __mmu_notifier_unregister() defined.

Moving to a different lock solves the problem.


-- jack

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 1/6] mmu_notifier: Core code
  2008-01-30 15:53     ` Jack Steiner
@ 2008-01-30 16:38       ` Andrea Arcangeli
  2008-01-30 19:19       ` Christoph Lameter
  1 sibling, 0 replies; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-30 16:38 UTC (permalink / raw)
  To: Jack Steiner
  Cc: Christoph Lameter, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Wed, Jan 30, 2008 at 09:53:06AM -0600, Jack Steiner wrote:
> That will also resolve the problem we discussed yesterday. 
> I want to unregister my mmu_notifier when a GRU segment is
> unmapped. This would not necessarily be at task termination.

My proof that there is something wrong in the smp locking of the
current code is very simple: it can't be right to use
hlist_for_each_entry_safe_rcu and rcu_read_lock inside
mmu_notifier_release, and then to call hlist_del_rcu without any
spinlock or semaphore. If we walk the list with
hlist_for_each_entry_safe_rcu (and not with
hlist_for_each_entry_safe), it means the list _can_ change from under
us, and in turn the hlist_del_rcu must be surrounded by a spinlock or
sempahore too!

If by design the list _can't_ change from under us and calling
hlist_del_rcu was safe w/o locks, then hlist_for_each_entry_safe is
_sure_ enough for mmu_notifier_release, and rcu_read_lock most
certainly can be removed too.

To make an usage case where the race could trigger, I was thinking at
somebody bumping the mm_count (not mm_users) and registering a
notifier while mmu_notifier_release runs and relaying on ->release to
know if it has to run mmu_notifier_unregister. However I now started
wondering how it can relay on ->release to know that if ->release is
called after hlist_del_rcu because with the latest changes ->release
will also allow the mn to release itself ;). It's unsafe to call
list_del_rcu twice (the second will crash on a poisoned entry).

This starts to make me think we should remove the auto-disarming
feature and require the notifier-user to have the ->release call
mmu_notifier_unregister first and to free the "mn" inside ->release
too if needed. Or alternatively the notifier-user can bump mm_count
and to call a mmu_notifier_unregister before calling mmdrop (like kvm
could do).

Another approach is to simply define mmu_notifier_release as
implicitly serialized by other code design, with a real lock (not rcu)
against the whole register/unregister operations. So to guarantee the
notifier list can't change from under us while mmu_notifier_release
runs. If we go this route, yes, the auto-disarming hlist_del can be
kept, the current code would have been safe, but to avoid confusion
the mmu_notifier_release shall become this:

void mmu_notifier_release(struct mm_struct *mm)
{
	struct mmu_notifier *mn;
	struct hlist_node *n, *t;

	if (unlikely(!hlist_empty(&mm->mmu_notifier.head))) {
		hlist_for_each_entry_safe(mn, n, t,
					  &mm->mmu_notifier.head, hlist) {
			hlist_del(&mn->hlist);
			if (mn->ops->release)
				mn->ops->release(mn, mm);
		}
	}
}

> However, the mmap_sem is already held for write by the core
> VM at the point I would call the unregister function.
> Currently, there is no __mmu_notifier_unregister() defined.
> 
> Moving to a different lock solves the problem.

Unless the mmu_notifier_release becomes like above and we rely on the
user of the mmu notifiers to implement a highlevel external lock that
will we definitely forbid to bump the mm_count of the mm, and to call
register/unregister while mmu_notifier_release could run, 1) moving to a
different lock and 2) removing the auto-disarming hlist_del_rcu from
mmu_notifier_release sounds the only possible smp safe way.

As far as KVM is concerned mmu_notifier_released could be changed to
the version I written above and everything should be ok. For KVM the
mm_count bump is done by the task that also holds a mm_user, so when
exit_mmap runs I don't think the list could possible change anymore.

Anyway those are details that can be perfected after mainline merging,
so this isn't something to worry about too much right now. My idea is
to keep working to perfect it while I hope progress is being made by
Christoph to merge the mmu notifiers V3 patchset in mainline ;).

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 1/6] mmu_notifier: Core code
  2008-01-30 15:37   ` Andrea Arcangeli
  2008-01-30 15:53     ` Jack Steiner
@ 2008-01-30 17:10     ` Peter Zijlstra
  2008-01-30 19:28       ` Christoph Lameter
  1 sibling, 1 reply; 101+ messages in thread
From: Peter Zijlstra @ 2008-01-30 17:10 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Christoph Lameter, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, steiner,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins


On Wed, 2008-01-30 at 16:37 +0100, Andrea Arcangeli wrote:
> On Tue, Jan 29, 2008 at 06:29:10PM -0800, Christoph Lameter wrote:
> > +void mmu_notifier_release(struct mm_struct *mm)
> > +{
> > +	struct mmu_notifier *mn;
> > +	struct hlist_node *n, *t;
> > +
> > +	if (unlikely(!hlist_empty(&mm->mmu_notifier.head))) {
> > +		rcu_read_lock();
> > +		hlist_for_each_entry_safe_rcu(mn, n, t,
> > +					  &mm->mmu_notifier.head, hlist) {
> > +			hlist_del_rcu(&mn->hlist);
> 
> This will race and kernel crash against mmu_notifier_register in
> SMP. You should resurrect the per-mmu_notifier_head lock in my last
> patch (except it can be converted from a rwlock_t to a regular
> spinlock_t) and drop the mmap_sem from
> mmu_notifier_register/unregister.

Agreed, sorry for this oversight.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 1/6] mmu_notifier: Core code
  2008-01-30  2:29 ` [patch 1/6] mmu_notifier: Core code Christoph Lameter
  2008-01-30 15:37   ` Andrea Arcangeli
@ 2008-01-30 18:02   ` Robin Holt
  2008-01-30 19:08     ` Christoph Lameter
  2008-01-30 19:14     ` Christoph Lameter
  1 sibling, 2 replies; 101+ messages in thread
From: Robin Holt @ 2008-01-30 18:02 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	steiner, linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

Back to one of Andrea's points from a couple days ago, I think we still
have a problem with the PageExternalRmap page flag.

If I had two drivers with external rmap implementations, there is no way
I can think of for a simple flag to coordinate a single page being
exported and maintained by the two.

Since the intended use seems to point in the direction of the external
rmap must be maintained consistent with the all pages the driver has
exported and the driver will already need to handle cases where the page
does not appear in its rmap, I would propose the setting and clearing
should be handled in the mmu_notifier code.

This is the first of two patches.  This one is intended as an addition
to patch 1/6.  I will post the other shortly under the patch 3/6 thread.


Index: git-linus/include/linux/mmu_notifier.h
===================================================================
--- git-linus.orig/include/linux/mmu_notifier.h	2008-01-30 11:43:45.000000000 -0600
+++ git-linus/include/linux/mmu_notifier.h	2008-01-30 11:44:35.000000000 -0600
@@ -146,6 +146,7 @@ static inline void mmu_notifier_head_ini
 
 extern void mmu_rmap_notifier_register(struct mmu_rmap_notifier *mrn);
 extern void mmu_rmap_notifier_unregister(struct mmu_rmap_notifier *mrn);
+extern void mmu_rmap_export_page(struct page *page);
 
 extern struct hlist_head mmu_rmap_notifier_list;
 
Index: git-linus/mm/mmu_notifier.c
===================================================================
--- git-linus.orig/mm/mmu_notifier.c	2008-01-30 11:43:45.000000000 -0600
+++ git-linus/mm/mmu_notifier.c	2008-01-30 11:56:08.000000000 -0600
@@ -99,3 +99,8 @@ void mmu_rmap_notifier_unregister(struct
 }
 EXPORT_SYMBOL(mmu_rmap_notifier_unregister);
 
+void mmu_rmap_export_page(struct page *page)
+{
+	SetPageExternalRmap(page);
+}
+EXPORT_SYMBOL(mmu_rmap_export_page);

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 3/6] mmu_notifier: invalidate_page callbacks for subsystems with rmap
  2008-01-30  2:29 ` [patch 3/6] mmu_notifier: invalidate_page callbacks for subsystems with rmap Christoph Lameter
@ 2008-01-30 18:03   ` Robin Holt
  0 siblings, 0 replies; 101+ messages in thread
From: Robin Holt @ 2008-01-30 18:03 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	steiner, linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

This is the second part of a patch posted to patch 1/6.


Index: git-linus/mm/rmap.c
===================================================================
--- git-linus.orig/mm/rmap.c	2008-01-30 11:55:56.000000000 -0600
+++ git-linus/mm/rmap.c	2008-01-30 12:01:28.000000000 -0600
@@ -476,8 +476,10 @@ int page_mkclean(struct page *page)
 		struct address_space *mapping = page_mapping(page);
 		if (mapping) {
 			ret = page_mkclean_file(mapping, page);
-			if (unlikely(PageExternalRmap(page)))
+			if (unlikely(PageExternalRmap(page))) {
 				mmu_rmap_notifier(invalidate_page, page);
+				ClearPageExported(page);
+			}
 			if (page_test_dirty(page)) {
 				page_clear_dirty(page);
 				ret = 1;
@@ -980,8 +982,10 @@ int try_to_unmap(struct page *page, int 
 	else
 		ret = try_to_unmap_file(page, migration);
 
-	if (unlikely(PageExternalRmap(page)))
+	if (unlikely(PageExternalRmap(page))) {
 		mmu_rmap_notifier(invalidate_page, page);
+		ClearPageExported(page);
+	}
 
 	if (!page_mapped(page))
 		ret = SWAP_SUCCESS;

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 1/6] mmu_notifier: Core code
  2008-01-30 18:02   ` Robin Holt
@ 2008-01-30 19:08     ` Christoph Lameter
  2008-01-30 19:14     ` Christoph Lameter
  1 sibling, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30 19:08 UTC (permalink / raw)
  To: Robin Holt
  Cc: Andrea Arcangeli, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Wed, 30 Jan 2008, Robin Holt wrote:

> Index: git-linus/mm/mmu_notifier.c
> ===================================================================
> --- git-linus.orig/mm/mmu_notifier.c	2008-01-30 11:43:45.000000000 -0600
> +++ git-linus/mm/mmu_notifier.c	2008-01-30 11:56:08.000000000 -0600
> @@ -99,3 +99,8 @@ void mmu_rmap_notifier_unregister(struct
>  }
>  EXPORT_SYMBOL(mmu_rmap_notifier_unregister);
>  
> +void mmu_rmap_export_page(struct page *page)
> +{
> +	SetPageExternalRmap(page);
> +}
> +EXPORT_SYMBOL(mmu_rmap_export_page);

Then mmu_rmap_export_page would have to be called before the subsystem 
establishes the rmap entry for the page. Could we do all PageExternalRmap 
modifications under Pagelock?



^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 1/6] mmu_notifier: Core code
  2008-01-30 18:02   ` Robin Holt
  2008-01-30 19:08     ` Christoph Lameter
@ 2008-01-30 19:14     ` Christoph Lameter
  1 sibling, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30 19:14 UTC (permalink / raw)
  To: Robin Holt
  Cc: Andrea Arcangeli, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

Ok. So I added the following patch:

---
 include/linux/mmu_notifier.h |    1 +
 mm/mmu_notifier.c            |   12 ++++++++++++
 2 files changed, 13 insertions(+)

Index: linux-2.6/include/linux/mmu_notifier.h
===================================================================
--- linux-2.6.orig/include/linux/mmu_notifier.h	2008-01-30 11:09:06.000000000 -0800
+++ linux-2.6/include/linux/mmu_notifier.h	2008-01-30 11:10:38.000000000 -0800
@@ -146,6 +146,7 @@ static inline void mmu_notifier_head_ini
 
 extern void mmu_rmap_notifier_register(struct mmu_rmap_notifier *mrn);
 extern void mmu_rmap_notifier_unregister(struct mmu_rmap_notifier *mrn);
+extern void mmu_rmap_export_page(struct page *page);
 
 extern struct hlist_head mmu_rmap_notifier_list;
 
Index: linux-2.6/mm/mmu_notifier.c
===================================================================
--- linux-2.6.orig/mm/mmu_notifier.c	2008-01-30 11:09:01.000000000 -0800
+++ linux-2.6/mm/mmu_notifier.c	2008-01-30 11:12:10.000000000 -0800
@@ -99,3 +99,15 @@ void mmu_rmap_notifier_unregister(struct
 }
 EXPORT_SYMBOL(mmu_rmap_notifier_unregister);
 
+/*
+ * Export a page.
+ *
+ * Pagelock must be held.
+ * Must be called before a page is put on an external rmap.
+ */
+void mmu_rmap_export_page(struct page *page)
+{
+	BUG_ON(!PageLocked(page));
+	SetPageExternalRmap(page);
+}
+EXPORT_SYMBOL(mmu_rmap_export_page);


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 1/6] mmu_notifier: Core code
  2008-01-30 15:53     ` Jack Steiner
  2008-01-30 16:38       ` Andrea Arcangeli
@ 2008-01-30 19:19       ` Christoph Lameter
  2008-01-30 22:20         ` Robin Holt
  1 sibling, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30 19:19 UTC (permalink / raw)
  To: Jack Steiner
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Wed, 30 Jan 2008, Jack Steiner wrote:

> Moving to a different lock solves the problem.

Well it gets us back to the issue why we removed the lock. As Robin said 
before: If its global then we can have a huge number of tasks contending 
for the lock on startup of a process with a large number of ranks. The 
reason to go to mmap_sem was that it was placed in the mm_struct and so we 
would just have a couple of contentions per mm_struct.

I'll be looking for some other way to do this.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 1/6] mmu_notifier: Core code
  2008-01-30 17:10     ` Peter Zijlstra
@ 2008-01-30 19:28       ` Christoph Lameter
  0 siblings, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30 19:28 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, steiner,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

How about just taking the mmap_sem writelock in release? We have only a 
single caller of mmu_notifier_release() in mm/mmap.c and we know that we 
are not holding mmap_sem at that point. So just acquire it when needed?

Index: linux-2.6/mm/mmu_notifier.c
===================================================================
--- linux-2.6.orig/mm/mmu_notifier.c	2008-01-30 11:21:57.000000000 -0800
+++ linux-2.6/mm/mmu_notifier.c	2008-01-30 11:24:59.000000000 -0800
@@ -18,6 +19,7 @@ void mmu_notifier_release(struct mm_stru
 	struct hlist_node *n, *t;
 
 	if (unlikely(!hlist_empty(&mm->mmu_notifier.head))) {
+		down_write(&mm->mmap_sem);
 		rcu_read_lock();
 		hlist_for_each_entry_safe_rcu(mn, n, t,
 					  &mm->mmu_notifier.head, hlist) {
@@ -26,6 +28,7 @@ void mmu_notifier_release(struct mm_stru
 				mn->ops->release(mn, mm);
 		}
 		rcu_read_unlock();
+		up_write(&mm->mmap_sem);
 		synchronize_rcu();
 	}
 }

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 1/6] mmu_notifier: Core code
  2008-01-30 19:19       ` Christoph Lameter
@ 2008-01-30 22:20         ` Robin Holt
  2008-01-30 23:38           ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Robin Holt @ 2008-01-30 22:20 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Jack Steiner, Andrea Arcangeli, Robin Holt, Avi Kivity,
	Izik Eidus, Nick Piggin, kvm-devel, Benjamin Herrenschmidt,
	Peter Zijlstra, linux-kernel, linux-mm, daniel.blueman,
	Hugh Dickins

On Wed, Jan 30, 2008 at 11:19:28AM -0800, Christoph Lameter wrote:
> On Wed, 30 Jan 2008, Jack Steiner wrote:
> 
> > Moving to a different lock solves the problem.
> 
> Well it gets us back to the issue why we removed the lock. As Robin said 
> before: If its global then we can have a huge number of tasks contending 
> for the lock on startup of a process with a large number of ranks. The 
> reason to go to mmap_sem was that it was placed in the mm_struct and so we 
> would just have a couple of contentions per mm_struct.
> 
> I'll be looking for some other way to do this.

I think Andrea's original concept of the lock in the mmu_notifier_head
structure was the best.  I agree with him that it should be a spinlock
instead of the rw_lock.

Thanks,
Robin

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 1/6] mmu_notifier: Core code
  2008-01-30 22:20         ` Robin Holt
@ 2008-01-30 23:38           ` Andrea Arcangeli
  2008-01-30 23:55             ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-30 23:38 UTC (permalink / raw)
  To: Robin Holt
  Cc: Christoph Lameter, Jack Steiner, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Wed, Jan 30, 2008 at 04:20:35PM -0600, Robin Holt wrote:
> On Wed, Jan 30, 2008 at 11:19:28AM -0800, Christoph Lameter wrote:
> > On Wed, 30 Jan 2008, Jack Steiner wrote:
> > 
> > > Moving to a different lock solves the problem.
> > 
> > Well it gets us back to the issue why we removed the lock. As Robin said 
> > before: If its global then we can have a huge number of tasks contending 
> > for the lock on startup of a process with a large number of ranks. The 
> > reason to go to mmap_sem was that it was placed in the mm_struct and so we 
> > would just have a couple of contentions per mm_struct.
> > 
> > I'll be looking for some other way to do this.
> 
> I think Andrea's original concept of the lock in the mmu_notifier_head
> structure was the best.  I agree with him that it should be a spinlock
> instead of the rw_lock.

BTW, I don't see the scalability concern with huge number of tasks:
the lock is still in the mm, down_write(mm->mmap_sem); oneinstruction;
up_write(mm->mmap_sem) is always going to scale worse than
spin_lock(mm->somethingelse); oneinstruction;
spin_unlock(mm->somethinglese).

Furthermore if we go this route and we don't relay on implicit
serialization of all the mmu notifier users against exit_mmap
(i.e. the mmu notifier user must agree to stop calling
mmu_notifier_register on a mm after the last mmput) the autodisarming
feature will likely have to be removed or it can't possibly be safe to
run mmu_notifier_unregister while mmu_notifier_release runs. With the
auto-disarming feature, there is no way to safely know if
mmu_notifier_unregister has to be called or not. I'm ok with removing
the auto-disarming feature and to have as self-contained-as-possible
locking. Then mmu_notifier_release can just become the
invalidate_all_after and invalidate_all, invalidate_all_before.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 1/6] mmu_notifier: Core code
  2008-01-30 23:38           ` Andrea Arcangeli
@ 2008-01-30 23:55             ` Christoph Lameter
  2008-01-31  0:12               ` [kvm-devel] " Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30 23:55 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Jack Steiner, Avi Kivity, Izik Eidus, Nick Piggin,
	kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Thu, 31 Jan 2008, Andrea Arcangeli wrote:

> > I think Andrea's original concept of the lock in the mmu_notifier_head
> > structure was the best.  I agree with him that it should be a spinlock
> > instead of the rw_lock.
> 
> BTW, I don't see the scalability concern with huge number of tasks:
> the lock is still in the mm, down_write(mm->mmap_sem); oneinstruction;
> up_write(mm->mmap_sem) is always going to scale worse than
> spin_lock(mm->somethingelse); oneinstruction;
> spin_unlock(mm->somethinglese).

If we put it elsewhere in the mm then we increase the size of the memory 
used in the mm_struct.

> Furthermore if we go this route and we don't relay on implicit
> serialization of all the mmu notifier users against exit_mmap
> (i.e. the mmu notifier user must agree to stop calling
> mmu_notifier_register on a mm after the last mmput) the autodisarming
> feature will likely have to be removed or it can't possibly be safe to
> run mmu_notifier_unregister while mmu_notifier_release runs. With the
> auto-disarming feature, there is no way to safely know if
> mmu_notifier_unregister has to be called or not. I'm ok with removing
> the auto-disarming feature and to have as self-contained-as-possible
> locking. Then mmu_notifier_release can just become the
> invalidate_all_after and invalidate_all, invalidate_all_before.

Hmmmm.. exit_mmap is only called when the last reference is removed 
against the mm right? So no tasks are running anymore. No pages are left. 
Do we need to serialize at all for mmu_notifier_release?

 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [kvm-devel] [patch 1/6] mmu_notifier: Core code
  2008-01-30 23:55             ` Christoph Lameter
@ 2008-01-31  0:12               ` Andrea Arcangeli
  2008-01-31  1:27                 ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-31  0:12 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, Peter Zijlstra, linux-mm, Benjamin Herrenschmidt,
	Jack Steiner, linux-kernel, Avi Kivity, kvm-devel,
	daniel.blueman, Robin Holt, Hugh Dickins

On Wed, Jan 30, 2008 at 03:55:37PM -0800, Christoph Lameter wrote:
> On Thu, 31 Jan 2008, Andrea Arcangeli wrote:
> 
> > > I think Andrea's original concept of the lock in the mmu_notifier_head
> > > structure was the best.  I agree with him that it should be a spinlock
> > > instead of the rw_lock.
> > 
> > BTW, I don't see the scalability concern with huge number of tasks:
> > the lock is still in the mm, down_write(mm->mmap_sem); oneinstruction;
> > up_write(mm->mmap_sem) is always going to scale worse than
> > spin_lock(mm->somethingelse); oneinstruction;
> > spin_unlock(mm->somethinglese).
> 
> If we put it elsewhere in the mm then we increase the size of the memory 
> used in the mm_struct.

Yes, and it will increase of the same amount of RAM that you pretend
everyone to pay even if MMU_NOTIFIER=n after your patch is applied (vs
mine that generated 0 ram utilization increase when
MMU_NOTIFIER=n). And the additional ram will provide not just
self-contained locking but higher scalability too.

I think it's much more important to generate zero ram and CPU overhead
for the embedded (this is something I was very careful to enforce in
all my patches), than to reduce scalability and not having a self
contained locking on full configurations with MMU_NOTIFIER=y.

> Hmmmm.. exit_mmap is only called when the last reference is removed 
> against the mm right? So no tasks are running anymore. No pages are left. 
> Do we need to serialize at all for mmu_notifier_release?

KVM sure doesn't need any locking there.  I thought somebody had to
possibly take a pin on the "mm_count" and pretend to call
mmu_notifier_register at will until mmdrop was finally called, in a
out of order fashion given mmu_notifier_release was implemented like
if the list could change from under it. Note mmdrop != mmput. mmput
and in turn mm_users is the serialization point if you prefer to drop
all locking from _release. Nobody must ever attempt a mmu_notifier_*
after calling mmput for that mm. That should be enough to be
safe. I'm fine either ways...

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [kvm-devel] [patch 1/6] mmu_notifier: Core code
  2008-01-31  0:12               ` [kvm-devel] " Andrea Arcangeli
@ 2008-01-31  1:27                 ` Christoph Lameter
  0 siblings, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-31  1:27 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, Peter Zijlstra, linux-mm, Benjamin Herrenschmidt,
	Jack Steiner, linux-kernel, Avi Kivity, kvm-devel,
	daniel.blueman, Robin Holt, Hugh Dickins

On Thu, 31 Jan 2008, Andrea Arcangeli wrote:

> > Hmmmm.. exit_mmap is only called when the last reference is removed 
> > against the mm right? So no tasks are running anymore. No pages are left. 
> > Do we need to serialize at all for mmu_notifier_release?
> 
> KVM sure doesn't need any locking there.  I thought somebody had to
> possibly take a pin on the "mm_count" and pretend to call
> mmu_notifier_register at will until mmdrop was finally called, in a
> out of order fashion given mmu_notifier_release was implemented like
> if the list could change from under it. Note mmdrop != mmput. mmput
> and in turn mm_users is the serialization point if you prefer to drop
> all locking from _release. Nobody must ever attempt a mmu_notifier_*
> after calling mmput for that mm. That should be enough to be
> safe. I'm fine either ways...

exit_mmap (where we call invalidate_all() and release()) is called when 
mm_users == 0:

void mmput(struct mm_struct *mm)
{
        might_sleep();

        if (atomic_dec_and_test(&mm->mm_users)) {
                exit_aio(mm);
                exit_mmap(mm);
                if (!list_empty(&mm->mmlist)) {
                        spin_lock(&mmlist_lock);
                        list_del(&mm->mmlist);
                        spin_unlock(&mmlist_lock);
                }
                put_swap_token(mm);
                mmdrop(mm);
        }
}
EXPORT_SYMBOL_GPL(mmput);

So there is only a single thread executing at the time when 
invalidate_all() is called from exit_mmap(). Then we drop the 
pages, and the page tables. After the page tables we call the ->release 
method and then remove the vmas.

So even dropping off the mmu_notifier chain in invalidate_all() could be 
done without an issue and without locking.

Trouble is if other callbacks attempt the same. Do we need to support the 
removal from the mmu_notifier list in invalidate_range()?


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-03-04 18:58             ` Christoph Lameter
@ 2008-03-05  0:52               ` Nick Piggin
  0 siblings, 0 replies; 101+ messages in thread
From: Nick Piggin @ 2008-03-05  0:52 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: akpm, Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	kvm-devel, Peter Zijlstra, general, Steve Wise, Roland Dreier,
	Kanoj Sarcar, steiner, linux-kernel, linux-mm, daniel.blueman

On Wednesday 05 March 2008 05:58, Christoph Lameter wrote:
> On Tue, 4 Mar 2008, Nick Piggin wrote:
> > > Then put it into the arch code for TLB invalidation. Paravirt ops gives
> > > good examples on how to do that.
> >
> > Put what into arch code?
>
> The mmu notifier code.

It isn't arch specific.


> > > > What about a completely different approach... XPmem runs over
> > > > NUMAlink, right? Why not provide some non-sleeping way to basically
> > > > IPI remote nodes over the NUMAlink where they can process the
> > > > invalidation? If you intra-node cache coherency has to run over this
> > > > link anyway, then presumably it is capable.
> > >
> > > There is another Linux instance at the remote end that first has to
> > > remove its own ptes.
> >
> > Yeah, what's the problem?
>
> The remote end has to invalidate the page which involves locking etc.

I don't see what the problem is.


> > > Also would not work for Inifiniband and other
> > > solutions.
> >
> > infiniband doesn't want it. Other solutions is just handwaving,
> > because if we don't know what the other soloutions are, then we can't
> > make any sort of informed choices.
>
> We need a solution in general to avoid the pinning problems. Infiniband
> has those too.
>
> > > All the approaches that require evictions in an atomic context
> > > are limiting the approach and do not allow the generic functionality
> > > that we want in order to not add alternate APIs for this.
> >
> > The only generic way to do this that I have seen (and the only proposed
> > way that doesn't add alternate APIs for that matter) is turning VM locks
> > into sleeping locks. In which case, Andrea's notifiers will work just
> > fine (except for relatively minor details like rcu list scanning).
>
> No they wont. As you pointed out the callback need RCU locking.

That can be fixed easily.


> > > The good enough solution right now is to pin pages by elevating
> > > refcounts.
> >
> > Which kind of leads to the question of why do you need any further
> > kernel patches if that is good enough?
>
> Well its good enough with severe problems during reclaim, livelocks etc.
> One could improve on that scheme through Rik's work trying to add a new
> page flag that mark pinned pages and then keep them off the LRUs and
> limiting their number. Having pinned page would limit the ability to
> reclaim by the VM and make page migration, memory unplug etc impossible.

Well not impossible. You could have a callback to invalidate the remote
TLB and drop the pin on a given page.


> It is better to have notifier scheme that allows to tell a device driver
> to free up the memory it has mapped.

Yeah, it would be nice for those people with clusters of Altixes. Doesn't
mean it has to go upstream, though.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-03-03 19:50           ` Nick Piggin
@ 2008-03-04 18:58             ` Christoph Lameter
  2008-03-05  0:52               ` Nick Piggin
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-03-04 18:58 UTC (permalink / raw)
  To: Nick Piggin
  Cc: akpm, Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	kvm-devel, Peter Zijlstra, general, Steve Wise, Roland Dreier,
	Kanoj Sarcar, steiner, linux-kernel, linux-mm, daniel.blueman

On Tue, 4 Mar 2008, Nick Piggin wrote:

> > Then put it into the arch code for TLB invalidation. Paravirt ops gives
> > good examples on how to do that.
> 
> Put what into arch code?

The mmu notifier code.

> > > What about a completely different approach... XPmem runs over NUMAlink,
> > > right? Why not provide some non-sleeping way to basically IPI remote
> > > nodes over the NUMAlink where they can process the invalidation? If you
> > > intra-node cache coherency has to run over this link anyway, then
> > > presumably it is capable.
> >
> > There is another Linux instance at the remote end that first has to
> > remove its own ptes.
> 
> Yeah, what's the problem?

The remote end has to invalidate the page which involves locking etc.

> > Also would not work for Inifiniband and other 
> > solutions.
> 
> infiniband doesn't want it. Other solutions is just handwaving,
> because if we don't know what the other soloutions are, then we can't
> make any sort of informed choices.

We need a solution in general to avoid the pinning problems. Infiniband 
has those too.

> > All the approaches that require evictions in an atomic context 
> > are limiting the approach and do not allow the generic functionality that
> > we want in order to not add alternate APIs for this.
> 
> The only generic way to do this that I have seen (and the only proposed
> way that doesn't add alternate APIs for that matter) is turning VM locks
> into sleeping locks. In which case, Andrea's notifiers will work just
> fine (except for relatively minor details like rcu list scanning).

No they wont. As you pointed out the callback need RCU locking.

> > The good enough solution right now is to pin pages by elevating
> > refcounts.
> 
> Which kind of leads to the question of why do you need any further
> kernel patches if that is good enough?

Well its good enough with severe problems during reclaim, livelocks etc. 
One could improve on that scheme through Rik's work trying to add a new 
page flag that mark pinned pages and then keep them off the LRUs and 
limiting their number. Having pinned page would limit the ability to 
reclaim by the VM and make page migration, memory unplug etc impossible. 
It is better to have notifier scheme that allows to tell a device driver 
to free up the memory it has mapped.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-03-03 19:28         ` Christoph Lameter
@ 2008-03-03 19:50           ` Nick Piggin
  2008-03-04 18:58             ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Nick Piggin @ 2008-03-03 19:50 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: akpm, Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	kvm-devel, Peter Zijlstra, general, Steve Wise, Roland Dreier,
	Kanoj Sarcar, steiner, linux-kernel, linux-mm, daniel.blueman

On Tuesday 04 March 2008 06:28, Christoph Lameter wrote:
> On Mon, 3 Mar 2008, Nick Piggin wrote:
> > Your skeleton is just registering notifiers and saying
> >
> > /* you fill the hard part in */
> >
> > If somebody needs a skeleton in order just to register the notifiers,
> > then almost by definition they are unqualified to write the hard
> > part ;)
>
> Its also providing a locking scheme.

Not the full locking scheme. If you have a look at the real code
required to do it, it is non trivial.


> > OK, there are ways to solve it or hack around it. But this is exactly
> > why I think the implementations should be kept seperate. Andrea's
> > notifiers are coherent, work on all types of mappings, and will
> > hopefully match closely the regular TLB invalidation sequence in the
> > Linux VM (at the moment it is quite close, but I hope to make it a
> > bit closer) so that it requires almost no changes to the mm.
>
> Then put it into the arch code for TLB invalidation. Paravirt ops gives
> good examples on how to do that.

Put what into arch code?


> > What about a completely different approach... XPmem runs over NUMAlink,
> > right? Why not provide some non-sleeping way to basically IPI remote
> > nodes over the NUMAlink where they can process the invalidation? If you
> > intra-node cache coherency has to run over this link anyway, then
> > presumably it is capable.
>
> There is another Linux instance at the remote end that first has to
> remove its own ptes.

Yeah, what's the problem?


> Also would not work for Inifiniband and other 
> solutions.

infiniband doesn't want it. Other solutions is just handwaving,
because if we don't know what the other soloutions are, then we can't
make any sort of informed choices.


> All the approaches that require evictions in an atomic context 
> are limiting the approach and do not allow the generic functionality that
> we want in order to not add alternate APIs for this.

The only generic way to do this that I have seen (and the only proposed
way that doesn't add alternate APIs for that matter) is turning VM locks
into sleeping locks. In which case, Andrea's notifiers will work just
fine (except for relatively minor details like rcu list scanning).

So I don't see what you're arguing for. There is no requirement that we
support sleeping notifiers in the same patch as non-sleeping ones.
Considering the simplicity of the non-sleeping notifiers and the
problems with sleeping ones, I think it is pretty clear that they are
different beasts (unless VM locking is changed).


> > Or another idea, why don't you LD_PRELOAD in the MPT library to also
> > intercept munmap, mprotect, mremap etc as well as just fork()? That
> > would give you similarly "good enough" coherency as the mmu notifier
> > patches except that you can't swap (which Robin said was not a big
> > problem).
>
> The good enough solution right now is to pin pages by elevating
> refcounts.

Which kind of leads to the question of why do you need any further
kernel patches if that is good enough?


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-03-03  5:11       ` Nick Piggin
@ 2008-03-03 19:28         ` Christoph Lameter
  2008-03-03 19:50           ` Nick Piggin
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-03-03 19:28 UTC (permalink / raw)
  To: Nick Piggin
  Cc: akpm, Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	kvm-devel, Peter Zijlstra, general, Steve Wise, Roland Dreier,
	Kanoj Sarcar, steiner, linux-kernel, linux-mm, daniel.blueman

On Mon, 3 Mar 2008, Nick Piggin wrote:

> Your skeleton is just registering notifiers and saying
> 
> /* you fill the hard part in */
> 
> If somebody needs a skeleton in order just to register the notifiers,
> then almost by definition they are unqualified to write the hard
> part ;)

Its also providing a locking scheme.

> OK, there are ways to solve it or hack around it. But this is exactly
> why I think the implementations should be kept seperate. Andrea's
> notifiers are coherent, work on all types of mappings, and will
> hopefully match closely the regular TLB invalidation sequence in the
> Linux VM (at the moment it is quite close, but I hope to make it a
> bit closer) so that it requires almost no changes to the mm.

Then put it into the arch code for TLB invalidation. Paravirt ops gives 
good examples on how to do that.

> What about a completely different approach... XPmem runs over NUMAlink,
> right? Why not provide some non-sleeping way to basically IPI remote
> nodes over the NUMAlink where they can process the invalidation? If you
> intra-node cache coherency has to run over this link anyway, then
> presumably it is capable.

There is another Linux instance at the remote end that first has to 
remove its own ptes. Also would not work for Inifiniband and other 
solutions. All the approaches that require evictions in an atomic context 
are limiting the approach and do not allow the generic functionality that 
we want in order to not add alternate APIs for this.

> Or another idea, why don't you LD_PRELOAD in the MPT library to also
> intercept munmap, mprotect, mremap etc as well as just fork()? That
> would give you similarly "good enough" coherency as the mmu notifier
> patches except that you can't swap (which Robin said was not a big
> problem).

The good enough solution right now is to pin pages by elevating 
refcounts.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-27 22:35     ` Christoph Lameter
                         ` (2 preceding siblings ...)
  2008-02-28  0:11       ` Andrea Arcangeli
@ 2008-03-03  5:11       ` Nick Piggin
  2008-03-03 19:28         ` Christoph Lameter
  3 siblings, 1 reply; 101+ messages in thread
From: Nick Piggin @ 2008-03-03  5:11 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: akpm, Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	kvm-devel, Peter Zijlstra, general, Steve Wise, Roland Dreier,
	Kanoj Sarcar, steiner, linux-kernel, linux-mm, daniel.blueman

On Thursday 28 February 2008 09:35, Christoph Lameter wrote:
> On Wed, 20 Feb 2008, Nick Piggin wrote:
> > On Friday 15 February 2008 17:49, Christoph Lameter wrote:

> > Also, what we are going to need here are not skeleton drivers
> > that just do all the *easy* bits (of registering their callbacks),
> > but actual fully working examples that do everything that any
> > real driver will need to do. If not for the sanity of the driver
> > writer, then for the sanity of the VM developers (I don't want
> > to have to understand xpmem or infiniband in order to understand
> > how the VM works).
>
> There are 3 different drivers that can already use it but the code is
> complex and not easy to review. Skeletons are easy to allow people to get
> started with it.

Your skeleton is just registering notifiers and saying

/* you fill the hard part in */

If somebody needs a skeleton in order just to register the notifiers,
then almost by definition they are unqualified to write the hard
part ;)


> > >  	lru_add_drain();
> > >  	tlb = tlb_gather_mmu(mm, 0);
> > >  	update_hiwater_rss(mm);
> > > +	mmu_notifier(invalidate_range_begin, mm, address, end, atomic);
> > >  	end = unmap_vmas(&tlb, vma, address, end, &nr_accounted, details);
> > >  	if (tlb)
> > >  		tlb_finish_mmu(tlb, address, end);
> > > +	mmu_notifier(invalidate_range_end, mm, address, end, atomic);
> > >  	return end;
> > >  }
> >
> > Where do you invalidate for munmap()?
>
> zap_page_range() called from unmap_vmas().

But it is not allowed to sleep. Where do you call the sleepable one
from?


> > Also, how to you resolve the case where you are not allowed to sleep?
> > I would have thought either you have to handle it, in which case nobody
> > needs to sleep; or you can't handle it, in which case the code is
> > broken.
>
> That can be done in a variety of ways:
>
> 1. Change VM locking
>
> 2. Not handle file backed mappings (XPmem could work mostly in such a
> config)
>
> 3. Keep the refcount elevated until pages are freed in another execution
> context.

OK, there are ways to solve it or hack around it. But this is exactly
why I think the implementations should be kept seperate. Andrea's
notifiers are coherent, work on all types of mappings, and will
hopefully match closely the regular TLB invalidation sequence in the
Linux VM (at the moment it is quite close, but I hope to make it a
bit closer) so that it requires almost no changes to the mm.

All the other things to try to make it sleep are either hacking holes
in it (eg by removing coherency). So I don't think it is reasonable to
require that any patch handle all cases. I actually think Andrea's
patch is quite nice and simple itself, wheras I am against the patches
that you posted.

What about a completely different approach... XPmem runs over NUMAlink,
right? Why not provide some non-sleeping way to basically IPI remote
nodes over the NUMAlink where they can process the invalidation? If you
intra-node cache coherency has to run over this link anyway, then
presumably it is capable.

Or another idea, why don't you LD_PRELOAD in the MPT library to also
intercept munmap, mprotect, mremap etc as well as just fork()? That
would give you similarly "good enough" coherency as the mmu notifier
patches except that you can't swap (which Robin said was not a big
problem).


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-29 22:12                                     ` Christoph Lameter
@ 2008-02-29 22:41                                       ` Andrea Arcangeli
  0 siblings, 0 replies; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-29 22:41 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Fri, Feb 29, 2008 at 02:12:57PM -0800, Christoph Lameter wrote:
> On Fri, 29 Feb 2008, Andrea Arcangeli wrote:
> 
> > > AFAICT The rw semaphore fastpath is similar in performance to a rw 
> > > spinlock. 
> > 
> > read side is taken in the slow path.
> 
> Slowpath meaning VM slowpath or lock slow path? Its seems that the rwsem 

With slow path I meant the VM. Sorry if that was confusing given locks
also have fast paths (no contention) and slow paths (contention).

> read side path is pretty efficient:

Yes. The assembly doesn't worry me at all.

> > pagefault is fast path, VM during swapping is slow path.
> 
> Not sure what you are saying here. A pagefault should be considered as a 
> fast path and swapping is not performance critical?

Yes, swapping is I/O bound and it rarely becomes CPU hog in the common
case.

There are corner case workloads (including OOM) where swapping can
become cpu bound (that's also where rwlock helps). But certainly the
speed of fork() and a page fault, is critical for _everyone_, not just
a few workloads and setups.

> Ok too many calls to schedule() because the slow path (of the semaphore) 
> is taken?

Yes, that's the only possible worry when converting a spinlock to
mutex.

> But that is only happening for the contended case. Certainly a spinlock is 
> better for 2p system but the more processors content for the lock (and 
> the longer the hold off is, typical for the processors with 4p or 8p or 
> more) the better a semaphore will work.

Sure. That's also why the PT lock switches for >4way compiles. Config
option helps to keep the VM optimal for everyone. Here it is possible
it won't be necessary but I can't be sure given both i_mmap_lock and
anon-vma lock are used in some many places. Some TPC comparison would
be nice before making a default switch IMHO.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-29 21:48                                   ` Andrea Arcangeli
@ 2008-02-29 22:12                                     ` Christoph Lameter
  2008-02-29 22:41                                       ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-02-29 22:12 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Fri, 29 Feb 2008, Andrea Arcangeli wrote:

> > AFAICT The rw semaphore fastpath is similar in performance to a rw 
> > spinlock. 
> 
> read side is taken in the slow path.

Slowpath meaning VM slowpath or lock slow path? Its seems that the rwsem 
read side path is pretty efficient:

static inline void __down_read(struct rw_semaphore *sem)
{
        __asm__ __volatile__(
                "# beginning down_read\n\t"
LOCK_PREFIX     "  incl      (%%eax)\n\t" /* adds 0x00000001, returns the old value */
                "  jns        1f\n"
                "  call call_rwsem_down_read_failed\n"
                "1:\n\t"
                "# ending down_read\n\t"
                : "+m" (sem->count)
                : "a" (sem)
                : "memory", "cc");
}



> 
> write side is taken in the fast path.
> 
> pagefault is fast path, VM during swapping is slow path.

Not sure what you are saying here. A pagefault should be considered as a 
fast path and swapping is not performance critical?

> > > Perhaps the rwlock spinlock can be changed to a rw semaphore without
> > > measurable overscheduling in the fast path. However theoretically
> > 
> > Overscheduling? You mean overhead?
> 
> The only possible overhead that a rw semaphore could ever generate vs
> a rw lock is overscheduling.

Ok too many calls to schedule() because the slow path (of the semaphore) 
is taken?

> > On the other hand a semaphore puts the process to sleep and may actually 
> > improve performance because there is less time spend in a busy loop. 
> > Other processes may do something useful and we stay off the contended 
> > cacheline reducing traffic on the interconnect.
> 
> Yes, that's the positive side, the negative side is that you'll put
> the task in uninterruptible sleep and call schedule() and require a
> wakeup, because a list_add taking <1usec is running in the
> other cpu. No other downside. But that's the only reason it's a
> spinlock right now, infact there can't be any other reason.

But that is only happening for the contended case. Certainly a spinlock is 
better for 2p system but the more processors content for the lock (and 
the longer the hold off is, typical for the processors with 4p or 8p or 
more) the better a semaphore will work.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-29 21:34                                 ` Christoph Lameter
@ 2008-02-29 21:48                                   ` Andrea Arcangeli
  2008-02-29 22:12                                     ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-29 21:48 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Fri, Feb 29, 2008 at 01:34:34PM -0800, Christoph Lameter wrote:
> On Fri, 29 Feb 2008, Andrea Arcangeli wrote:
> 
> > On Fri, Feb 29, 2008 at 01:03:16PM -0800, Christoph Lameter wrote:
> > > That means we need both the anon_vma locks and the i_mmap_lock to become 
> > > semaphores. I think semaphores are better than mutexes. Rik and Lee saw 
> > > some performance improvements because list can be traversed in parallel 
> > > when the anon_vma lock is switched to be a rw lock.
> > 
> > The improvement was with a rw spinlock IIRC, so I don't see how it's
> > related to this.
> 
> AFAICT The rw semaphore fastpath is similar in performance to a rw 
> spinlock. 

read side is taken in the slow path.

write side is taken in the fast path.

pagefault is fast path, VM during swapping is slow path.

> > Perhaps the rwlock spinlock can be changed to a rw semaphore without
> > measurable overscheduling in the fast path. However theoretically
> 
> Overscheduling? You mean overhead?

The only possible overhead that a rw semaphore could ever generate vs
a rw lock is overscheduling.

> > speaking the rw_lock spinlock is more efficient than a rw semaphore in
> > case of a little contention during the page fault fast path because
> > the critical section is just a list_add so it'd be overkill to
> > schedule while waiting. That's why currently it's a spinlock (or rw
> > spinlock).
> 
> On the other hand a semaphore puts the process to sleep and may actually 
> improve performance because there is less time spend in a busy loop. 
> Other processes may do something useful and we stay off the contended 
> cacheline reducing traffic on the interconnect.

Yes, that's the positive side, the negative side is that you'll put
the task in uninterruptible sleep and call schedule() and require a
wakeup, because a list_add taking <1usec is running in the
other cpu. No other downside. But that's the only reason it's a
spinlock right now, infact there can't be any other reason.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-29 21:23                               ` Andrea Arcangeli
  2008-02-29 21:29                                 ` Christoph Lameter
@ 2008-02-29 21:34                                 ` Christoph Lameter
  2008-02-29 21:48                                   ` Andrea Arcangeli
  1 sibling, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-02-29 21:34 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Fri, 29 Feb 2008, Andrea Arcangeli wrote:

> On Fri, Feb 29, 2008 at 01:03:16PM -0800, Christoph Lameter wrote:
> > That means we need both the anon_vma locks and the i_mmap_lock to become 
> > semaphores. I think semaphores are better than mutexes. Rik and Lee saw 
> > some performance improvements because list can be traversed in parallel 
> > when the anon_vma lock is switched to be a rw lock.
> 
> The improvement was with a rw spinlock IIRC, so I don't see how it's
> related to this.

AFAICT The rw semaphore fastpath is similar in performance to a rw 
spinlock. 
 
> Perhaps the rwlock spinlock can be changed to a rw semaphore without
> measurable overscheduling in the fast path. However theoretically

Overscheduling? You mean overhead?

> speaking the rw_lock spinlock is more efficient than a rw semaphore in
> case of a little contention during the page fault fast path because
> the critical section is just a list_add so it'd be overkill to
> schedule while waiting. That's why currently it's a spinlock (or rw
> spinlock).

On the other hand a semaphore puts the process to sleep and may actually 
improve performance because there is less time spend in a busy loop. 
Other processes may do something useful and we stay off the contended 
cacheline reducing traffic on the interconnect.
 
> preempt-rt runs quite a bit slower, or we could rip spinlocks out of
> the kernel in the first place ;)

The question is why that is the case and it seesm that there are issues 
with interrupt on/off that are important here and particularly significant 
with the SLAB allocator (significant hacks there to deal with that issue). 
The fastpath that we have in the works for SLUB may address a large 
part of that issue because it no longer relies on disabling interrupts.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-29 21:23                               ` Andrea Arcangeli
@ 2008-02-29 21:29                                 ` Christoph Lameter
  2008-02-29 21:34                                 ` Christoph Lameter
  1 sibling, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-02-29 21:29 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Fri, 29 Feb 2008, Andrea Arcangeli wrote:

> I don't have a strong opinion if it should become a semaphore
> unconditionally or only with a CONFIG_XPMEM=y. But keep in mind
> preempt-rt runs quite a bit slower, or we could rip spinlocks out of
> the kernel in the first place ;)

D you just skip comments of people on the mmu_notifier? It took me to 
remind you about Andrew's comments to note those. And I just responded on 
the XPmem issue in the morning.
 
Again for the gazillionth time: There will be no CONFIG_XPMEM because the 
functionality needs to be generic and not XPMEM specific.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-29 21:03                             ` Christoph Lameter
@ 2008-02-29 21:23                               ` Andrea Arcangeli
  2008-02-29 21:29                                 ` Christoph Lameter
  2008-02-29 21:34                                 ` Christoph Lameter
  0 siblings, 2 replies; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-29 21:23 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Fri, Feb 29, 2008 at 01:03:16PM -0800, Christoph Lameter wrote:
> That means we need both the anon_vma locks and the i_mmap_lock to become 
> semaphores. I think semaphores are better than mutexes. Rik and Lee saw 
> some performance improvements because list can be traversed in parallel 
> when the anon_vma lock is switched to be a rw lock.

The improvement was with a rw spinlock IIRC, so I don't see how it's
related to this.

Perhaps the rwlock spinlock can be changed to a rw semaphore without
measurable overscheduling in the fast path. However theoretically
speaking the rw_lock spinlock is more efficient than a rw semaphore in
case of a little contention during the page fault fast path because
the critical section is just a list_add so it'd be overkill to
schedule while waiting. That's why currently it's a spinlock (or rw
spinlock).

> Sounds like we get to a conceptually clean version here?

I don't have a strong opinion if it should become a semaphore
unconditionally or only with a CONFIG_XPMEM=y. But keep in mind
preempt-rt runs quite a bit slower, or we could rip spinlocks out of
the kernel in the first place ;)

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-29 20:17                           ` Andrea Arcangeli
@ 2008-02-29 21:03                             ` Christoph Lameter
  2008-02-29 21:23                               ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-02-29 21:03 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Fri, 29 Feb 2008, Andrea Arcangeli wrote:

> Agreed. I just thought xpmem needed an invalidate-by-page, but
> I'm glad if xpmem can go in sync with the KVM/GRU/DRI model in this
> regard.

That means we need both the anon_vma locks and the i_mmap_lock to become 
semaphores. I think semaphores are better than mutexes. Rik and Lee saw 
some performance improvements because list can be traversed in parallel 
when the anon_vma lock is switched to be a rw lock.

Sounds like we get to a conceptually clean version here?


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-29 19:55                         ` Christoph Lameter
@ 2008-02-29 20:17                           ` Andrea Arcangeli
  2008-02-29 21:03                             ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-29 20:17 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Fri, Feb 29, 2008 at 11:55:17AM -0800, Christoph Lameter wrote:
> >    post the invalidate in the mmio region of the device
> >    smp_call_function()
> >    while (mmio device wait-bitflag is on);
> 
> So the device driver on UP can only operate through interrupts? If you are 
> hogging the only cpu then driver operations may not be possible.

There was no irq involved in the above pseudocode, the irq if
something would run in the remote system. Still irqs can run fine
during the while loop like they run fine on top of
smp_call_function. The send-irq and the following spin-on-a-bitflag
works exactly as smp_call_function except this isn't a numa-CPU to
invalidate.

> And yes I would like to get rid of the mmu_rmap_notifiers altogether. It 
> would be much cleaner with just one mmu_notifier that can sleep in all 
> functions.

Agreed. I just thought xpmem needed an invalidate-by-page, but
I'm glad if xpmem can go in sync with the KVM/GRU/DRI model in this
regard.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-29 13:13                       ` Andrea Arcangeli
@ 2008-02-29 19:55                         ` Christoph Lameter
  2008-02-29 20:17                           ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-02-29 19:55 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Fri, 29 Feb 2008, Andrea Arcangeli wrote:

> On Thu, Feb 28, 2008 at 04:59:59PM -0800, Christoph Lameter wrote:
> > And thus the device driver may stop receiving data on a UP system? It will 
> > never get the ack.
> 
> Not sure to follow, sorry.
> 
> My idea was:
> 
>    post the invalidate in the mmio region of the device
>    smp_call_function()
>    while (mmio device wait-bitflag is on);

So the device driver on UP can only operate through interrupts? If you are 
hogging the only cpu then driver operations may not be possible.

> > invalidate_page_before/end could be realized as an 
> > invalidate_range_begin/end on a page sized range?
> 
> If we go this route, once you add support to xpmem, you'll have to
> make the anon_vma lock a mutex too, that would be fine with me
> though. The main reason invalidate_page exists, is to allow you to
> leave it as non-sleep-capable even after you make invalidate_range
> sleep capable, and to implement the mmu_rmap_notifiers sleep capable
> in all the paths that invalidate_page would be called. That was the
> strategy you had in your patch. I'll try to drop invalidate_page. I
> wonder if then you won't need the mmu_rmap_notifiers anymore.

I am mainly concerned with making the mmu notifier a generally useful 
feature for multiple users. Xpmem is one example of a different user. It 
should be considered as one example of a different type of callback user. 
It is not the gold standard that you make it to be. RDMA is another and 
there are likely scores of others (DMA engines etc) once it becomes clear 
that such a feature is available. In general the mmu notifier will allows 
us to fix the problems caused by memory pinning and mlock by various 
devices and other mechanisms that need to directly access memory. 

And yes I would like to get rid of the mmu_rmap_notifiers altogether. It 
would be much cleaner with just one mmu_notifier that can sleep in all 
functions.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-29  0:59                     ` Christoph Lameter
@ 2008-02-29 13:13                       ` Andrea Arcangeli
  2008-02-29 19:55                         ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-29 13:13 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Thu, Feb 28, 2008 at 04:59:59PM -0800, Christoph Lameter wrote:
> And thus the device driver may stop receiving data on a UP system? It will 
> never get the ack.

Not sure to follow, sorry.

My idea was:

   post the invalidate in the mmio region of the device
   smp_call_function()
   while (mmio device wait-bitflag is on);

Instead of the current:

   smp_call_function()
   post the invalidate in the mmio region of the device
   while (mmio device wait-bitflag is on);

To decrease the wait loop time.

> invalidate_page_before/end could be realized as an 
> invalidate_range_begin/end on a page sized range?

If we go this route, once you add support to xpmem, you'll have to
make the anon_vma lock a mutex too, that would be fine with me
though. The main reason invalidate_page exists, is to allow you to
leave it as non-sleep-capable even after you make invalidate_range
sleep capable, and to implement the mmu_rmap_notifiers sleep capable
in all the paths that invalidate_page would be called. That was the
strategy you had in your patch. I'll try to drop invalidate_page. I
wonder if then you won't need the mmu_rmap_notifiers anymore.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-29  0:55                   ` Andrea Arcangeli
@ 2008-02-29  0:59                     ` Christoph Lameter
  2008-02-29 13:13                       ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-02-29  0:59 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Fri, 29 Feb 2008, Andrea Arcangeli wrote:

> On Thu, Feb 28, 2008 at 10:43:54AM -0800, Christoph Lameter wrote:
> > What about invalidate_page()?
> 
> That would just spin waiting an ack (just like the smp-tlb-flushing
> invalidates in numa already does).

And thus the device driver may stop receiving data on a UP system? It will 
never get the ack.
 
> Thinking more about this, we could also parallelize it with an
> invalidate_page_before/end. If it takes 1usec to flush remotely,
> scheduling would be overkill, but spending 1usec in a while loop isn't
> nice if we can parallelize that 1usec with the ipi-tlb-flush. Not sure
> if it makes sense... it certainly would be quick to add it (especially
> thanks to _notify ;).

invalidate_page_before/end could be realized as an 
invalidate_range_begin/end on a page sized range?

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-28 18:43                 ` Christoph Lameter
@ 2008-02-29  0:55                   ` Andrea Arcangeli
  2008-02-29  0:59                     ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-29  0:55 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Thu, Feb 28, 2008 at 10:43:54AM -0800, Christoph Lameter wrote:
> What about invalidate_page()?

That would just spin waiting an ack (just like the smp-tlb-flushing
invalidates in numa already does).

Thinking more about this, we could also parallelize it with an
invalidate_page_before/end. If it takes 1usec to flush remotely,
scheduling would be overkill, but spending 1usec in a while loop isn't
nice if we can parallelize that 1usec with the ipi-tlb-flush. Not sure
if it makes sense... it certainly would be quick to add it (especially
thanks to _notify ;).

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-28  1:10               ` Andrea Arcangeli
@ 2008-02-28 18:43                 ` Christoph Lameter
  2008-02-29  0:55                   ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-02-28 18:43 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Thu, 28 Feb 2008, Andrea Arcangeli wrote:

> On Wed, Feb 27, 2008 at 05:03:21PM -0800, Christoph Lameter wrote:
> > RDMA works across a network and I would assume that it needs confirmation 
> > that a connection has been torn down before pages can be unmapped.
> 
> Depends on the latency of the network, for example with page pinning
> it can even try to reduce the wait time, by tearing down the mapping
> in range_begin and spin waiting the ack only later in range_end.

What about invalidate_page()?
 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-28  0:52           ` Andrea Arcangeli
  2008-02-28  1:03             ` Christoph Lameter
@ 2008-02-28 10:53             ` Robin Holt
  1 sibling, 0 replies; 101+ messages in thread
From: Robin Holt @ 2008-02-28 10:53 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Christoph Lameter, Nick Piggin, akpm, Robin Holt, Avi Kivity,
	Izik Eidus, kvm-devel, Peter Zijlstra, general, Steve Wise,
	Roland Dreier, Kanoj Sarcar, steiner, linux-kernel, linux-mm,
	daniel.blueman

On Thu, Feb 28, 2008 at 01:52:50AM +0100, Andrea Arcangeli wrote:
> On Wed, Feb 27, 2008 at 04:14:08PM -0800, Christoph Lameter wrote:
> > Erm. This would also be needed by RDMA etc.
> 
> The only RDMA I know is Quadrics, and Quadrics apparently doesn't need
> to schedule inside the invalidate methods AFIK, so I doubt the above
> is true. It'd be interesting to know if IB is like Quadrics and it
> also doesn't require blocking to invalidate certain remote mappings.

We got an answer from the IB guys already.  They do not track which of
their handles are being used by remote processes so neither approach
will work for their purposes with the exception of straight unmaps.  In
that case, they could use the callout to remove TLB information and rely
on the lack of page table information to kill the users process.
Without changes to their library spec, I don't believe anything further
is possible.  If they did change their library spec, I believe they
could get things to work the same way that XPMEM has gotten things to
work, where a message is sent to the remote side for TLB clearing and
that will require sleeping.

Thanks,
Robin

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-28  1:03             ` Christoph Lameter
@ 2008-02-28  1:10               ` Andrea Arcangeli
  2008-02-28 18:43                 ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-28  1:10 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Wed, Feb 27, 2008 at 05:03:21PM -0800, Christoph Lameter wrote:
> RDMA works across a network and I would assume that it needs confirmation 
> that a connection has been torn down before pages can be unmapped.

Depends on the latency of the network, for example with page pinning
it can even try to reduce the wait time, by tearing down the mapping
in range_begin and spin waiting the ack only later in range_end.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-28  0:52           ` Andrea Arcangeli
@ 2008-02-28  1:03             ` Christoph Lameter
  2008-02-28  1:10               ` Andrea Arcangeli
  2008-02-28 10:53             ` Robin Holt
  1 sibling, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-02-28  1:03 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Thu, 28 Feb 2008, Andrea Arcangeli wrote:

> On Wed, Feb 27, 2008 at 04:14:08PM -0800, Christoph Lameter wrote:
> > Erm. This would also be needed by RDMA etc.
> 
> The only RDMA I know is Quadrics, and Quadrics apparently doesn't need
> to schedule inside the invalidate methods AFIK, so I doubt the above
> is true. It'd be interesting to know if IB is like Quadrics and it
> also doesn't require blocking to invalidate certain remote mappings.

RDMA works across a network and I would assume that it needs confirmation 
that a connection has been torn down before pages can be unmapped.
 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-28  0:14         ` Christoph Lameter
@ 2008-02-28  0:52           ` Andrea Arcangeli
  2008-02-28  1:03             ` Christoph Lameter
  2008-02-28 10:53             ` Robin Holt
  0 siblings, 2 replies; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-28  0:52 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Wed, Feb 27, 2008 at 04:14:08PM -0800, Christoph Lameter wrote:
> Erm. This would also be needed by RDMA etc.

The only RDMA I know is Quadrics, and Quadrics apparently doesn't need
to schedule inside the invalidate methods AFIK, so I doubt the above
is true. It'd be interesting to know if IB is like Quadrics and it
also doesn't require blocking to invalidate certain remote mappings.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-27 22:39       ` Christoph Lameter
@ 2008-02-28  0:38         ` Andrea Arcangeli
  0 siblings, 0 replies; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-28  0:38 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Wed, Feb 27, 2008 at 02:39:46PM -0800, Christoph Lameter wrote:
> On Wed, 20 Feb 2008, Andrea Arcangeli wrote:
> 
> > Well, xpmem requirements are complex. As as side effect of the
> > simplicity of my approach, my patch is 100% safe since #v1. Now it
> > also works for GRU and it cluster invalidates.
> 
> The patch has to satisfy RDMA, XPMEM, GRU and KVM. I keep hearing that we 
> have a KVM only solution that works 100% (which makes me just switch 
> ignore the rest of the argument because 100% solutions usually do not 
> exist).

I only said 100% safe, I didn't imply anything other than it won't
crash the kernel ;).

#v6 and #v7 only leaves XPMEM out AFIK, and that can be supported
later with a CONFIG_XPMEM that purely changes some VM locking. #v7
also provides maximum performance to GRU.

> > rcu_read_lock), no "atomic" parameters, and it doesn't open a window
> > where sptes have a view on older pages and linux pte has view on newer
> > pages (this can happen with remap_file_pages with my KVM swapping
> > patch to use V8 Christoph's patch).
> 
> Ok so you are now getting away from keeping the refcount elevated? That 
> was your design decision....

No, I'm not getting away from it. If I would get away from it, I would
be forced to implement invalidate_range_begin. However even if I don't
get away from it, the fact I only implement invalidate_range_end, and
that's called after the PT lock is dropped, opens a little window with
lost coherency (which may not be detectable by userland anyway). But this
little window is fine for KVM and it doesn't impose any security
risk. But clearly proving the locking safe becomes a bit more complex
in #v7 than in #v6.

> It would have helped if you would have repeated my answers that you had 
> already gotten before. You knew I was on vacation....

I didn't remember the BUG_ON crystal clear sorry, but not sure why you
think it was your call, this was a lowlevel XPMEM question and Robin
promptly answered/reminded about it infact.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-28  0:11       ` Andrea Arcangeli
@ 2008-02-28  0:14         ` Christoph Lameter
  2008-02-28  0:52           ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-02-28  0:14 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Thu, 28 Feb 2008, Andrea Arcangeli wrote:

> > 3. Keep the refcount elevated until pages are freed in another execution 
> > context.
> 
> Page refcount is not enough (the mmu_notifier_release will run in
> another cpu the moment after i_mmap_lock is unlocked) but mm_users may
> prevent us to change the i_mmap_lock to a mutex, but it'll slowdown
> truncate as it'll have to drop the lock and restart the radix tree
> walk every time so a change like this better fits as a separate
> CONFIG_XPMEM IMHO.

Erm. This would also be needed by RDMA etc.

 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-27 22:35     ` Christoph Lameter
  2008-02-27 22:42       ` Jack Steiner
  2008-02-28  0:10       ` Christoph Lameter
@ 2008-02-28  0:11       ` Andrea Arcangeli
  2008-02-28  0:14         ` Christoph Lameter
  2008-03-03  5:11       ` Nick Piggin
  3 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-28  0:11 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Wed, Feb 27, 2008 at 02:35:59PM -0800, Christoph Lameter wrote:
> Could you be specific? This refers to page migration? Hmmm... Guess we 

If the reader schedule, the synchronize_rcu will return in the other
cpu and the objects in the list will be freed and overwritten, and
when the task is scheduled back in, it'll follow dangling pointers...
You can't use RCU if you want any of your invalidate methods to
schedule. Otherwise it's like having zero locking.

> 2. Not handle file backed mappings (XPmem could work mostly in such a 
> config)

IMHO that fits under your definition of "hacking something in now and
then having to modify it later".

> 3. Keep the refcount elevated until pages are freed in another execution 
> context.

Page refcount is not enough (the mmu_notifier_release will run in
another cpu the moment after i_mmap_lock is unlocked) but mm_users may
prevent us to change the i_mmap_lock to a mutex, but it'll slowdown
truncate as it'll have to drop the lock and restart the radix tree
walk every time so a change like this better fits as a separate
CONFIG_XPMEM IMHO.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-27 22:35     ` Christoph Lameter
  2008-02-27 22:42       ` Jack Steiner
@ 2008-02-28  0:10       ` Christoph Lameter
  2008-02-28  0:11       ` Andrea Arcangeli
  2008-03-03  5:11       ` Nick Piggin
  3 siblings, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-02-28  0:10 UTC (permalink / raw)
  To: Nick Piggin
  Cc: akpm, Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	kvm-devel, Peter Zijlstra, general, Steve Wise, Roland Dreier,
	Kanoj Sarcar, steiner, linux-kernel, linux-mm, daniel.blueman

On Wed, 27 Feb 2008, Christoph Lameter wrote:

> Could you be specific? This refers to page migration? Hmmm... Guess we 
> would need to inc the refcount there instead?

Argh. No its the callback list scanning. Yuck. No one noticed.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-27 22:23       ` Christoph Lameter
@ 2008-02-27 23:57         ` Andrea Arcangeli
  0 siblings, 0 replies; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-27 23:57 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Wed, Feb 27, 2008 at 02:23:29PM -0800, Christoph Lameter wrote:
> How would that work? You rely on the pte locking. Thus calls are all in an 

I don't rely on the pte locking in #v7, exactly to satisfy GRU
(so far purely theoretical) performance complains.

> atomic context. I think we need a general scheme that allows sleeping when 

Calls are still in atomic context until we change the i_mmap_lock to a
mutex under a CONFIG_XPMEM, or unless we boost mm_users, drop the lock
and restart the loop at every different mm. In any case those changes
should be under CONFIG_XPMEM IMHO given desktop users definitely don't
need this (regular non-blocking mmu notifiers in my patch are all what
a desktop user need as far as I can tell).

> references are invalidates. Even the GRU has performance issues when using 
> the KVM patch.

GRU will perform the same with #v7 or V8.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-27 22:35     ` Christoph Lameter
@ 2008-02-27 22:42       ` Jack Steiner
  2008-02-28  0:10       ` Christoph Lameter
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 101+ messages in thread
From: Jack Steiner @ 2008-02-27 22:42 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Nick Piggin, akpm, Andrea Arcangeli, Robin Holt, Avi Kivity,
	Izik Eidus, kvm-devel, Peter Zijlstra, general, Steve Wise,
	Roland Dreier, Kanoj Sarcar, linux-kernel, linux-mm,
	daniel.blueman

>  
> > Also, what we are going to need here are not skeleton drivers
> > that just do all the *easy* bits (of registering their callbacks),
> > but actual fully working examples that do everything that any
> > real driver will need to do. If not for the sanity of the driver
> > writer, then for the sanity of the VM developers (I don't want
> > to have to understand xpmem or infiniband in order to understand
> > how the VM works).
> 
> There are 3 different drivers that can already use it but the code is 
> complex and not easy to review. Skeletons are easy to allow people to get 
> started with it.


I posted the full GRU driver late last week. It is a lot of
code & somewhat difficult to understand w/o access to full chip
specs (sorry). The code is fairly well commented &  the
parts related to TLB management should be understandable.



^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-20  1:00     ` Andrea Arcangeli
  2008-02-20  3:00       ` Robin Holt
@ 2008-02-27 22:39       ` Christoph Lameter
  2008-02-28  0:38         ` Andrea Arcangeli
  1 sibling, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-02-27 22:39 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Wed, 20 Feb 2008, Andrea Arcangeli wrote:

> Well, xpmem requirements are complex. As as side effect of the
> simplicity of my approach, my patch is 100% safe since #v1. Now it
> also works for GRU and it cluster invalidates.

The patch has to satisfy RDMA, XPMEM, GRU and KVM. I keep hearing that we 
have a KVM only solution that works 100% (which makes me just switch 
ignore the rest of the argument because 100% solutions usually do not 
exist).


> rcu_read_lock), no "atomic" parameters, and it doesn't open a window
> where sptes have a view on older pages and linux pte has view on newer
> pages (this can happen with remap_file_pages with my KVM swapping
> patch to use V8 Christoph's patch).

Ok so you are now getting away from keeping the refcount elevated? That 
was your design decision....


> > Also, how to you resolve the case where you are not allowed to sleep?
> > I would have thought either you have to handle it, in which case nobody
> > needs to sleep; or you can't handle it, in which case the code is
> > broken.
> 
> I also asked exactly this, glad you reasked this too.

It would have helped if you would have repeated my answers that you had 
already gotten before. You knew I was on vacation....


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-19 23:08   ` Nick Piggin
  2008-02-20  1:00     ` Andrea Arcangeli
@ 2008-02-27 22:35     ` Christoph Lameter
  2008-02-27 22:42       ` Jack Steiner
                         ` (3 more replies)
  1 sibling, 4 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-02-27 22:35 UTC (permalink / raw)
  To: Nick Piggin
  Cc: akpm, Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	kvm-devel, Peter Zijlstra, general, Steve Wise, Roland Dreier,
	Kanoj Sarcar, steiner, linux-kernel, linux-mm, daniel.blueman

On Wed, 20 Feb 2008, Nick Piggin wrote:

> On Friday 15 February 2008 17:49, Christoph Lameter wrote:
> > The invalidation of address ranges in a mm_struct needs to be
> > performed when pages are removed or permissions etc change.
> >
> > If invalidate_range_begin() is called with locks held then we
> > pass a flag into invalidate_range() to indicate that no sleeping is
> > possible. Locks are only held for truncate and huge pages.
> 
> You can't sleep inside rcu_read_lock()!

Could you be specific? This refers to page migration? Hmmm... Guess we 
would need to inc the refcount there instead?

> I must say that for a patch that is up to v8 or whatever and is
> posted twice a week to such a big cc list, it is kind of slack to
> not even test it and expect other people to review it.

It was tested with the GRU and XPmem. Andrea also reported success.
 
> Also, what we are going to need here are not skeleton drivers
> that just do all the *easy* bits (of registering their callbacks),
> but actual fully working examples that do everything that any
> real driver will need to do. If not for the sanity of the driver
> writer, then for the sanity of the VM developers (I don't want
> to have to understand xpmem or infiniband in order to understand
> how the VM works).

There are 3 different drivers that can already use it but the code is 
complex and not easy to review. Skeletons are easy to allow people to get 
started with it.

> >  	lru_add_drain();
> >  	tlb = tlb_gather_mmu(mm, 0);
> >  	update_hiwater_rss(mm);
> > +	mmu_notifier(invalidate_range_begin, mm, address, end, atomic);
> >  	end = unmap_vmas(&tlb, vma, address, end, &nr_accounted, details);
> >  	if (tlb)
> >  		tlb_finish_mmu(tlb, address, end);
> > +	mmu_notifier(invalidate_range_end, mm, address, end, atomic);
> >  	return end;
> >  }
> >
> 
> Where do you invalidate for munmap()?

zap_page_range() called from unmap_vmas().

> Also, how to you resolve the case where you are not allowed to sleep?
> I would have thought either you have to handle it, in which case nobody
> needs to sleep; or you can't handle it, in which case the code is
> broken.

That can be done in a variety of ways:

1. Change VM locking

2. Not handle file backed mappings (XPmem could work mostly in such a 
config)

3. Keep the refcount elevated until pages are freed in another execution 
context.



^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-19 13:34     ` Andrea Arcangeli
@ 2008-02-27 22:23       ` Christoph Lameter
  2008-02-27 23:57         ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-02-27 22:23 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, akpm, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Tue, 19 Feb 2008, Andrea Arcangeli wrote:

> Yes, that's why I kept maintaining my patch and I posted the last
> revision to Andrew. I use pte/tlb locking of the core VM, it's
> unintrusive and obviously safe. Furthermore it can be extended with
> Christoph's stuff in a 100% backwards compatible fashion later if needed.

How would that work? You rely on the pte locking. Thus calls are all in an 
atomic context. I think we need a general scheme that allows sleeping when 
references are invalidates. Even the GRU has performance issues when using 
the KVM patch.


 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-20  3:11         ` Nick Piggin
@ 2008-02-20  3:19           ` Robin Holt
  0 siblings, 0 replies; 101+ messages in thread
From: Robin Holt @ 2008-02-20  3:19 UTC (permalink / raw)
  To: Nick Piggin
  Cc: Robin Holt, Andrea Arcangeli, Christoph Lameter, akpm,
	Avi Kivity, Izik Eidus, kvm-devel, Peter Zijlstra, general,
	Steve Wise, Roland Dreier, Kanoj Sarcar, steiner, linux-kernel,
	linux-mm, daniel.blueman

On Wed, Feb 20, 2008 at 02:11:41PM +1100, Nick Piggin wrote:
> On Wednesday 20 February 2008 14:00, Robin Holt wrote:
> > On Wed, Feb 20, 2008 at 02:00:38AM +0100, Andrea Arcangeli wrote:
> > > On Wed, Feb 20, 2008 at 10:08:49AM +1100, Nick Piggin wrote:
> 
> > > > Also, how to you resolve the case where you are not allowed to sleep?
> > > > I would have thought either you have to handle it, in which case nobody
> > > > needs to sleep; or you can't handle it, in which case the code is
> > > > broken.
> > >
> > > I also asked exactly this, glad you reasked this too.
> >
> > Currently, we BUG_ON having a PFN in our tables and not being able
> > to sleep.  These are mappings which MPT has never supported in the past
> > and XPMEM was already not allowing page faults for VMAs which are not
> > anonymous so it should never happen.  If the file-backed operations can
> > ever get changed to allow for sleeping and a customer has a need for it,
> > we would need to change XPMEM to allow those types of faults to succeed.
> 
> Do you really want to be able to swap, or are you just interested
> in keeping track of unmaps / prot changes?

I would rather not swap, but we do have one customer that would like
swapout to work for certain circumstances.  Additionally, we have
many customers that would rather that their system not die under I/O
termination.

Thanks,
Robin

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-20  3:00       ` Robin Holt
@ 2008-02-20  3:11         ` Nick Piggin
  2008-02-20  3:19           ` Robin Holt
  0 siblings, 1 reply; 101+ messages in thread
From: Nick Piggin @ 2008-02-20  3:11 UTC (permalink / raw)
  To: Robin Holt
  Cc: Andrea Arcangeli, Christoph Lameter, akpm, Avi Kivity,
	Izik Eidus, kvm-devel, Peter Zijlstra, general, Steve Wise,
	Roland Dreier, Kanoj Sarcar, steiner, linux-kernel, linux-mm,
	daniel.blueman

On Wednesday 20 February 2008 14:00, Robin Holt wrote:
> On Wed, Feb 20, 2008 at 02:00:38AM +0100, Andrea Arcangeli wrote:
> > On Wed, Feb 20, 2008 at 10:08:49AM +1100, Nick Piggin wrote:

> > > Also, how to you resolve the case where you are not allowed to sleep?
> > > I would have thought either you have to handle it, in which case nobody
> > > needs to sleep; or you can't handle it, in which case the code is
> > > broken.
> >
> > I also asked exactly this, glad you reasked this too.
>
> Currently, we BUG_ON having a PFN in our tables and not being able
> to sleep.  These are mappings which MPT has never supported in the past
> and XPMEM was already not allowing page faults for VMAs which are not
> anonymous so it should never happen.  If the file-backed operations can
> ever get changed to allow for sleeping and a customer has a need for it,
> we would need to change XPMEM to allow those types of faults to succeed.

Do you really want to be able to swap, or are you just interested
in keeping track of unmaps / prot changes?


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-20  1:00     ` Andrea Arcangeli
@ 2008-02-20  3:00       ` Robin Holt
  2008-02-20  3:11         ` Nick Piggin
  2008-02-27 22:39       ` Christoph Lameter
  1 sibling, 1 reply; 101+ messages in thread
From: Robin Holt @ 2008-02-20  3:00 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Nick Piggin, Christoph Lameter, akpm, Robin Holt, Avi Kivity,
	Izik Eidus, kvm-devel, Peter Zijlstra, general, Steve Wise,
	Roland Dreier, Kanoj Sarcar, steiner, linux-kernel, linux-mm,
	daniel.blueman

On Wed, Feb 20, 2008 at 02:00:38AM +0100, Andrea Arcangeli wrote:
> On Wed, Feb 20, 2008 at 10:08:49AM +1100, Nick Piggin wrote:
> > You can't sleep inside rcu_read_lock()!
> > 
> > I must say that for a patch that is up to v8 or whatever and is
> > posted twice a week to such a big cc list, it is kind of slack to
> > not even test it and expect other people to review it.
> 
> Well, xpmem requirements are complex. As as side effect of the
> simplicity of my approach, my patch is 100% safe since #v1. Now it
> also works for GRU and it cluster invalidates.
> 
> > Also, what we are going to need here are not skeleton drivers
> > that just do all the *easy* bits (of registering their callbacks),
> > but actual fully working examples that do everything that any
> > real driver will need to do. If not for the sanity of the driver
> 
> I've a fully working scenario for my patch, infact I didn't post the
> mmu notifier patch until I got KVM to swap 100% reliably to be sure I
> would post something that works well. mmu notifiers are already used
> in KVM for:
> 
> 1) 100% reliable and efficient swapping of guest physical memory
> 2) copy-on-writes of writeprotect faults after ksm page sharing of guest
>    physical memory
> 3) ballooning using madvise to give the guest memory back to the host
> 
> My implementation is the most handy because it requires zero changes
> to the ksm code too (no explicit mmu notifier calls after
> ptep_clear_flush) and it's also 100% safe (no mess with schedules over
> rcu_read_lock), no "atomic" parameters, and it doesn't open a window
> where sptes have a view on older pages and linux pte has view on newer
> pages (this can happen with remap_file_pages with my KVM swapping
> patch to use V8 Christoph's patch).
> 
> > Also, how to you resolve the case where you are not allowed to sleep?
> > I would have thought either you have to handle it, in which case nobody
> > needs to sleep; or you can't handle it, in which case the code is
> > broken.
> 
> I also asked exactly this, glad you reasked this too.

Currently, we BUG_ON having a PFN in our tables and not being able
to sleep.  These are mappings which MPT has never supported in the past
and XPMEM was already not allowing page faults for VMAs which are not
anonymous so it should never happen.  If the file-backed operations can
ever get changed to allow for sleeping and a customer has a need for it,
we would need to change XPMEM to allow those types of faults to succeed.

Thanks,
Robin

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-19 23:08   ` Nick Piggin
@ 2008-02-20  1:00     ` Andrea Arcangeli
  2008-02-20  3:00       ` Robin Holt
  2008-02-27 22:39       ` Christoph Lameter
  2008-02-27 22:35     ` Christoph Lameter
  1 sibling, 2 replies; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-20  1:00 UTC (permalink / raw)
  To: Nick Piggin
  Cc: Christoph Lameter, akpm, Robin Holt, Avi Kivity, Izik Eidus,
	kvm-devel, Peter Zijlstra, general, Steve Wise, Roland Dreier,
	Kanoj Sarcar, steiner, linux-kernel, linux-mm, daniel.blueman

On Wed, Feb 20, 2008 at 10:08:49AM +1100, Nick Piggin wrote:
> You can't sleep inside rcu_read_lock()!
> 
> I must say that for a patch that is up to v8 or whatever and is
> posted twice a week to such a big cc list, it is kind of slack to
> not even test it and expect other people to review it.

Well, xpmem requirements are complex. As as side effect of the
simplicity of my approach, my patch is 100% safe since #v1. Now it
also works for GRU and it cluster invalidates.

> Also, what we are going to need here are not skeleton drivers
> that just do all the *easy* bits (of registering their callbacks),
> but actual fully working examples that do everything that any
> real driver will need to do. If not for the sanity of the driver

I've a fully working scenario for my patch, infact I didn't post the
mmu notifier patch until I got KVM to swap 100% reliably to be sure I
would post something that works well. mmu notifiers are already used
in KVM for:

1) 100% reliable and efficient swapping of guest physical memory
2) copy-on-writes of writeprotect faults after ksm page sharing of guest
   physical memory
3) ballooning using madvise to give the guest memory back to the host

My implementation is the most handy because it requires zero changes
to the ksm code too (no explicit mmu notifier calls after
ptep_clear_flush) and it's also 100% safe (no mess with schedules over
rcu_read_lock), no "atomic" parameters, and it doesn't open a window
where sptes have a view on older pages and linux pte has view on newer
pages (this can happen with remap_file_pages with my KVM swapping
patch to use V8 Christoph's patch).

> Also, how to you resolve the case where you are not allowed to sleep?
> I would have thought either you have to handle it, in which case nobody
> needs to sleep; or you can't handle it, in which case the code is
> broken.

I also asked exactly this, glad you reasked this too.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-15  6:49 ` [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges Christoph Lameter
  2008-02-16  3:37   ` Andrew Morton
  2008-02-19  8:54   ` Nick Piggin
@ 2008-02-19 23:08   ` Nick Piggin
  2008-02-20  1:00     ` Andrea Arcangeli
  2008-02-27 22:35     ` Christoph Lameter
  2 siblings, 2 replies; 101+ messages in thread
From: Nick Piggin @ 2008-02-19 23:08 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: akpm, Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	kvm-devel, Peter Zijlstra, general, Steve Wise, Roland Dreier,
	Kanoj Sarcar, steiner, linux-kernel, linux-mm, daniel.blueman

On Friday 15 February 2008 17:49, Christoph Lameter wrote:
> The invalidation of address ranges in a mm_struct needs to be
> performed when pages are removed or permissions etc change.
>
> If invalidate_range_begin() is called with locks held then we
> pass a flag into invalidate_range() to indicate that no sleeping is
> possible. Locks are only held for truncate and huge pages.

You can't sleep inside rcu_read_lock()!

I must say that for a patch that is up to v8 or whatever and is
posted twice a week to such a big cc list, it is kind of slack to
not even test it and expect other people to review it.

Also, what we are going to need here are not skeleton drivers
that just do all the *easy* bits (of registering their callbacks),
but actual fully working examples that do everything that any
real driver will need to do. If not for the sanity of the driver
writer, then for the sanity of the VM developers (I don't want
to have to understand xpmem or infiniband in order to understand
how the VM works).



> In two cases we use invalidate_range_begin/end to invalidate
> single pages because the pair allows holding off new references
> (idea by Robin Holt).
>
> do_wp_page(): We hold off new references while we update the pte.
>
> xip_unmap: We are not taking the PageLock so we cannot
> use the invalidate_page mmu_rmap_notifier. invalidate_range_begin/end
> stands in.
>
> Signed-off-by: Andrea Arcangeli <andrea@qumranet.com>
> Signed-off-by: Robin Holt <holt@sgi.com>
> Signed-off-by: Christoph Lameter <clameter@sgi.com>
>
> ---
>  mm/filemap_xip.c |    5 +++++
>  mm/fremap.c      |    3 +++
>  mm/hugetlb.c     |    3 +++
>  mm/memory.c      |   35 +++++++++++++++++++++++++++++------
>  mm/mmap.c        |    2 ++
>  mm/mprotect.c    |    3 +++
>  mm/mremap.c      |    7 ++++++-
>  7 files changed, 51 insertions(+), 7 deletions(-)
>
> Index: linux-2.6/mm/fremap.c
> ===================================================================
> --- linux-2.6.orig/mm/fremap.c	2008-02-14 18:43:31.000000000 -0800
> +++ linux-2.6/mm/fremap.c	2008-02-14 18:45:07.000000000 -0800
> @@ -15,6 +15,7 @@
>  #include <linux/rmap.h>
>  #include <linux/module.h>
>  #include <linux/syscalls.h>
> +#include <linux/mmu_notifier.h>
>
>  #include <asm/mmu_context.h>
>  #include <asm/cacheflush.h>
> @@ -214,7 +215,9 @@ asmlinkage long sys_remap_file_pages(uns
>  		spin_unlock(&mapping->i_mmap_lock);
>  	}
>
> +	mmu_notifier(invalidate_range_begin, mm, start, start + size, 0);
>  	err = populate_range(mm, vma, start, size, pgoff);
> +	mmu_notifier(invalidate_range_end, mm, start, start + size, 0);
>  	if (!err && !(flags & MAP_NONBLOCK)) {
>  		if (unlikely(has_write_lock)) {
>  			downgrade_write(&mm->mmap_sem);
> Index: linux-2.6/mm/memory.c
> ===================================================================
> --- linux-2.6.orig/mm/memory.c	2008-02-14 18:43:31.000000000 -0800
> +++ linux-2.6/mm/memory.c	2008-02-14 18:45:07.000000000 -0800
> @@ -51,6 +51,7 @@
>  #include <linux/init.h>
>  #include <linux/writeback.h>
>  #include <linux/memcontrol.h>
> +#include <linux/mmu_notifier.h>
>
>  #include <asm/pgalloc.h>
>  #include <asm/uaccess.h>
> @@ -611,6 +612,9 @@ int copy_page_range(struct mm_struct *ds
>  	if (is_vm_hugetlb_page(vma))
>  		return copy_hugetlb_page_range(dst_mm, src_mm, vma);
>
> +	if (is_cow_mapping(vma->vm_flags))
> +		mmu_notifier(invalidate_range_begin, src_mm, addr, end, 0);
> +
>  	dst_pgd = pgd_offset(dst_mm, addr);
>  	src_pgd = pgd_offset(src_mm, addr);
>  	do {
> @@ -621,6 +625,11 @@ int copy_page_range(struct mm_struct *ds
>  						vma, addr, next))
>  			return -ENOMEM;
>  	} while (dst_pgd++, src_pgd++, addr = next, addr != end);
> +
> +	if (is_cow_mapping(vma->vm_flags))
> +		mmu_notifier(invalidate_range_end, src_mm,
> +						vma->vm_start, end, 0);
> +
>  	return 0;
>  }
>
> @@ -893,13 +902,16 @@ unsigned long zap_page_range(struct vm_a
>  	struct mmu_gather *tlb;
>  	unsigned long end = address + size;
>  	unsigned long nr_accounted = 0;
> +	int atomic = details ? (details->i_mmap_lock != 0) : 0;
>
>  	lru_add_drain();
>  	tlb = tlb_gather_mmu(mm, 0);
>  	update_hiwater_rss(mm);
> +	mmu_notifier(invalidate_range_begin, mm, address, end, atomic);
>  	end = unmap_vmas(&tlb, vma, address, end, &nr_accounted, details);
>  	if (tlb)
>  		tlb_finish_mmu(tlb, address, end);
> +	mmu_notifier(invalidate_range_end, mm, address, end, atomic);
>  	return end;
>  }
>

Where do you invalidate for munmap()?

Also, how to you resolve the case where you are not allowed to sleep?
I would have thought either you have to handle it, in which case nobody
needs to sleep; or you can't handle it, in which case the code is
broken.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-19  8:54   ` Nick Piggin
@ 2008-02-19 13:34     ` Andrea Arcangeli
  2008-02-27 22:23       ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-02-19 13:34 UTC (permalink / raw)
  To: Nick Piggin
  Cc: Christoph Lameter, akpm, Robin Holt, Avi Kivity, Izik Eidus,
	kvm-devel, Peter Zijlstra, general, Steve Wise, Roland Dreier,
	Kanoj Sarcar, steiner, linux-kernel, linux-mm, daniel.blueman

On Tue, Feb 19, 2008 at 07:54:14PM +1100, Nick Piggin wrote:
> As far as sleeping inside callbacks goes... I think there are big
> problems with the patch (the sleeping patch and the external rmap
> patch). I don't think it is workable in its current state. Either
> we have to make some big changes to the core VM, or we have to turn
> some locks into sleeping locks to do it properly AFAIKS. Neither
> one is good.

Agreed.

The thing is quite simple, the moment we support xpmem the complexity
in the mmu notifier patch start and there are hacks, duplicated
functionality through the same xpmem callbacks etc... GRU can already
be 100% supported (infact simpler and safer) with my patch.

> But anyway, I don't really think the two approaches (Andrea's
> notifiers vs sleeping/xrmap) should be tangled up too much. I
> think Andrea's can possibly be quite unintrusive and useful very
> soon.

Yes, that's why I kept maintaining my patch and I posted the last
revision to Andrew. I use pte/tlb locking of the core VM, it's
unintrusive and obviously safe. Furthermore it can be extended with
Christoph's stuff in a 100% backwards compatible fashion later if needed.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-15  6:49 ` [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges Christoph Lameter
  2008-02-16  3:37   ` Andrew Morton
@ 2008-02-19  8:54   ` Nick Piggin
  2008-02-19 13:34     ` Andrea Arcangeli
  2008-02-19 23:08   ` Nick Piggin
  2 siblings, 1 reply; 101+ messages in thread
From: Nick Piggin @ 2008-02-19  8:54 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: akpm, Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	kvm-devel, Peter Zijlstra, general, Steve Wise, Roland Dreier,
	Kanoj Sarcar, steiner, linux-kernel, linux-mm, daniel.blueman

On Friday 15 February 2008 17:49, Christoph Lameter wrote:
> The invalidation of address ranges in a mm_struct needs to be
> performed when pages are removed or permissions etc change.
>
> If invalidate_range_begin() is called with locks held then we
> pass a flag into invalidate_range() to indicate that no sleeping is
> possible. Locks are only held for truncate and huge pages.
>
> In two cases we use invalidate_range_begin/end to invalidate
> single pages because the pair allows holding off new references
> (idea by Robin Holt).
>
> do_wp_page(): We hold off new references while we update the pte.
>
> xip_unmap: We are not taking the PageLock so we cannot
> use the invalidate_page mmu_rmap_notifier. invalidate_range_begin/end
> stands in.

This whole thing would be much better if you didn't rely on the page
lock at all, but either a) used the same locking as Linux does for its
ptes/tlbs, or b) have some locking that is private to the mmu notifier
code. Then there is not all this new stuff that has to be understood in
the core VM.

Also, why do you have to "invalidate" ranges when switching to a
_more_ permissive state? This stuff should basically be the same as
(a subset of) the TLB flushing API AFAIKS. Anything more is a pretty
big burden to put in the core VM.

See my alternative patch I posted -- I can't see why it won't work
just like a TLB.

As far as sleeping inside callbacks goes... I think there are big
problems with the patch (the sleeping patch and the external rmap
patch). I don't think it is workable in its current state. Either
we have to make some big changes to the core VM, or we have to turn
some locks into sleeping locks to do it properly AFAIKS. Neither
one is good.

But anyway, I don't really think the two approaches (Andrea's
notifiers vs sleeping/xrmap) should be tangled up too much. I
think Andrea's can possibly be quite unintrusive and useful very
soon.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-16  3:37   ` Andrew Morton
@ 2008-02-16 19:26     ` Christoph Lameter
  0 siblings, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-02-16 19:26 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Fri, 15 Feb 2008, Andrew Morton wrote:

> On Thu, 14 Feb 2008 22:49:01 -0800 Christoph Lameter <clameter@sgi.com> wrote:
> 
> > The invalidation of address ranges in a mm_struct needs to be
> > performed when pages are removed or permissions etc change.
> 
> hm.  Do they?  Why?  If I'm in the process of zero-copy writing a hunk of
> memory out to hardware then do I care if someone write-protects the ptes?
> 
> Spose so, but some fleshing-out of the various scenarios here would clarify
> things.

You care f.e. if the VM needs to writeprotect a memory range and a write 
occurs. In that case the VM needs to be proper write processing and write 
through an external pte would cause memory corruption.

> > If invalidate_range_begin() is called with locks held then we
> > pass a flag into invalidate_range() to indicate that no sleeping is
> > possible. Locks are only held for truncate and huge pages.
> 
> This is so bad.

Ok so I can twidlle around with the inode_mmap_lock to drop it while this 
is called?

> > In two cases we use invalidate_range_begin/end to invalidate
> > single pages because the pair allows holding off new references
> > (idea by Robin Holt).
> 
> Assuming that there is a missing "within the range" in this description, I
> assume that all clients will just throw up theior hands in horror and will
> disallow all references to all parts of the mm.

Right. Missing within the range. We only need to disallow creating new 
ptes right? Why disallow references?
 

> > xip_unmap: We are not taking the PageLock so we cannot
> > use the invalidate_page mmu_rmap_notifier. invalidate_range_begin/end
> > stands in.
> 
> What does "stands in" mean?

Use a range begin / end to invalidate a page.

> > +	mmu_notifier(invalidate_range_begin, mm, start, start + size, 0);
> >  	err = populate_range(mm, vma, start, size, pgoff);
> > +	mmu_notifier(invalidate_range_end, mm, start, start + size, 0);
> 
> To avoid off-by-one confusion the changelogs, documentation and comments
> should be very careful to tell the reader whether the range includes the
> byte at start+size.  I don't thik that was done?

No it was not. I assumed that the convention is always start - (end - 1) 
and the byte at end is not affected by the operation.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-15  6:49 ` [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges Christoph Lameter
@ 2008-02-16  3:37   ` Andrew Morton
  2008-02-16 19:26     ` Christoph Lameter
  2008-02-19  8:54   ` Nick Piggin
  2008-02-19 23:08   ` Nick Piggin
  2 siblings, 1 reply; 101+ messages in thread
From: Andrew Morton @ 2008-02-16  3:37 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

On Thu, 14 Feb 2008 22:49:01 -0800 Christoph Lameter <clameter@sgi.com> wrote:

> The invalidation of address ranges in a mm_struct needs to be
> performed when pages are removed or permissions etc change.

hm.  Do they?  Why?  If I'm in the process of zero-copy writing a hunk of
memory out to hardware then do I care if someone write-protects the ptes?

Spose so, but some fleshing-out of the various scenarios here would clarify
things.

> If invalidate_range_begin() is called with locks held then we
> pass a flag into invalidate_range() to indicate that no sleeping is
> possible. Locks are only held for truncate and huge pages.

This is so bad.

I supposed in the restricted couple of cases which you're focussed on it
works OK.  But is it generally suitable?  What if IO is in progress?  What
if other cluster nodes need to be talked to?  Does it suit RDMA?

> In two cases we use invalidate_range_begin/end to invalidate
> single pages because the pair allows holding off new references
> (idea by Robin Holt).

Assuming that there is a missing "within the range" in this description, I
assume that all clients will just throw up theior hands in horror and will
disallow all references to all parts of the mm.

Of course, to do that they will need to take a sleeping lock to prevent
other threads from establishing new references.  whoops.

> do_wp_page(): We hold off new references while we update the pte.
> 
> xip_unmap: We are not taking the PageLock so we cannot
> use the invalidate_page mmu_rmap_notifier. invalidate_range_begin/end
> stands in.

What does "stands in" mean?

> Signed-off-by: Andrea Arcangeli <andrea@qumranet.com>
> Signed-off-by: Robin Holt <holt@sgi.com>
> Signed-off-by: Christoph Lameter <clameter@sgi.com>
> 
> ---
>  mm/filemap_xip.c |    5 +++++
>  mm/fremap.c      |    3 +++
>  mm/hugetlb.c     |    3 +++
>  mm/memory.c      |   35 +++++++++++++++++++++++++++++------
>  mm/mmap.c        |    2 ++
>  mm/mprotect.c    |    3 +++
>  mm/mremap.c      |    7 ++++++-
>  7 files changed, 51 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6/mm/fremap.c
> ===================================================================
> --- linux-2.6.orig/mm/fremap.c	2008-02-14 18:43:31.000000000 -0800
> +++ linux-2.6/mm/fremap.c	2008-02-14 18:45:07.000000000 -0800
> @@ -15,6 +15,7 @@
>  #include <linux/rmap.h>
>  #include <linux/module.h>
>  #include <linux/syscalls.h>
> +#include <linux/mmu_notifier.h>
>  
>  #include <asm/mmu_context.h>
>  #include <asm/cacheflush.h>
> @@ -214,7 +215,9 @@ asmlinkage long sys_remap_file_pages(uns
>  		spin_unlock(&mapping->i_mmap_lock);
>  	}
>  
> +	mmu_notifier(invalidate_range_begin, mm, start, start + size, 0);
>  	err = populate_range(mm, vma, start, size, pgoff);
> +	mmu_notifier(invalidate_range_end, mm, start, start + size, 0);

To avoid off-by-one confusion the changelogs, documentation and comments
should be very careful to tell the reader whether the range includes the
byte at start+size.  I don't thik that was done?



^ permalink raw reply	[flat|nested] 101+ messages in thread

* [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-15  6:48 [patch 0/6] MMU Notifiers V7 Christoph Lameter
@ 2008-02-15  6:49 ` Christoph Lameter
  2008-02-16  3:37   ` Andrew Morton
                     ` (2 more replies)
  0 siblings, 3 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-02-15  6:49 UTC (permalink / raw)
  To: akpm
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, general, Steve Wise, Roland Dreier, Kanoj Sarcar,
	steiner, linux-kernel, linux-mm, daniel.blueman

[-- Attachment #1: mmu_invalidate_range_callbacks --]
[-- Type: text/plain, Size: 11235 bytes --]

The invalidation of address ranges in a mm_struct needs to be
performed when pages are removed or permissions etc change.

If invalidate_range_begin() is called with locks held then we
pass a flag into invalidate_range() to indicate that no sleeping is
possible. Locks are only held for truncate and huge pages.

In two cases we use invalidate_range_begin/end to invalidate
single pages because the pair allows holding off new references
(idea by Robin Holt).

do_wp_page(): We hold off new references while we update the pte.

xip_unmap: We are not taking the PageLock so we cannot
use the invalidate_page mmu_rmap_notifier. invalidate_range_begin/end
stands in.

Signed-off-by: Andrea Arcangeli <andrea@qumranet.com>
Signed-off-by: Robin Holt <holt@sgi.com>
Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 mm/filemap_xip.c |    5 +++++
 mm/fremap.c      |    3 +++
 mm/hugetlb.c     |    3 +++
 mm/memory.c      |   35 +++++++++++++++++++++++++++++------
 mm/mmap.c        |    2 ++
 mm/mprotect.c    |    3 +++
 mm/mremap.c      |    7 ++++++-
 7 files changed, 51 insertions(+), 7 deletions(-)

Index: linux-2.6/mm/fremap.c
===================================================================
--- linux-2.6.orig/mm/fremap.c	2008-02-14 18:43:31.000000000 -0800
+++ linux-2.6/mm/fremap.c	2008-02-14 18:45:07.000000000 -0800
@@ -15,6 +15,7 @@
 #include <linux/rmap.h>
 #include <linux/module.h>
 #include <linux/syscalls.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/mmu_context.h>
 #include <asm/cacheflush.h>
@@ -214,7 +215,9 @@ asmlinkage long sys_remap_file_pages(uns
 		spin_unlock(&mapping->i_mmap_lock);
 	}
 
+	mmu_notifier(invalidate_range_begin, mm, start, start + size, 0);
 	err = populate_range(mm, vma, start, size, pgoff);
+	mmu_notifier(invalidate_range_end, mm, start, start + size, 0);
 	if (!err && !(flags & MAP_NONBLOCK)) {
 		if (unlikely(has_write_lock)) {
 			downgrade_write(&mm->mmap_sem);
Index: linux-2.6/mm/memory.c
===================================================================
--- linux-2.6.orig/mm/memory.c	2008-02-14 18:43:31.000000000 -0800
+++ linux-2.6/mm/memory.c	2008-02-14 18:45:07.000000000 -0800
@@ -51,6 +51,7 @@
 #include <linux/init.h>
 #include <linux/writeback.h>
 #include <linux/memcontrol.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/pgalloc.h>
 #include <asm/uaccess.h>
@@ -611,6 +612,9 @@ int copy_page_range(struct mm_struct *ds
 	if (is_vm_hugetlb_page(vma))
 		return copy_hugetlb_page_range(dst_mm, src_mm, vma);
 
+	if (is_cow_mapping(vma->vm_flags))
+		mmu_notifier(invalidate_range_begin, src_mm, addr, end, 0);
+
 	dst_pgd = pgd_offset(dst_mm, addr);
 	src_pgd = pgd_offset(src_mm, addr);
 	do {
@@ -621,6 +625,11 @@ int copy_page_range(struct mm_struct *ds
 						vma, addr, next))
 			return -ENOMEM;
 	} while (dst_pgd++, src_pgd++, addr = next, addr != end);
+
+	if (is_cow_mapping(vma->vm_flags))
+		mmu_notifier(invalidate_range_end, src_mm,
+						vma->vm_start, end, 0);
+
 	return 0;
 }
 
@@ -893,13 +902,16 @@ unsigned long zap_page_range(struct vm_a
 	struct mmu_gather *tlb;
 	unsigned long end = address + size;
 	unsigned long nr_accounted = 0;
+	int atomic = details ? (details->i_mmap_lock != 0) : 0;
 
 	lru_add_drain();
 	tlb = tlb_gather_mmu(mm, 0);
 	update_hiwater_rss(mm);
+	mmu_notifier(invalidate_range_begin, mm, address, end, atomic);
 	end = unmap_vmas(&tlb, vma, address, end, &nr_accounted, details);
 	if (tlb)
 		tlb_finish_mmu(tlb, address, end);
+	mmu_notifier(invalidate_range_end, mm, address, end, atomic);
 	return end;
 }
 
@@ -1339,7 +1351,7 @@ int remap_pfn_range(struct vm_area_struc
 {
 	pgd_t *pgd;
 	unsigned long next;
-	unsigned long end = addr + PAGE_ALIGN(size);
+	unsigned long start = addr, end = addr + PAGE_ALIGN(size);
 	struct mm_struct *mm = vma->vm_mm;
 	int err;
 
@@ -1373,6 +1385,7 @@ int remap_pfn_range(struct vm_area_struc
 	pfn -= addr >> PAGE_SHIFT;
 	pgd = pgd_offset(mm, addr);
 	flush_cache_range(vma, addr, end);
+	mmu_notifier(invalidate_range_begin, mm, start, end, 0);
 	do {
 		next = pgd_addr_end(addr, end);
 		err = remap_pud_range(mm, pgd, addr, next,
@@ -1380,6 +1393,7 @@ int remap_pfn_range(struct vm_area_struc
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
+	mmu_notifier(invalidate_range_end, mm, start, end, 0);
 	return err;
 }
 EXPORT_SYMBOL(remap_pfn_range);
@@ -1463,10 +1477,11 @@ int apply_to_page_range(struct mm_struct
 {
 	pgd_t *pgd;
 	unsigned long next;
-	unsigned long end = addr + size;
+	unsigned long start = addr, end = addr + size;
 	int err;
 
 	BUG_ON(addr >= end);
+	mmu_notifier(invalidate_range_begin, mm, start, end, 0);
 	pgd = pgd_offset(mm, addr);
 	do {
 		next = pgd_addr_end(addr, end);
@@ -1474,6 +1489,7 @@ int apply_to_page_range(struct mm_struct
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
+	mmu_notifier(invalidate_range_end, mm, start, end, 0);
 	return err;
 }
 EXPORT_SYMBOL_GPL(apply_to_page_range);
@@ -1614,8 +1630,10 @@ static int do_wp_page(struct mm_struct *
 			page_table = pte_offset_map_lock(mm, pmd, address,
 							 &ptl);
 			page_cache_release(old_page);
-			if (!pte_same(*page_table, orig_pte))
-				goto unlock;
+			if (!pte_same(*page_table, orig_pte)) {
+				pte_unmap_unlock(page_table, ptl);
+				goto check_dirty;
+			}
 
 			page_mkwrite = 1;
 		}
@@ -1631,7 +1649,8 @@ static int do_wp_page(struct mm_struct *
 		if (ptep_set_access_flags(vma, address, page_table, entry,1))
 			update_mmu_cache(vma, address, entry);
 		ret |= VM_FAULT_WRITE;
-		goto unlock;
+		pte_unmap_unlock(page_table, ptl);
+		goto check_dirty;
 	}
 
 	/*
@@ -1653,6 +1672,8 @@ gotten:
 	if (mem_cgroup_charge(new_page, mm, GFP_KERNEL))
 		goto oom_free_new;
 
+	mmu_notifier(invalidate_range_begin, mm, address,
+				address + PAGE_SIZE, 0);
 	/*
 	 * Re-check the pte - we dropped the lock
 	 */
@@ -1691,8 +1712,10 @@ gotten:
 		page_cache_release(new_page);
 	if (old_page)
 		page_cache_release(old_page);
-unlock:
 	pte_unmap_unlock(page_table, ptl);
+	mmu_notifier(invalidate_range_end, mm,
+				address, address + PAGE_SIZE, 0);
+check_dirty:
 	if (dirty_page) {
 		if (vma->vm_file)
 			file_update_time(vma->vm_file);
Index: linux-2.6/mm/mmap.c
===================================================================
--- linux-2.6.orig/mm/mmap.c	2008-02-14 18:44:56.000000000 -0800
+++ linux-2.6/mm/mmap.c	2008-02-14 18:45:07.000000000 -0800
@@ -1748,11 +1748,13 @@ static void unmap_region(struct mm_struc
 	lru_add_drain();
 	tlb = tlb_gather_mmu(mm, 0);
 	update_hiwater_rss(mm);
+	mmu_notifier(invalidate_range_begin, mm, start, end, 0);
 	unmap_vmas(&tlb, vma, start, end, &nr_accounted, NULL);
 	vm_unacct_memory(nr_accounted);
 	free_pgtables(&tlb, vma, prev? prev->vm_end: FIRST_USER_ADDRESS,
 				 next? next->vm_start: 0);
 	tlb_finish_mmu(tlb, start, end);
+	mmu_notifier(invalidate_range_end, mm, start, end, 0);
 }
 
 /*
Index: linux-2.6/mm/hugetlb.c
===================================================================
--- linux-2.6.orig/mm/hugetlb.c	2008-02-14 18:43:31.000000000 -0800
+++ linux-2.6/mm/hugetlb.c	2008-02-14 18:45:07.000000000 -0800
@@ -14,6 +14,7 @@
 #include <linux/mempolicy.h>
 #include <linux/cpuset.h>
 #include <linux/mutex.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/page.h>
 #include <asm/pgtable.h>
@@ -755,6 +756,7 @@ void __unmap_hugepage_range(struct vm_ar
 	BUG_ON(start & ~HPAGE_MASK);
 	BUG_ON(end & ~HPAGE_MASK);
 
+	mmu_notifier(invalidate_range_begin, mm, start, end, 1);
 	spin_lock(&mm->page_table_lock);
 	for (address = start; address < end; address += HPAGE_SIZE) {
 		ptep = huge_pte_offset(mm, address);
@@ -775,6 +777,7 @@ void __unmap_hugepage_range(struct vm_ar
 	}
 	spin_unlock(&mm->page_table_lock);
 	flush_tlb_range(vma, start, end);
+	mmu_notifier(invalidate_range_end, mm, start, end, 1);
 	list_for_each_entry_safe(page, tmp, &page_list, lru) {
 		list_del(&page->lru);
 		put_page(page);
Index: linux-2.6/mm/filemap_xip.c
===================================================================
--- linux-2.6.orig/mm/filemap_xip.c	2008-02-14 18:43:31.000000000 -0800
+++ linux-2.6/mm/filemap_xip.c	2008-02-14 18:45:07.000000000 -0800
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/uio.h>
 #include <linux/rmap.h>
+#include <linux/mmu_notifier.h>
 #include <linux/sched.h>
 #include <asm/tlbflush.h>
 
@@ -190,6 +191,8 @@ __xip_unmap (struct address_space * mapp
 		address = vma->vm_start +
 			((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
 		BUG_ON(address < vma->vm_start || address >= vma->vm_end);
+		mmu_notifier(invalidate_range_begin, mm, address,
+					address + PAGE_SIZE, 1);
 		pte = page_check_address(page, mm, address, &ptl);
 		if (pte) {
 			/* Nuke the page table entry. */
@@ -201,6 +204,8 @@ __xip_unmap (struct address_space * mapp
 			pte_unmap_unlock(pte, ptl);
 			page_cache_release(page);
 		}
+		mmu_notifier(invalidate_range_end, mm,
+				address, address + PAGE_SIZE, 1);
 	}
 	spin_unlock(&mapping->i_mmap_lock);
 }
Index: linux-2.6/mm/mremap.c
===================================================================
--- linux-2.6.orig/mm/mremap.c	2008-02-14 18:43:31.000000000 -0800
+++ linux-2.6/mm/mremap.c	2008-02-14 18:45:07.000000000 -0800
@@ -18,6 +18,7 @@
 #include <linux/highmem.h>
 #include <linux/security.h>
 #include <linux/syscalls.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/uaccess.h>
 #include <asm/cacheflush.h>
@@ -124,12 +125,15 @@ unsigned long move_page_tables(struct vm
 		unsigned long old_addr, struct vm_area_struct *new_vma,
 		unsigned long new_addr, unsigned long len)
 {
-	unsigned long extent, next, old_end;
+	unsigned long extent, next, old_start, old_end;
 	pmd_t *old_pmd, *new_pmd;
 
+	old_start = old_addr;
 	old_end = old_addr + len;
 	flush_cache_range(vma, old_addr, old_end);
 
+	mmu_notifier(invalidate_range_begin, vma->vm_mm,
+					old_addr, old_end, 0);
 	for (; old_addr < old_end; old_addr += extent, new_addr += extent) {
 		cond_resched();
 		next = (old_addr + PMD_SIZE) & PMD_MASK;
@@ -150,6 +154,7 @@ unsigned long move_page_tables(struct vm
 		move_ptes(vma, old_pmd, old_addr, old_addr + extent,
 				new_vma, new_pmd, new_addr);
 	}
+	mmu_notifier(invalidate_range_end, vma->vm_mm, old_start, old_end, 0);
 
 	return len + old_addr - old_end;	/* how much done */
 }
Index: linux-2.6/mm/mprotect.c
===================================================================
--- linux-2.6.orig/mm/mprotect.c	2008-02-14 18:43:31.000000000 -0800
+++ linux-2.6/mm/mprotect.c	2008-02-14 18:45:07.000000000 -0800
@@ -21,6 +21,7 @@
 #include <linux/syscalls.h>
 #include <linux/swap.h>
 #include <linux/swapops.h>
+#include <linux/mmu_notifier.h>
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>
 #include <asm/cacheflush.h>
@@ -198,10 +199,12 @@ success:
 		dirty_accountable = 1;
 	}
 
+	mmu_notifier(invalidate_range_begin, mm, start, end, 0);
 	if (is_vm_hugetlb_page(vma))
 		hugetlb_change_protection(vma, start, end, vma->vm_page_prot);
 	else
 		change_protection(vma, start, end, vma->vm_page_prot, dirty_accountable);
+	mmu_notifier(invalidate_range_end, mm, start, end, 0);
 	vm_stat_account(mm, oldflags, vma->vm_file, -nrpages);
 	vm_stat_account(mm, newflags, vma->vm_file, nrpages);
 	return 0;

-- 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-02-08 22:06 [patch 0/6] MMU Notifiers V6 Christoph Lameter
@ 2008-02-08 22:06 ` Christoph Lameter
  0 siblings, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-02-08 22:06 UTC (permalink / raw)
  To: akpm
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus, kvm-devel,
	Peter Zijlstra, steiner, linux-kernel, linux-mm, daniel.blueman

[-- Attachment #1: mmu_invalidate_range_callbacks --]
[-- Type: text/plain, Size: 11235 bytes --]

The invalidation of address ranges in a mm_struct needs to be
performed when pages are removed or permissions etc change.

If invalidate_range_begin() is called with locks held then we
pass a flag into invalidate_range() to indicate that no sleeping is
possible. Locks are only held for truncate and huge pages.

In two cases we use invalidate_range_begin/end to invalidate
single pages because the pair allows holding off new references
(idea by Robin Holt).

do_wp_page(): We hold off new references while we update the pte.

xip_unmap: We are not taking the PageLock so we cannot
use the invalidate_page mmu_rmap_notifier. invalidate_range_begin/end
stands in.

Signed-off-by: Andrea Arcangeli <andrea@qumranet.com>
Signed-off-by: Robin Holt <holt@sgi.com>
Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 mm/filemap_xip.c |    5 +++++
 mm/fremap.c      |    3 +++
 mm/hugetlb.c     |    3 +++
 mm/memory.c      |   35 +++++++++++++++++++++++++++++------
 mm/mmap.c        |    2 ++
 mm/mprotect.c    |    3 +++
 mm/mremap.c      |    7 ++++++-
 7 files changed, 51 insertions(+), 7 deletions(-)

Index: linux-2.6/mm/fremap.c
===================================================================
--- linux-2.6.orig/mm/fremap.c	2008-02-08 13:18:58.000000000 -0800
+++ linux-2.6/mm/fremap.c	2008-02-08 13:25:22.000000000 -0800
@@ -15,6 +15,7 @@
 #include <linux/rmap.h>
 #include <linux/module.h>
 #include <linux/syscalls.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/mmu_context.h>
 #include <asm/cacheflush.h>
@@ -214,7 +215,9 @@ asmlinkage long sys_remap_file_pages(uns
 		spin_unlock(&mapping->i_mmap_lock);
 	}
 
+	mmu_notifier(invalidate_range_begin, mm, start, start + size, 0);
 	err = populate_range(mm, vma, start, size, pgoff);
+	mmu_notifier(invalidate_range_end, mm, start, start + size, 0);
 	if (!err && !(flags & MAP_NONBLOCK)) {
 		if (unlikely(has_write_lock)) {
 			downgrade_write(&mm->mmap_sem);
Index: linux-2.6/mm/memory.c
===================================================================
--- linux-2.6.orig/mm/memory.c	2008-02-08 13:22:14.000000000 -0800
+++ linux-2.6/mm/memory.c	2008-02-08 13:25:22.000000000 -0800
@@ -51,6 +51,7 @@
 #include <linux/init.h>
 #include <linux/writeback.h>
 #include <linux/memcontrol.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/pgalloc.h>
 #include <asm/uaccess.h>
@@ -611,6 +612,9 @@ int copy_page_range(struct mm_struct *ds
 	if (is_vm_hugetlb_page(vma))
 		return copy_hugetlb_page_range(dst_mm, src_mm, vma);
 
+	if (is_cow_mapping(vma->vm_flags))
+		mmu_notifier(invalidate_range_begin, src_mm, addr, end, 0);
+
 	dst_pgd = pgd_offset(dst_mm, addr);
 	src_pgd = pgd_offset(src_mm, addr);
 	do {
@@ -621,6 +625,11 @@ int copy_page_range(struct mm_struct *ds
 						vma, addr, next))
 			return -ENOMEM;
 	} while (dst_pgd++, src_pgd++, addr = next, addr != end);
+
+	if (is_cow_mapping(vma->vm_flags))
+		mmu_notifier(invalidate_range_end, src_mm,
+						vma->vm_start, end, 0);
+
 	return 0;
 }
 
@@ -893,13 +902,16 @@ unsigned long zap_page_range(struct vm_a
 	struct mmu_gather *tlb;
 	unsigned long end = address + size;
 	unsigned long nr_accounted = 0;
+	int atomic = details ? (details->i_mmap_lock != 0) : 0;
 
 	lru_add_drain();
 	tlb = tlb_gather_mmu(mm, 0);
 	update_hiwater_rss(mm);
+	mmu_notifier(invalidate_range_begin, mm, address, end, atomic);
 	end = unmap_vmas(&tlb, vma, address, end, &nr_accounted, details);
 	if (tlb)
 		tlb_finish_mmu(tlb, address, end);
+	mmu_notifier(invalidate_range_end, mm, address, end, atomic);
 	return end;
 }
 
@@ -1337,7 +1349,7 @@ int remap_pfn_range(struct vm_area_struc
 {
 	pgd_t *pgd;
 	unsigned long next;
-	unsigned long end = addr + PAGE_ALIGN(size);
+	unsigned long start = addr, end = addr + PAGE_ALIGN(size);
 	struct mm_struct *mm = vma->vm_mm;
 	int err;
 
@@ -1371,6 +1383,7 @@ int remap_pfn_range(struct vm_area_struc
 	pfn -= addr >> PAGE_SHIFT;
 	pgd = pgd_offset(mm, addr);
 	flush_cache_range(vma, addr, end);
+	mmu_notifier(invalidate_range_begin, mm, start, end, 0);
 	do {
 		next = pgd_addr_end(addr, end);
 		err = remap_pud_range(mm, pgd, addr, next,
@@ -1378,6 +1391,7 @@ int remap_pfn_range(struct vm_area_struc
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
+	mmu_notifier(invalidate_range_end, mm, start, end, 0);
 	return err;
 }
 EXPORT_SYMBOL(remap_pfn_range);
@@ -1461,10 +1475,11 @@ int apply_to_page_range(struct mm_struct
 {
 	pgd_t *pgd;
 	unsigned long next;
-	unsigned long end = addr + size;
+	unsigned long start = addr, end = addr + size;
 	int err;
 
 	BUG_ON(addr >= end);
+	mmu_notifier(invalidate_range_begin, mm, start, end, 0);
 	pgd = pgd_offset(mm, addr);
 	do {
 		next = pgd_addr_end(addr, end);
@@ -1472,6 +1487,7 @@ int apply_to_page_range(struct mm_struct
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
+	mmu_notifier(invalidate_range_end, mm, start, end, 0);
 	return err;
 }
 EXPORT_SYMBOL_GPL(apply_to_page_range);
@@ -1612,8 +1628,10 @@ static int do_wp_page(struct mm_struct *
 			page_table = pte_offset_map_lock(mm, pmd, address,
 							 &ptl);
 			page_cache_release(old_page);
-			if (!pte_same(*page_table, orig_pte))
-				goto unlock;
+			if (!pte_same(*page_table, orig_pte)) {
+				pte_unmap_unlock(page_table, ptl);
+				goto check_dirty;
+			}
 
 			page_mkwrite = 1;
 		}
@@ -1629,7 +1647,8 @@ static int do_wp_page(struct mm_struct *
 		if (ptep_set_access_flags(vma, address, page_table, entry,1))
 			update_mmu_cache(vma, address, entry);
 		ret |= VM_FAULT_WRITE;
-		goto unlock;
+		pte_unmap_unlock(page_table, ptl);
+		goto check_dirty;
 	}
 
 	/*
@@ -1651,6 +1670,8 @@ gotten:
 	if (mem_cgroup_charge(new_page, mm, GFP_KERNEL))
 		goto oom_free_new;
 
+	mmu_notifier(invalidate_range_begin, mm, address,
+				address + PAGE_SIZE, 0);
 	/*
 	 * Re-check the pte - we dropped the lock
 	 */
@@ -1689,8 +1710,10 @@ gotten:
 		page_cache_release(new_page);
 	if (old_page)
 		page_cache_release(old_page);
-unlock:
 	pte_unmap_unlock(page_table, ptl);
+	mmu_notifier(invalidate_range_end, mm,
+				address, address + PAGE_SIZE, 0);
+check_dirty:
 	if (dirty_page) {
 		if (vma->vm_file)
 			file_update_time(vma->vm_file);
Index: linux-2.6/mm/mmap.c
===================================================================
--- linux-2.6.orig/mm/mmap.c	2008-02-08 13:25:21.000000000 -0800
+++ linux-2.6/mm/mmap.c	2008-02-08 13:25:22.000000000 -0800
@@ -1748,11 +1748,13 @@ static void unmap_region(struct mm_struc
 	lru_add_drain();
 	tlb = tlb_gather_mmu(mm, 0);
 	update_hiwater_rss(mm);
+	mmu_notifier(invalidate_range_begin, mm, start, end, 0);
 	unmap_vmas(&tlb, vma, start, end, &nr_accounted, NULL);
 	vm_unacct_memory(nr_accounted);
 	free_pgtables(&tlb, vma, prev? prev->vm_end: FIRST_USER_ADDRESS,
 				 next? next->vm_start: 0);
 	tlb_finish_mmu(tlb, start, end);
+	mmu_notifier(invalidate_range_end, mm, start, end, 0);
 }
 
 /*
Index: linux-2.6/mm/hugetlb.c
===================================================================
--- linux-2.6.orig/mm/hugetlb.c	2008-02-08 13:22:14.000000000 -0800
+++ linux-2.6/mm/hugetlb.c	2008-02-08 13:25:22.000000000 -0800
@@ -14,6 +14,7 @@
 #include <linux/mempolicy.h>
 #include <linux/cpuset.h>
 #include <linux/mutex.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/page.h>
 #include <asm/pgtable.h>
@@ -753,6 +754,7 @@ void __unmap_hugepage_range(struct vm_ar
 	BUG_ON(start & ~HPAGE_MASK);
 	BUG_ON(end & ~HPAGE_MASK);
 
+	mmu_notifier(invalidate_range_begin, mm, start, end, 1);
 	spin_lock(&mm->page_table_lock);
 	for (address = start; address < end; address += HPAGE_SIZE) {
 		ptep = huge_pte_offset(mm, address);
@@ -773,6 +775,7 @@ void __unmap_hugepage_range(struct vm_ar
 	}
 	spin_unlock(&mm->page_table_lock);
 	flush_tlb_range(vma, start, end);
+	mmu_notifier(invalidate_range_end, mm, start, end, 1);
 	list_for_each_entry_safe(page, tmp, &page_list, lru) {
 		list_del(&page->lru);
 		put_page(page);
Index: linux-2.6/mm/filemap_xip.c
===================================================================
--- linux-2.6.orig/mm/filemap_xip.c	2008-02-08 13:22:14.000000000 -0800
+++ linux-2.6/mm/filemap_xip.c	2008-02-08 13:25:22.000000000 -0800
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/uio.h>
 #include <linux/rmap.h>
+#include <linux/mmu_notifier.h>
 #include <linux/sched.h>
 #include <asm/tlbflush.h>
 
@@ -190,6 +191,8 @@ __xip_unmap (struct address_space * mapp
 		address = vma->vm_start +
 			((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
 		BUG_ON(address < vma->vm_start || address >= vma->vm_end);
+		mmu_notifier(invalidate_range_begin, mm, address,
+					address + PAGE_SIZE, 1);
 		pte = page_check_address(page, mm, address, &ptl);
 		if (pte) {
 			/* Nuke the page table entry. */
@@ -201,6 +204,8 @@ __xip_unmap (struct address_space * mapp
 			pte_unmap_unlock(pte, ptl);
 			page_cache_release(page);
 		}
+		mmu_notifier(invalidate_range_end, mm,
+				address, address + PAGE_SIZE, 1);
 	}
 	spin_unlock(&mapping->i_mmap_lock);
 }
Index: linux-2.6/mm/mremap.c
===================================================================
--- linux-2.6.orig/mm/mremap.c	2008-02-08 13:18:58.000000000 -0800
+++ linux-2.6/mm/mremap.c	2008-02-08 13:25:22.000000000 -0800
@@ -18,6 +18,7 @@
 #include <linux/highmem.h>
 #include <linux/security.h>
 #include <linux/syscalls.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/uaccess.h>
 #include <asm/cacheflush.h>
@@ -124,12 +125,15 @@ unsigned long move_page_tables(struct vm
 		unsigned long old_addr, struct vm_area_struct *new_vma,
 		unsigned long new_addr, unsigned long len)
 {
-	unsigned long extent, next, old_end;
+	unsigned long extent, next, old_start, old_end;
 	pmd_t *old_pmd, *new_pmd;
 
+	old_start = old_addr;
 	old_end = old_addr + len;
 	flush_cache_range(vma, old_addr, old_end);
 
+	mmu_notifier(invalidate_range_begin, vma->vm_mm,
+					old_addr, old_end, 0);
 	for (; old_addr < old_end; old_addr += extent, new_addr += extent) {
 		cond_resched();
 		next = (old_addr + PMD_SIZE) & PMD_MASK;
@@ -150,6 +154,7 @@ unsigned long move_page_tables(struct vm
 		move_ptes(vma, old_pmd, old_addr, old_addr + extent,
 				new_vma, new_pmd, new_addr);
 	}
+	mmu_notifier(invalidate_range_end, vma->vm_mm, old_start, old_end, 0);
 
 	return len + old_addr - old_end;	/* how much done */
 }
Index: linux-2.6/mm/mprotect.c
===================================================================
--- linux-2.6.orig/mm/mprotect.c	2008-02-08 13:18:58.000000000 -0800
+++ linux-2.6/mm/mprotect.c	2008-02-08 13:25:22.000000000 -0800
@@ -21,6 +21,7 @@
 #include <linux/syscalls.h>
 #include <linux/swap.h>
 #include <linux/swapops.h>
+#include <linux/mmu_notifier.h>
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>
 #include <asm/cacheflush.h>
@@ -198,10 +199,12 @@ success:
 		dirty_accountable = 1;
 	}
 
+	mmu_notifier(invalidate_range_begin, mm, start, end, 0);
 	if (is_vm_hugetlb_page(vma))
 		hugetlb_change_protection(vma, start, end, vma->vm_page_prot);
 	else
 		change_protection(vma, start, end, vma->vm_page_prot, dirty_accountable);
+	mmu_notifier(invalidate_range_end, mm, start, end, 0);
 	vm_stat_account(mm, oldflags, vma->vm_file, -nrpages);
 	vm_stat_account(mm, newflags, vma->vm_file, nrpages);
 	return 0;

-- 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30 23:52                           ` Andrea Arcangeli
@ 2008-01-31  0:01                             ` Christoph Lameter
  0 siblings, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-31  0:01 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Thu, 31 Jan 2008, Andrea Arcangeli wrote:

> > -	void (*invalidate_range)(struct mmu_notifier *mn,
> > +	void (*invalidate_range_begin)(struct mmu_notifier *mn,
> >  				 struct mm_struct *mm,
> > -				 unsigned long start, unsigned long end,
> >  				 int lock);
> > +
> > +	void (*invalidate_range_end)(struct mmu_notifier *mn,
> > +				 struct mm_struct *mm,
> > +				 unsigned long start, unsigned long end);
> >  };
> 
> start/finish/begin/end/before/after? ;)

Well lets pick one and then stick to it.

> I'd drop the 'int lock', you should skip the before/after if
> i_mmap_lock isn't null and offload it to the caller before taking the
> lock. At least for the "after" call that looks a few liner change,
> didn't figure out the "before" yet.

How we offload that? Before the scan of the rmaps we do not have the 
mmstruct. So we'd need another notifier_rmap_callback.

> Given the amount of changes that are going on in design terms to cover
> both XPMEM and GRE, can we split the minimal invalidate_page that
> provides an obviously safe and feature complete mmu notifier code for
> KVM, and merge that first patch that will cover KVM 100%, it will

The obvious solution does not scale. You will have a callback for every 
page and there may be a million of those if you have a 4GB process.

> made so that are extendible in backwards compatible way. I think
> invalidate_page inside ptep_clear_flush is the first fundamental block
> of the mmu notifiers. Then once the fundamental is in and obviously
> safe and feature complete for KVM, the rest can be added very easily
> with incremental patches as far as I can tell. That would be my
> preferred route ;)

We need to have a coherent notifier solution that works for multiple 
scenarios. I think a working invalidate_range would also be required for 
KVM. KVM and GRUB are very similar so they should be able to use the same 
mechanisms and we need to properly document how that mechanism is safe. 
Either both take a page refcount or none.




^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30 19:50                         ` Christoph Lameter
  2008-01-30 22:18                           ` Robin Holt
@ 2008-01-30 23:52                           ` Andrea Arcangeli
  2008-01-31  0:01                             ` Christoph Lameter
  1 sibling, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-30 23:52 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Wed, Jan 30, 2008 at 11:50:26AM -0800, Christoph Lameter wrote:
> Then we have 
> 
> invalidate_range_start(mm)
> 
> and
> 
> invalidate_range_finish(mm, start, end)
> 
> in addition to the invalidate rmap_notifier?
> 
> ---
>  include/linux/mmu_notifier.h |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/include/linux/mmu_notifier.h
> ===================================================================
> --- linux-2.6.orig/include/linux/mmu_notifier.h	2008-01-30 11:49:02.000000000 -0800
> +++ linux-2.6/include/linux/mmu_notifier.h	2008-01-30 11:49:57.000000000 -0800
> @@ -69,10 +69,13 @@ struct mmu_notifier_ops {
>  	/*
>  	 * lock indicates that the function is called under spinlock.
>  	 */
> -	void (*invalidate_range)(struct mmu_notifier *mn,
> +	void (*invalidate_range_begin)(struct mmu_notifier *mn,
>  				 struct mm_struct *mm,
> -				 unsigned long start, unsigned long end,
>  				 int lock);
> +
> +	void (*invalidate_range_end)(struct mmu_notifier *mn,
> +				 struct mm_struct *mm,
> +				 unsigned long start, unsigned long end);
>  };

start/finish/begin/end/before/after? ;)

I'd drop the 'int lock', you should skip the before/after if
i_mmap_lock isn't null and offload it to the caller before taking the
lock. At least for the "after" call that looks a few liner change,
didn't figure out the "before" yet.

Given the amount of changes that are going on in design terms to cover
both XPMEM and GRE, can we split the minimal invalidate_page that
provides an obviously safe and feature complete mmu notifier code for
KVM, and merge that first patch that will cover KVM 100%, it will
cover GRE 90%, and then we add invalidate_range_before/after in a
separate patch and we close the remaining 10% for GRE covering
ptep_get_and_clear or whatever else ptep_*?  The mmu notifiers are
made so that are extendible in backwards compatible way. I think
invalidate_page inside ptep_clear_flush is the first fundamental block
of the mmu notifiers. Then once the fundamental is in and obviously
safe and feature complete for KVM, the rest can be added very easily
with incremental patches as far as I can tell. That would be my
preferred route ;)

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30 19:50                         ` Christoph Lameter
@ 2008-01-30 22:18                           ` Robin Holt
  2008-01-30 23:52                           ` Andrea Arcangeli
  1 sibling, 0 replies; 101+ messages in thread
From: Robin Holt @ 2008-01-30 22:18 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	steiner, linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Wed, Jan 30, 2008 at 11:50:26AM -0800, Christoph Lameter wrote:
> On Wed, 30 Jan 2008, Andrea Arcangeli wrote:
> 
> > XPMEM requires with invalidate_range (sleepy) +
> > before_invalidate_range (sleepy). invalidate_all should also be called
> > before_release (both sleepy).
> > 
> > It sounds we need full overlap of information provided by
> > invalidate_page and invalidate_range to fit all three models (the
> > opposite of the zero objective that current V3 is taking). And the
> > swap will be handled only by invalidate_page either through linux rmap
> > or external rmap (with the latter that can sleep so it's ok for you,
> > the former not). GRU can safely use the either the linux rmap notifier
> > or the external rmap notifier equally well, because when try_to_unmap
> > is called the page is locked and obviously pinned by the VM itself.
> 
> So put the invalidate_page() callbacks in everywhere.

The way I am envisioning it, we essentially drop back to Andrea's original
patch.  We then introduce a invalidate_range_begin (I was really thinking
of it as invalidate_and_lock_range()) and an invalidate_range_end (again
I was thinking of unlock_range).

Thanks,
Robin

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30 20:29                           ` Jack Steiner
@ 2008-01-30 20:55                             ` Christoph Lameter
  0 siblings, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30 20:55 UTC (permalink / raw)
  To: Jack Steiner
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Wed, 30 Jan 2008, Jack Steiner wrote:

> > Seems that we cannot rely on the invalidate_ranges for correctness at all?
> > We need to have invalidate_page() always. invalidate_range() is only an 
> > optimization.
> > 
> 
> I don't understand your point "an optimization". How would invalidate_range
> as currently defined be correctly used?

We are changing definitions. The original patch by Andrea calls 
invalidate_page for each pte that is cleared. So strictly you would not 
need an invalidate_range.

> It _looks_ like it would work only if xpmem/gru/etc takes a refcnt on
> the page & drops it when invalidate_range is called. That may work (not sure)
> for xpmem but not for the GRU.

The refcount is not necessary if we adopt Andrea's approach of a callback 
on the clearing of each pte. At that point the page is still guaranteed to 
exist. If we do the range_invalidate later (as in V3) then the page may 
have been released (see sys_remap_file_pages() f.e.) before we zap the GRU 
ptes. So there will be a time when the GRU may write to a page that has 
been freed and used for another purpose.

Taking a refcount on the page defers the free until the range_invalidate 
runs.

I would prefer a solution that does not require taking refcounts (pins) 
for establishing an external pte and for release (like what the GRU does).

If we could effectively determine that there are no external ptes in a 
range then the invalidate_page() call may return immediately. Maybe it is 
then effective to do these gazillions of invalidate_page() calls when a 
process terminates or an remap is performed.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30 19:41                         ` Christoph Lameter
@ 2008-01-30 20:29                           ` Jack Steiner
  2008-01-30 20:55                             ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Jack Steiner @ 2008-01-30 20:29 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Wed, Jan 30, 2008 at 11:41:29AM -0800, Christoph Lameter wrote:
> On Wed, 30 Jan 2008, Jack Steiner wrote:
> 
> > I see what you mean. I need to review to mail to see why this changed
> > but in the original discussions with Christoph, the invalidate_range
> > callouts were suppose to be made BEFORE the pages were put on the freelist.
> 
> Seems that we cannot rely on the invalidate_ranges for correctness at all?
> We need to have invalidate_page() always. invalidate_range() is only an 
> optimization.
> 

I don't understand your point "an optimization". How would invalidate_range
as currently defined be correctly used?

It _looks_ like it would work only if xpmem/gru/etc takes a refcnt on
the page & drops it when invalidate_range is called. That may work (not sure)
for xpmem but not for the GRU.

--- jack


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30 18:25                       ` Andrea Arcangeli
@ 2008-01-30 19:50                         ` Christoph Lameter
  2008-01-30 22:18                           ` Robin Holt
  2008-01-30 23:52                           ` Andrea Arcangeli
  0 siblings, 2 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30 19:50 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Wed, 30 Jan 2008, Andrea Arcangeli wrote:

> XPMEM requires with invalidate_range (sleepy) +
> before_invalidate_range (sleepy). invalidate_all should also be called
> before_release (both sleepy).
> 
> It sounds we need full overlap of information provided by
> invalidate_page and invalidate_range to fit all three models (the
> opposite of the zero objective that current V3 is taking). And the
> swap will be handled only by invalidate_page either through linux rmap
> or external rmap (with the latter that can sleep so it's ok for you,
> the former not). GRU can safely use the either the linux rmap notifier
> or the external rmap notifier equally well, because when try_to_unmap
> is called the page is locked and obviously pinned by the VM itself.

So put the invalidate_page() callbacks in everywhere.

Then we have 

invalidate_range_start(mm)

and

invalidate_range_finish(mm, start, end)

in addition to the invalidate rmap_notifier?

---
 include/linux/mmu_notifier.h |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: linux-2.6/include/linux/mmu_notifier.h
===================================================================
--- linux-2.6.orig/include/linux/mmu_notifier.h	2008-01-30 11:49:02.000000000 -0800
+++ linux-2.6/include/linux/mmu_notifier.h	2008-01-30 11:49:57.000000000 -0800
@@ -69,10 +69,13 @@ struct mmu_notifier_ops {
 	/*
 	 * lock indicates that the function is called under spinlock.
 	 */
-	void (*invalidate_range)(struct mmu_notifier *mn,
+	void (*invalidate_range_begin)(struct mmu_notifier *mn,
 				 struct mm_struct *mm,
-				 unsigned long start, unsigned long end,
 				 int lock);
+
+	void (*invalidate_range_end)(struct mmu_notifier *mn,
+				 struct mm_struct *mm,
+				 unsigned long start, unsigned long end);
 };
 
 struct mmu_rmap_notifier_ops;

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30 14:43                       ` Jack Steiner
@ 2008-01-30 19:41                         ` Christoph Lameter
  2008-01-30 20:29                           ` Jack Steiner
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30 19:41 UTC (permalink / raw)
  To: Jack Steiner
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Wed, 30 Jan 2008, Jack Steiner wrote:

> I see what you mean. I need to review to mail to see why this changed
> but in the original discussions with Christoph, the invalidate_range
> callouts were suppose to be made BEFORE the pages were put on the freelist.

Seems that we cannot rely on the invalidate_ranges for correctness at all?
We need to have invalidate_page() always. invalidate_range() is only an 
optimization.



^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30 16:11                 ` Robin Holt
  2008-01-30 17:04                   ` Andrea Arcangeli
@ 2008-01-30 19:35                   ` Christoph Lameter
  1 sibling, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30 19:35 UTC (permalink / raw)
  To: Robin Holt
  Cc: Andrea Arcangeli, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Wed, 30 Jan 2008, Robin Holt wrote:

> I think I need to straighten this discussion out in my head a little bit.
> Am I correct in assuming Andrea's original patch set did not have any SMP
> race conditions for KVM?  If so, then we need to start looking at how to
> implement Christoph's and my changes in a safe fashion.  Andrea, I agree
> complete that our introduction of the range callouts have introduced
> SMP races.

The original patch drew the clearing of the sptes into ptep_clear_flush(). 
So the invalidate_page was called for each page regardless if we had been 
doing an invalidate range before or not. It seems that the the 
invalidate_range() was just there for optimization.
 
> The three issues we need to simultaneously solve is revoking the remote
> page table/tlb information while still in a sleepable context and not
> having the remote faulters become out of sync with the granting process.
> Currently, I don't see a way to do that cleanly with a single callout.

You could use the invalidate_page callouts to set a flag that no 
additional rmap entries may be added until the invalidate_range has 
occurred? We could add back all the original invalidate_pages() and pass
a flag that specifies that an invalidate range will follow. The notifier 
can then decide what to do with that information. If its okay to defer 
then do nothing and wait for the range_invalidate. XPmem could stop 
allowing external references to be established until the invalidate_range 
was successful.

Jack had a concern that multiple callouts for the same pte could cause 
problems.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30 17:30                     ` Robin Holt
@ 2008-01-30 18:25                       ` Andrea Arcangeli
  2008-01-30 19:50                         ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-30 18:25 UTC (permalink / raw)
  To: Robin Holt
  Cc: Christoph Lameter, Avi Kivity, Izik Eidus, Nick Piggin,
	kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra, steiner,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Wed, Jan 30, 2008 at 11:30:09AM -0600, Robin Holt wrote:
> I don't think I saw the answer to my original question.  I assume your
> original patch, extended in a way similar to what Christoph has done,
> can be made to work to cover both the KVM and GRU (Jack's) case.

Yes, I think so.

> XPMEM, however, does not look to be solvable due to the three simultaneous
> issues above.  To address that, I think I am coming to the conclusion
> that we need an accompanying but seperate pair of callouts.  The first

The mmu_rmap_notifiers are already one separate pair of callouts and
we can add more of them of course.

> will ensure the remote page tables and TLBs are cleared and all page
> information is returned back to the process that is granting access to
> its address space.  That will include an implicit block on the address
> range so no further faults will be satisfied by the remote accessor
> (forgot the KVM name for this, sorry).  Any faults will be held off
> and only the processes page tables/TLBs are in play.  Once the normal

Good, this "block" is how you close the race condition, and you need
the second callout to "unblock" (this is why it could hardly work well
before with a single invalidate_range).

> processing of the kernel is complete, an unlock callout would be made
> for the range and then faulting may occur on behalf of the process again.

This sounds good.

> Currently, this is the only direct solution that I can see as a
> possibility.  My question is two fold.  Does this seem like a reasonable
> means to solve the three simultaneous issues above and if so, does it
> seem like the most reasonable means?

Yes.

KVM can deal with both invalidate_page (atomic) and invalidate_range (sleepy)

GRU can only deal with invalidate_page (atomic)

XPMEM requires with invalidate_range (sleepy) +
before_invalidate_range (sleepy). invalidate_all should also be called
before_release (both sleepy).

It sounds we need full overlap of information provided by
invalidate_page and invalidate_range to fit all three models (the
opposite of the zero objective that current V3 is taking). And the
swap will be handled only by invalidate_page either through linux rmap
or external rmap (with the latter that can sleep so it's ok for you,
the former not). GRU can safely use the either the linux rmap notifier
or the external rmap notifier equally well, because when try_to_unmap
is called the page is locked and obviously pinned by the VM itself.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30 17:04                   ` Andrea Arcangeli
@ 2008-01-30 17:30                     ` Robin Holt
  2008-01-30 18:25                       ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Robin Holt @ 2008-01-30 17:30 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Christoph Lameter, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	steiner, linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Wed, Jan 30, 2008 at 06:04:52PM +0100, Andrea Arcangeli wrote:
> On Wed, Jan 30, 2008 at 10:11:24AM -0600, Robin Holt wrote:
...
> > The three issues we need to simultaneously solve is revoking the remote
> > page table/tlb information while still in a sleepable context and not
> > having the remote faulters become out of sync with the granting process.
...
> > Could we consider doing a range-based recall and lock callout before
> > clearing the processes page tables/TLBs, then use the _page or _range
> > callouts from Andrea's patch to clear the mappings,  finally make a
> > range-based unlock callout.  The mmu_notifier user would usually use ops
> > for either the recall+lock/unlock family of callouts or the _page/_range
> > family of callouts.
> 
> invalidate_page/age_page can return inside ptep_clear_flush/young and
> Jack will need that too. Infact Jack will need an invalidate_page also
> inside ptep_get_and_clear. And the range callout will be done always
> in a sleeping context and it'll relay on the page-pin to be safe (when
> details->i_mmap_lock != NULL invalidate_range it shouldn't be called
> inside zap_page_range but before returning from
> unmap_mapping_range_vma before cond_resched). This will make
> everything a bit simpler and less prone to breakage IMHO, plus it'll
> have a chance to work for Jack w/o page-pin without additional
> cluttering of mm/*.c.

I don't think I saw the answer to my original question.  I assume your
original patch, extended in a way similar to what Christoph has done,
can be made to work to cover both the KVM and GRU (Jack's) case.

XPMEM, however, does not look to be solvable due to the three simultaneous
issues above.  To address that, I think I am coming to the conclusion
that we need an accompanying but seperate pair of callouts.  The first
will ensure the remote page tables and TLBs are cleared and all page
information is returned back to the process that is granting access to
its address space.  That will include an implicit block on the address
range so no further faults will be satisfied by the remote accessor
(forgot the KVM name for this, sorry).  Any faults will be held off
and only the processes page tables/TLBs are in play.  Once the normal
processing of the kernel is complete, an unlock callout would be made
for the range and then faulting may occur on behalf of the process again.

Currently, this is the only direct solution that I can see as a
possibility.  My question is two fold.  Does this seem like a reasonable
means to solve the three simultaneous issues above and if so, does it
seem like the most reasonable means?

Thanks,
Robin

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30 16:11                 ` Robin Holt
@ 2008-01-30 17:04                   ` Andrea Arcangeli
  2008-01-30 17:30                     ` Robin Holt
  2008-01-30 19:35                   ` Christoph Lameter
  1 sibling, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-30 17:04 UTC (permalink / raw)
  To: Robin Holt
  Cc: Christoph Lameter, Avi Kivity, Izik Eidus, Nick Piggin,
	kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra, steiner,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Wed, Jan 30, 2008 at 10:11:24AM -0600, Robin Holt wrote:
> > Robin, if you don't mind, could you please post or upload somewhere
> > your GPLv2 code that registers itself in Christoph's V2 notifiers? Or
> > is it top secret? I wouldn't mind to have a look so I can better
> > understand what's the exact reason you're sleeping besides attempting
> > GFP_KERNEL allocations. Thanks!
> 
> Dean is still actively working on updating the xpmem patch posted
> here a few months ago reworked for the mmu_notifiers.  I am sure
> we can give you a early look, but it is in a really rough state.
> 
> http://marc.info/?l=linux-mm&w=2&r=1&s=xpmem&q=t
> 
> The need to sleep comes from the fact that these PFNs are sent to other
> hosts on the same NUMA fabric which have direct access to the pages
> and then placed into remote process's page tables and then filled into
> their TLBs.  Our only means of communicating the recall is async.
> 
> I think I need to straighten this discussion out in my head a little bit.
> Am I correct in assuming Andrea's original patch set did not have any SMP
> race conditions for KVM?  If so, then we need to start looking at how to

Yes my last patch was SMP safe, stable and feature complete for KVM. I
tested it for 1 week on my smp workstation with real desktop load and
everything loaded, with 3G non-linux guest running on 2G of ram.

Now for whatever reason I adapted the KVM side to Christoph's V2/V3
and it hangs the moment it hits swap. However in the meantime I
changed test hardware, upgraded host to 2.6.24-hg, and upgraded kvm
kernel and userland. all patches applied cleanly (with a minor nit in
a .h include in V2 on top of current git). Swapping of regular tasks
on the test system is 100% solid or I wouldn't even wasting time
mentioning this. By code inspection I didn't expect a stability
regression or I wouldn't have chanced all variables at the same time
(taking the opportunity to move everything to bleeding edge while
moving to V2 turned out to be a bad idea). I already audited the mmu
notifiers a few times, infact I already went back to call
invalidate_page and age_page inside ptep_clear_flush/young in case the
page-pin wasn't enough to prevent the page to change under the sptes,
as I thought yesterday.

Christoph's V3 notably still misses the needed range flushes in mremap
for example, but that's not my problem.  (Jack instead will certainly
kernel crash due to the missing invalidate_page after ptep_clear_flush
in mremap, such an invalidate_page wasn't missing with my last patch)

I'm now going to run the same binaries that still are stable on my
workstation on the test system too, to rule out timings and hardware
differences.

> implement Christoph's and my changes in a safe fashion.  Andrea, I agree
> complete that our introduction of the range callouts have introduced
> SMP races.

I think for KVM basic swapping both V2 and V3 should be safe. V2 had
race conditions that would later break KSM yes, I fixed it and V3
should be already ok and I'm not testing KSM. This is all thanks to the
pin of the page in get_user_page that KVM does for every page mapped
in any spte.

> The three issues we need to simultaneously solve is revoking the remote
> page table/tlb information while still in a sleepable context and not
> having the remote faulters become out of sync with the granting process.
> Currently, I don't see a way to do that cleanly with a single callout.

Agreed.

> Could we consider doing a range-based recall and lock callout before
> clearing the processes page tables/TLBs, then use the _page or _range
> callouts from Andrea's patch to clear the mappings,  finally make a
> range-based unlock callout.  The mmu_notifier user would usually use ops
> for either the recall+lock/unlock family of callouts or the _page/_range
> family of callouts.

invalidate_page/age_page can return inside ptep_clear_flush/young and
Jack will need that too. Infact Jack will need an invalidate_page also
inside ptep_get_and_clear. And the range callout will be done always
in a sleeping context and it'll relay on the page-pin to be safe (when
details->i_mmap_lock != NULL invalidate_range it shouldn't be called
inside zap_page_range but before returning from
unmap_mapping_range_vma before cond_resched). This will make
everything a bit simpler and less prone to breakage IMHO, plus it'll
have a chance to work for Jack w/o page-pin without additional
cluttering of mm/*.c.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30  0:00               ` Andrea Arcangeli
  2008-01-30  0:05                 ` Andrea Arcangeli
  2008-01-30  0:20                 ` Christoph Lameter
@ 2008-01-30 16:11                 ` Robin Holt
  2008-01-30 17:04                   ` Andrea Arcangeli
  2008-01-30 19:35                   ` Christoph Lameter
  2 siblings, 2 replies; 101+ messages in thread
From: Robin Holt @ 2008-01-30 16:11 UTC (permalink / raw)
  To: Andrea Arcangeli, Christoph Lameter
  Cc: Christoph Lameter, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	steiner, linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

> Robin, if you don't mind, could you please post or upload somewhere
> your GPLv2 code that registers itself in Christoph's V2 notifiers? Or
> is it top secret? I wouldn't mind to have a look so I can better
> understand what's the exact reason you're sleeping besides attempting
> GFP_KERNEL allocations. Thanks!

Dean is still actively working on updating the xpmem patch posted
here a few months ago reworked for the mmu_notifiers.  I am sure
we can give you a early look, but it is in a really rough state.

http://marc.info/?l=linux-mm&w=2&r=1&s=xpmem&q=t

The need to sleep comes from the fact that these PFNs are sent to other
hosts on the same NUMA fabric which have direct access to the pages
and then placed into remote process's page tables and then filled into
their TLBs.  Our only means of communicating the recall is async.

I think I need to straighten this discussion out in my head a little bit.
Am I correct in assuming Andrea's original patch set did not have any SMP
race conditions for KVM?  If so, then we need to start looking at how to
implement Christoph's and my changes in a safe fashion.  Andrea, I agree
complete that our introduction of the range callouts have introduced
SMP races.

The three issues we need to simultaneously solve is revoking the remote
page table/tlb information while still in a sleepable context and not
having the remote faulters become out of sync with the granting process.
Currently, I don't see a way to do that cleanly with a single callout.

Could we consider doing a range-based recall and lock callout before
clearing the processes page tables/TLBs, then use the _page or _range
callouts from Andrea's patch to clear the mappings,  finally make a
range-based unlock callout.  The mmu_notifier user would usually use ops
for either the recall+lock/unlock family of callouts or the _page/_range
family of callouts.

Thanks,
Robin

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30 13:37                     ` Andrea Arcangeli
@ 2008-01-30 14:43                       ` Jack Steiner
  2008-01-30 19:41                         ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Jack Steiner @ 2008-01-30 14:43 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Christoph Lameter, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Wed, Jan 30, 2008 at 02:37:20PM +0100, Andrea Arcangeli wrote:
> On Tue, Jan 29, 2008 at 06:28:05PM -0600, Jack Steiner wrote:
> > On Tue, Jan 29, 2008 at 04:20:50PM -0800, Christoph Lameter wrote:
> > > On Wed, 30 Jan 2008, Andrea Arcangeli wrote:
> > > 
> > > > > invalidate_range after populate allows access to memory for which ptes 
> > > > > were zapped and the refcount was released.
> > > > 
> > > > The last refcount is released by the invalidate_range itself.
> > > 
> > > That is true for your implementation and to address Robin's issues. Jack: 
> > > Is that true for the GRU?
> > 
> > I'm not sure I understand the question. The GRU never (currently) takes
> > a reference on a page. It has no mechanism for tracking pages that
> > were exported to the external TLBs.
> 
> If you don't have a pin, then things like invalidate_range in
> remap_file_pages can't be safe as writes through the external TLBs can
> keep going on pages in the freelist. For you to be safe w/o a
> page-pin, you need to return in the direction of invalidate_page
> inside ptep_clear_flush (or anyway before
> page_cache_release/__free_page/put_page...). You're generally not safe
> with any invalidate_range that may run after the page pointed by the
> pte has been freed (or can be freed by the VM anytime because of being
> unpinned cache).

Yuck....

I see what you mean. I need to review to mail to see why this changed
but in the original discussions with Christoph, the invalidate_range
callouts were suppose to be made BEFORE the pages were put on the freelist.


--- jack

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30  0:28                   ` Jack Steiner
  2008-01-30  0:35                     ` Christoph Lameter
@ 2008-01-30 13:37                     ` Andrea Arcangeli
  2008-01-30 14:43                       ` Jack Steiner
  1 sibling, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-30 13:37 UTC (permalink / raw)
  To: Jack Steiner
  Cc: Christoph Lameter, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Tue, Jan 29, 2008 at 06:28:05PM -0600, Jack Steiner wrote:
> On Tue, Jan 29, 2008 at 04:20:50PM -0800, Christoph Lameter wrote:
> > On Wed, 30 Jan 2008, Andrea Arcangeli wrote:
> > 
> > > > invalidate_range after populate allows access to memory for which ptes 
> > > > were zapped and the refcount was released.
> > > 
> > > The last refcount is released by the invalidate_range itself.
> > 
> > That is true for your implementation and to address Robin's issues. Jack: 
> > Is that true for the GRU?
> 
> I'm not sure I understand the question. The GRU never (currently) takes
> a reference on a page. It has no mechanism for tracking pages that
> were exported to the external TLBs.

If you don't have a pin, then things like invalidate_range in
remap_file_pages can't be safe as writes through the external TLBs can
keep going on pages in the freelist. For you to be safe w/o a
page-pin, you need to return in the direction of invalidate_page
inside ptep_clear_flush (or anyway before
page_cache_release/__free_page/put_page...). You're generally not safe
with any invalidate_range that may run after the page pointed by the
pte has been freed (or can be freed by the VM anytime because of being
unpinned cache).

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30  0:59                     ` Andrea Arcangeli
@ 2008-01-30  8:26                       ` Peter Zijlstra
  0 siblings, 0 replies; 101+ messages in thread
From: Peter Zijlstra @ 2008-01-30  8:26 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Christoph Lameter, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, steiner,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins


On Wed, 2008-01-30 at 01:59 +0100, Andrea Arcangeli wrote:
> On Tue, Jan 29, 2008 at 04:22:46PM -0800, Christoph Lameter wrote:
> > That is only partially true. pte are created wronly in order to track 
> > dirty state these days. The first write will lead to a fault that switches 
> > the pte to writable. When the page undergoes writeback the page again 
> > becomes write protected. Thus our need to effectively deal with 
> > page_mkclean.
> 
> Well I was talking about anonymous memory.

Just to be absolutely clear on this (I lost track of what exactly we are
talking about here), nonlinear mappings no not do the dirty accounting,
and are not allowed on a backing store that would require dirty
accounting.




^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30  0:22                   ` Christoph Lameter
@ 2008-01-30  0:59                     ` Andrea Arcangeli
  2008-01-30  8:26                       ` Peter Zijlstra
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-30  0:59 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, Jan 29, 2008 at 04:22:46PM -0800, Christoph Lameter wrote:
> That is only partially true. pte are created wronly in order to track 
> dirty state these days. The first write will lead to a fault that switches 
> the pte to writable. When the page undergoes writeback the page again 
> becomes write protected. Thus our need to effectively deal with 
> page_mkclean.

Well I was talking about anonymous memory.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30  0:28                   ` Jack Steiner
@ 2008-01-30  0:35                     ` Christoph Lameter
  2008-01-30 13:37                     ` Andrea Arcangeli
  1 sibling, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30  0:35 UTC (permalink / raw)
  To: Jack Steiner
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Tue, 29 Jan 2008, Jack Steiner wrote:

> > That is true for your implementation and to address Robin's issues. Jack: 
> > Is that true for the GRU?
> 
> I'm not sure I understand the question. The GRU never (currently) takes
> a reference on a page. It has no mechanism for tracking pages that
> were exported to the external TLBs.

Thats what I was looking for. Thanks. KVM takes a refcount and so does 
XPmem.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 23:43                 ` Andrea Arcangeli
@ 2008-01-30  0:34                   ` Christoph Lameter
  0 siblings, 0 replies; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30  0:34 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Wed, 30 Jan 2008, Andrea Arcangeli wrote:

> > A user space spinlock plays into this??? That is irrelevant to the kernel. 
> > And we are discussing "your" placement of the invalidate_range not mine.
> 
> With "my" code, invalidate_range wasn't placed there at all, my
> modification to ptep_clear_flush already covered it in a automatic
> way, grep from the word fremap in my latest patch you won't find it,
> like you won't find any change to do_wp_page. Not sure why you keep
> thinking I added those invalidate_range when infact you did.

Well you moved the code at minimum. Hmmm... according 
http://marc.info/?l=linux-kernel&m=120114755620891&w=2 it was Robin.

> The user space spinlock plays also in declaring rdtscp unworkable to
> provide a monotone vgettimeofday w/o kernel locking.

No idea what you are talking about.

> My patch by calling invalidate_page inside ptep_clear_flush guaranteed
> that both the thread writing through sptes and the thread writing
> through linux ptes, couldn't possibly simultaneously write to two
> different physical pages.

But then the ptep_clear_flush will issue invalidate_page() for ranges 
that were already covered by invalidate_range(). There are multiple calls 
to clear the same spte.
>
> Your patch allows the thread writing through linux-pte to write to a
> new populated page while the old thread writing through sptes still
> writes to the old page. Is that safe? I don't know for sure. The fact
> the physical page backing the virtual address could change back and
> forth, perhaps invalidates the theory that somebody could possibly do
> some useful locking out of it relaying on all threads seeing the same
> physical page at the same time.

This is referrring to the remap issue not do_wp_page right?

> Actually above I was describing remap_file_pages not do_wp_page.

Ok.

The serialization of remap_file_pages does not seem that critical since we 
only take a read lock on mmap_sem here. There may already be concurrent 
access to pages from other processors while the ptes are remapped. So 
there is already some overlap.

We could take mmap_sem there writably and keep it writably for the case 
that we have an mmu notifier in the mm.



^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30  0:20                 ` Christoph Lameter
@ 2008-01-30  0:28                   ` Jack Steiner
  2008-01-30  0:35                     ` Christoph Lameter
  2008-01-30 13:37                     ` Andrea Arcangeli
  0 siblings, 2 replies; 101+ messages in thread
From: Jack Steiner @ 2008-01-30  0:28 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Andrea Arcangeli, Robin Holt, Avi Kivity, Izik Eidus,
	Nick Piggin, kvm-devel, Benjamin Herrenschmidt, Peter Zijlstra,
	linux-kernel, linux-mm, daniel.blueman, Hugh Dickins

On Tue, Jan 29, 2008 at 04:20:50PM -0800, Christoph Lameter wrote:
> On Wed, 30 Jan 2008, Andrea Arcangeli wrote:
> 
> > > invalidate_range after populate allows access to memory for which ptes 
> > > were zapped and the refcount was released.
> > 
> > The last refcount is released by the invalidate_range itself.
> 
> That is true for your implementation and to address Robin's issues. Jack: 
> Is that true for the GRU?

I'm not sure I understand the question. The GRU never (currently) takes
a reference on a page. It has no mechanism for tracking pages that
were exported to the external TLBs.

--- jack

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30  0:05                 ` Andrea Arcangeli
@ 2008-01-30  0:22                   ` Christoph Lameter
  2008-01-30  0:59                     ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30  0:22 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Wed, 30 Jan 2008, Andrea Arcangeli wrote:

> On Wed, Jan 30, 2008 at 01:00:39AM +0100, Andrea Arcangeli wrote:
> > get_user_pages, regular linux writes don't fault unless it's
> > explicitly writeprotect, which is mandatory in a few archs, x86 not).
> 
> actually get_user_pages doesn't fault either but it calls into
> set_page_dirty, however get_user_pages (unlike a userland-write) at
> least requires mmap_sem in read mode and the PT lock as serialization,
> userland writes don't, they just go ahead and mark the pte in hardware
> w/o faults. Anyway anonymous memory these days always mapped with
> dirty bit set regardless, even for read-faults, after Nick finally
> rightfully cleaned up the zero-page trick.

That is only partially true. pte are created wronly in order to track 
dirty state these days. The first write will lead to a fault that switches 
the pte to writable. When the page undergoes writeback the page again 
becomes write protected. Thus our need to effectively deal with 
page_mkclean.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30  0:00               ` Andrea Arcangeli
  2008-01-30  0:05                 ` Andrea Arcangeli
@ 2008-01-30  0:20                 ` Christoph Lameter
  2008-01-30  0:28                   ` Jack Steiner
  2008-01-30 16:11                 ` Robin Holt
  2 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-30  0:20 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Wed, 30 Jan 2008, Andrea Arcangeli wrote:

> > invalidate_range after populate allows access to memory for which ptes 
> > were zapped and the refcount was released.
> 
> The last refcount is released by the invalidate_range itself.

That is true for your implementation and to address Robin's issues. Jack: 
Is that true for the GRU?


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-30  0:00               ` Andrea Arcangeli
@ 2008-01-30  0:05                 ` Andrea Arcangeli
  2008-01-30  0:22                   ` Christoph Lameter
  2008-01-30  0:20                 ` Christoph Lameter
  2008-01-30 16:11                 ` Robin Holt
  2 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-30  0:05 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Wed, Jan 30, 2008 at 01:00:39AM +0100, Andrea Arcangeli wrote:
> get_user_pages, regular linux writes don't fault unless it's
> explicitly writeprotect, which is mandatory in a few archs, x86 not).

actually get_user_pages doesn't fault either but it calls into
set_page_dirty, however get_user_pages (unlike a userland-write) at
least requires mmap_sem in read mode and the PT lock as serialization,
userland writes don't, they just go ahead and mark the pte in hardware
w/o faults. Anyway anonymous memory these days always mapped with
dirty bit set regardless, even for read-faults, after Nick finally
rightfully cleaned up the zero-page trick.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 22:39             ` Christoph Lameter
@ 2008-01-30  0:00               ` Andrea Arcangeli
  2008-01-30  0:05                 ` Andrea Arcangeli
                                   ` (2 more replies)
  0 siblings, 3 replies; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-30  0:00 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, Jan 29, 2008 at 02:39:00PM -0800, Christoph Lameter wrote:
> If it does not run in write mode then concurrent faults are permissible 
> while we remap pages. Weird. Maybe we better handle this like individual
> page operations? Put the invalidate_page back into zap_pte. But then there 
> would be no callback w/o lock as required by Robin. Doing the 

The Robin requirements and the need to schedule are the source of the
complications indeed.

I posted all the KVM patches using mmu notifiers, today I reposted the
ones to work with your V2 (which crashes my host unlike my last
simpler mmu notifier patch but I also changed a few other variable
besides your mmu notifier changes, so I can't yet be sure it's a bug
in your V2, and the SMP regressions I fixed so far sure can't explain
the crashes because my KVM setup could never run in do_wp_page nor
remap_file_pages so it's something else I need to find ASAP).

Robin, if you don't mind, could you please post or upload somewhere
your GPLv2 code that registers itself in Christoph's V2 notifiers? Or
is it top secret? I wouldn't mind to have a look so I can better
understand what's the exact reason you're sleeping besides attempting
GFP_KERNEL allocations. Thanks!

> invalidate_range after populate allows access to memory for which ptes 
> were zapped and the refcount was released.

The last refcount is released by the invalidate_range itself.
 
> > All pins are gone by the time invalidate_page/range returns. But there
> > is no critical section between invalidate_page and the _later_
> > ptep_clear_flush. So get_user_pages is free to run and take the PT
> > lock before the ptep_clear_flush, find the linux pte still
> > instantiated, and to create a new spte, before ptep_clear_flush runs.
> 
> Hmmm... Right. Did not consider get_user_pages. A write to the page that 
> is not marked dirty would typically require a fault that will serialize.

The pte is already marked dirty (and this is the case only for
get_user_pages, regular linux writes don't fault unless it's
explicitly writeprotect, which is mandatory in a few archs, x86 not).

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 22:55               ` Christoph Lameter
@ 2008-01-29 23:43                 ` Andrea Arcangeli
  2008-01-30  0:34                   ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-29 23:43 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, Jan 29, 2008 at 02:55:56PM -0800, Christoph Lameter wrote:
> On Tue, 29 Jan 2008, Andrea Arcangeli wrote:
> 
> > But now I think there may be an issue with a third thread that may
> > show unsafe the removal of invalidate_page from ptep_clear_flush.
> > 
> > A third thread writing to a page through the linux-pte and the guest
> > VM writing to the same page through the sptes, will be writing on the
> > same physical page concurrently and using an userspace spinlock w/o
> > ever entering the kernel. With your patch that invalidate_range after
> > dropping the PT lock, the third thread may start writing on the new
> > page, when the guest is still writing to the old page through the
> > sptes. While this couldn't happen with my patch.
> 
> A user space spinlock plays into this??? That is irrelevant to the kernel. 
> And we are discussing "your" placement of the invalidate_range not mine.

With "my" code, invalidate_range wasn't placed there at all, my
modification to ptep_clear_flush already covered it in a automatic
way, grep from the word fremap in my latest patch you won't find it,
like you won't find any change to do_wp_page. Not sure why you keep
thinking I added those invalidate_range when infact you did.

The user space spinlock plays also in declaring rdtscp unworkable to
provide a monotone vgettimeofday w/o kernel locking.

My patch by calling invalidate_page inside ptep_clear_flush guaranteed
that both the thread writing through sptes and the thread writing
through linux ptes, couldn't possibly simultaneously write to two
different physical pages.

Your patch allows the thread writing through linux-pte to write to a
new populated page while the old thread writing through sptes still
writes to the old page. Is that safe? I don't know for sure. The fact
the physical page backing the virtual address could change back and
forth, perhaps invalidates the theory that somebody could possibly do
some useful locking out of it relaying on all threads seeing the same
physical page at the same time.

Anyway as long as invalidate_page/range happens after ptep_clear_flush
things are mostly ok.

> This is the scenario that I described before. You just need two threads.
> One thread is in do_wp_page and the other is writing through the spte. 
> We are in do_wp_page. Meaning the page is not writable. The writer will 

Actually above I was describing remap_file_pages not do_wp_page.

> have to take fault which will properly serialize access. It a bug if the 
> spte would allow write.

In that scenario because write is forbidden (unlike remap_file_pages)
like you said things should be ok. The spte reader will eventually see
the updates happening in the new page, as long as the spte invalidate
happens after ptep_clear_flush (i.e. with my incremental fix applied
to your code, or with my latest patch).

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 22:35             ` Andrea Arcangeli
@ 2008-01-29 22:55               ` Christoph Lameter
  2008-01-29 23:43                 ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-29 22:55 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, 29 Jan 2008, Andrea Arcangeli wrote:

> But now I think there may be an issue with a third thread that may
> show unsafe the removal of invalidate_page from ptep_clear_flush.
> 
> A third thread writing to a page through the linux-pte and the guest
> VM writing to the same page through the sptes, will be writing on the
> same physical page concurrently and using an userspace spinlock w/o
> ever entering the kernel. With your patch that invalidate_range after
> dropping the PT lock, the third thread may start writing on the new
> page, when the guest is still writing to the old page through the
> sptes. While this couldn't happen with my patch.

A user space spinlock plays into this??? That is irrelevant to the kernel. 
And we are discussing "your" placement of the invalidate_range not mine.

This is the scenario that I described before. You just need two threads.
One thread is in do_wp_page and the other is writing through the spte. 
We are in do_wp_page. Meaning the page is not writable. The writer will 
have to take fault which will properly serialize access. It a bug if the 
spte would allow write.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 22:02           ` Andrea Arcangeli
@ 2008-01-29 22:39             ` Christoph Lameter
  2008-01-30  0:00               ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-29 22:39 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

n Tue, 29 Jan 2008, Andrea Arcangeli wrote:

> hmm, "there" where? When I said it was taken in readonly mode I meant
> for the quoted code (it would be at the top if it wasn't cut), so I
> quote below again:
> 
> > > +   mmu_notifier(invalidate_range, mm, address,
> > > +                           address + PAGE_SIZE - 1, 0);
> > >     page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
> > >     if (likely(pte_same(*page_table, orig_pte))) {
> > >             if (old_page) {
> 
> The "there" for me was do_wp_page.

Maybe we better focus on one call at a time?

> Even for the code you quoted in freemap.c, the has_write_lock is set
> to 1 _only_ for the very first time you call sys_remap_file_pages on a
> VMA. Only the transition of the VMA between linear to nonlinear
> requires the mmap in write mode. So you can be sure all freemap code
> 99% of the time is populating (overwriting) already present ptes with
> only the mmap_sem in readonly mode like do_wp_page. It would be
> unnecessary to populate the nonlinear range with the mmap in write
> mode. Only the "vma" mangling requires the mmap_sem in write mode, the
> pte modifications only requires the PT_lock + mmap_sem in read mode.
> 
> Effectively the first invocation of populate_range runs with the
> mmap_sem in write mode, I wonder why, there seem to be no good reason
> for that. I guess it's a bit that should be optimized, by calling
> downgrade_write before calling populate_range even for the first time
> the vma switches from linear to nonlinear (after the vma has been
> fully updated to the new status). But for sure all later invocations
> runs populate_range with the semaphore readonly like the rest of the
> VM does when instantiating ptes in the page faults.

If it does not run in write mode then concurrent faults are permissible 
while we remap pages. Weird. Maybe we better handle this like individual
page operations? Put the invalidate_page back into zap_pte. But then there 
would be no callback w/o lock as required by Robin. Doing the 
invalidate_range after populate allows access to memory for which ptes 
were zapped and the refcount was released.

> All pins are gone by the time invalidate_page/range returns. But there
> is no critical section between invalidate_page and the _later_
> ptep_clear_flush. So get_user_pages is free to run and take the PT
> lock before the ptep_clear_flush, find the linux pte still
> instantiated, and to create a new spte, before ptep_clear_flush runs.

Hmmm... Right. Did not consider get_user_pages. A write to the page that 
is not marked dirty would typically require a fault that will serialize.


^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 21:53           ` Christoph Lameter
@ 2008-01-29 22:35             ` Andrea Arcangeli
  2008-01-29 22:55               ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-29 22:35 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, Jan 29, 2008 at 01:53:05PM -0800, Christoph Lameter wrote:
> On Tue, 29 Jan 2008, Andrea Arcangeli wrote:
> 
> > > We invalidate the range *after* populating it? Isnt it okay to establish 
> > > references while populate_range() runs?
> > 
> > It's not ok because that function can very well overwrite existing and
> > present ptes (it's actually the nonlinear common case fast path for
> > db). With your code the sptes created between invalidate_range and
> > populate_range, will keep pointing forever to the old physical page
> > instead of the newly populated one.
> 
> Seems though that the mmap_sem is taken for regular vmas writably and will 
> hold off new mappings.

It's taken writable due to the code being inefficient the first time,
all later times remap_populate_range overwrites ptes with the mmap_sem
in readonly mode (finally rightfully so). The first remap_file_pages I
guess it's irrelevant to optimize, the whole point of nonlinear is to
call remap_file_pages zillon of times on the same vma, overwriting
present ptes the whole time, so if the first time the mutex is not
readonly it probably doesn't make a difference.

get_user_pages invoked by the kvm spte-fault, can happen between
invalidate_range and populate_range. If it can't happen, for sure
nobody pointed out a good reason why it can't happen. The kvm page
faults as well rightfully only takes the mmap_sem in readonly mode, so
get_user_pages is only called internally to gfn_to_page with the
readonly semaphore.

With my approach ptep_clear_flush was not only invalidating sptes
after ptep_clear_flush, but it was also invalidating them inside the
PT lock, so it was totally obvious there could be no race vs
get_user_pages.

> > I'm also asking myself if it's a smp race not to call
> > mmu_notifier(invalidate_page) between ptep_clear_flush and set_pte_at
> > in install_file_pte. Probably not because the guest VM running in a
> > different thread would need to serialize outside the install_file_pte
> > code with the task running install_file_pte, if it wants to be sure to
> > write either all its data to the old or the new page. Certainly doing
> > the invalidate_page inside the PT lock was obviously safe but I hope
> > this is safe and this can accommodate your needs too.
> 
> But that would be doing two invalidates on one pte. One range and one page 
> invalidate.

Yes, but it would have been micro-optimized later if you really cared,
by simply changing ptep_clear_flush to __ptep_clear_flush, no big
deal. Definitely all methods must be robust about them being called
multiple times, even if the rmap finds no spte mapping such host
virtual address.

> Hmmm... So we could only do an invalidate_page here? Drop the strange 
> invalidate_range()?

That's a question you should answer.

> > > > @@ -1676,6 +1674,8 @@ gotten:
> > > >  		page_cache_release(old_page);
> > > >  unlock:
> > > >  	pte_unmap_unlock(page_table, ptl);
> > > > +	mmu_notifier(invalidate_range, mm, address,
> > > > +				address + PAGE_SIZE - 1, 0);
> > > >  	if (dirty_page) {
> > > >  		if (vma->vm_file)
> > > >  			file_update_time(vma->vm_file);
> > > 
> > > Now we invalidate the page after the transaction is complete. This means 
> > > external pte can persist while we change the pte? Possibly even dirty the 
> > > page?
> > 
> > Yes, and the only reason this can be safe is for the reason explained
> > at the top of the email, if the other cpu wants to serialize to be
> > sure to write in the "new" page, it has to serialize with the
> > page-fault but to serialize it has to wait the page fault to return
> > (example: we're not going to call futex code until the page fault
> > returns).
> 
> Serialize how? mmap_sem?

No, that's a different angle.

But now I think there may be an issue with a third thread that may
show unsafe the removal of invalidate_page from ptep_clear_flush.

A third thread writing to a page through the linux-pte and the guest
VM writing to the same page through the sptes, will be writing on the
same physical page concurrently and using an userspace spinlock w/o
ever entering the kernel. With your patch that invalidate_range after
dropping the PT lock, the third thread may start writing on the new
page, when the guest is still writing to the old page through the
sptes. While this couldn't happen with my patch.

So really at the light of the third thread, it seems your approach is
smp racey and ptep_clear_flush should invalidate_page as last thing
before returning. My patch was enforcing that ptep_clear_flush would
stop the third thread in a linux page fault, and to drop the spte,
before the new mapping could be instantiated in both the linux pte and
in the sptes. The PT lock provided the needed serialization. This
ensured the third thread and the guest VM would always write on the
same physical page even if the first thread runs a flood of
remap_file_pages on that same page moving it around the pagecache. So
it seems I found a unfixable smp race in pretending to invalidate in a
sleeping place.

Perhaps you want to change the PT lock to a mutex instead of a
spinlock, that may be your only chance to sleep while maintaining 100%
memory coherency with threads.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 21:35         ` Christoph Lameter
@ 2008-01-29 22:02           ` Andrea Arcangeli
  2008-01-29 22:39             ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-29 22:02 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, Jan 29, 2008 at 01:35:58PM -0800, Christoph Lameter wrote:
> On Tue, 29 Jan 2008, Andrea Arcangeli wrote:
> 
> > > It seems to be okay to invalidate range if you hold mmap_sem writably. In 
> > > that case no additional faults can happen that would create new ptes.
> > 
> > In that place the mmap_sem is taken but in readonly mode. I never rely
> > on the mmap_sem in the mmu notifier methods. Not invoking the notifier
> 
> Well it seems that we have to rely on mmap_sem otherwise concurrent faults 
> can occur. The mmap_sem seems to be acquired for write there.
      	     	 	  	      	       	   	 ^^^^^
> 
>               if (!has_write_lock) {
>                         up_read(&mm->mmap_sem);
>                         down_write(&mm->mmap_sem);
>                         has_write_lock = 1;
>                         goto retry;
>                 }


hmm, "there" where? When I said it was taken in readonly mode I meant
for the quoted code (it would be at the top if it wasn't cut), so I
quote below again:

> > +   mmu_notifier(invalidate_range, mm, address,
> > +                           address + PAGE_SIZE - 1, 0);
> >     page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
> >     if (likely(pte_same(*page_table, orig_pte))) {
> >             if (old_page) {

The "there" for me was do_wp_page.

Even for the code you quoted in freemap.c, the has_write_lock is set
to 1 _only_ for the very first time you call sys_remap_file_pages on a
VMA. Only the transition of the VMA between linear to nonlinear
requires the mmap in write mode. So you can be sure all freemap code
99% of the time is populating (overwriting) already present ptes with
only the mmap_sem in readonly mode like do_wp_page. It would be
unnecessary to populate the nonlinear range with the mmap in write
mode. Only the "vma" mangling requires the mmap_sem in write mode, the
pte modifications only requires the PT_lock + mmap_sem in read mode.

Effectively the first invocation of populate_range runs with the
mmap_sem in write mode, I wonder why, there seem to be no good reason
for that. I guess it's a bit that should be optimized, by calling
downgrade_write before calling populate_range even for the first time
the vma switches from linear to nonlinear (after the vma has been
fully updated to the new status). But for sure all later invocations
runs populate_range with the semaphore readonly like the rest of the
VM does when instantiating ptes in the page faults.

> > before releasing the PT lock adds quite some uncertainty on the smp
> > safety of the spte invalidates, because the pte may be unmapped and
> > remapped by a minor fault before invalidate_range is invoked, but I
> > didn't figure out a kernel crashing race yet thanks to the pin we take
> > through get_user_pages (and only thanks to it). The requirement is
> > that invalidate_range is invoked after the last ptep_clear_flush or it
> > leaks pins that's why I had to move it at the end.
>  
> So "pins" means a reference count right? I still do not get why you 

Yes.

> have refcount problems. You take a refcount when you export the page 
> through KVM and then drop the refcount in invalidate page right?

Yes.

> So you walk through the KVM ptes and drop the refcount for each spte you 
> encounter?

Yes.

All pins are gone by the time invalidate_page/range returns. But there
is no critical section between invalidate_page and the _later_
ptep_clear_flush. So get_user_pages is free to run and take the PT
lock before the ptep_clear_flush, find the linux pte still
instantiated, and to create a new spte, before ptep_clear_flush runs.

Think of why the tlb flushes are being called at the end of
ptep_clear_flush. The mmu notifier invalidate has to be called after
for the exact same reason.

Perhaps somebody else should explain this, I started exposing this
smp race the moment after I've seen the backwards ordering being
proposed in export-notifier-v1, sorry if I'm not clear enough.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 21:36         ` Andrea Arcangeli
@ 2008-01-29 21:53           ` Christoph Lameter
  2008-01-29 22:35             ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-29 21:53 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, 29 Jan 2008, Andrea Arcangeli wrote:

> > We invalidate the range *after* populating it? Isnt it okay to establish 
> > references while populate_range() runs?
> 
> It's not ok because that function can very well overwrite existing and
> present ptes (it's actually the nonlinear common case fast path for
> db). With your code the sptes created between invalidate_range and
> populate_range, will keep pointing forever to the old physical page
> instead of the newly populated one.

Seems though that the mmap_sem is taken for regular vmas writably and will 
hold off new mappings.

> I'm also asking myself if it's a smp race not to call
> mmu_notifier(invalidate_page) between ptep_clear_flush and set_pte_at
> in install_file_pte. Probably not because the guest VM running in a
> different thread would need to serialize outside the install_file_pte
> code with the task running install_file_pte, if it wants to be sure to
> write either all its data to the old or the new page. Certainly doing
> the invalidate_page inside the PT lock was obviously safe but I hope
> this is safe and this can accommodate your needs too.

But that would be doing two invalidates on one pte. One range and one page 
invalidate.

> > > diff --git a/mm/memory.c b/mm/memory.c
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -1639,8 +1639,6 @@ gotten:
> > >  	/*
> > >  	 * Re-check the pte - we dropped the lock
> > >  	 */
> > > -	mmu_notifier(invalidate_range, mm, address,
> > > -				address + PAGE_SIZE - 1, 0);
> > >  	page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
> > >  	if (likely(pte_same(*page_table, orig_pte))) {
> > >  		if (old_page) {
> > 
> > What we did is to invalidate the page (?!) before taking the pte lock. In 
> > the lock we replace the pte to point to another page. This means that we 
> > need to clear stale information. So we zap it before. If another reference 
> > is established after taking the spinlock then the pte contents have 
> > changed at the cirtical section fails.
> > 
> > Before the critical section starts we have gotten an extra refcount on the 
> > original page so the page cannot vanish from under us.
> 
> The problem is the missing invalidate_page/range _after_
> ptep_clear_flush. If a spte is built between invalidate_range and
> pte_offset_map_lock, it will remain pointing to the old page
> forever. Nothing will be called to invalidate that stale spte built
> between invalidate_page/range and ptep_clear_flush. This is why for
> the last few days I kept saying the mmu notifiers have to be invoked
> _after_ ptep_clear_flush and never before (remember the export
> notifier?). No idea how you can deal with this in your code, certainly
> for KVM sptes that's backwards and unworkable ordering of operation
> (exactly as backwards are doing the tlb flush before pte_clear in
> ptep_clear_flush, think spte as a tlb, you can't flush the tlb before
> clearing/updating the pte or it's smp unsafe).

Hmmm... So we could only do an invalidate_page here? Drop the strange 
invalidate_range()?

> 
> > > @@ -1676,6 +1674,8 @@ gotten:
> > >  		page_cache_release(old_page);
> > >  unlock:
> > >  	pte_unmap_unlock(page_table, ptl);
> > > +	mmu_notifier(invalidate_range, mm, address,
> > > +				address + PAGE_SIZE - 1, 0);
> > >  	if (dirty_page) {
> > >  		if (vma->vm_file)
> > >  			file_update_time(vma->vm_file);
> > 
> > Now we invalidate the page after the transaction is complete. This means 
> > external pte can persist while we change the pte? Possibly even dirty the 
> > page?
> 
> Yes, and the only reason this can be safe is for the reason explained
> at the top of the email, if the other cpu wants to serialize to be
> sure to write in the "new" page, it has to serialize with the
> page-fault but to serialize it has to wait the page fault to return
> (example: we're not going to call futex code until the page fault
> returns).

Serialize how? mmap_sem?
 

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 20:30       ` Christoph Lameter
@ 2008-01-29 21:36         ` Andrea Arcangeli
  2008-01-29 21:53           ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-29 21:36 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, Jan 29, 2008 at 12:30:06PM -0800, Christoph Lameter wrote:
> On Tue, 29 Jan 2008, Andrea Arcangeli wrote:
> 
> > diff --git a/mm/fremap.c b/mm/fremap.c
> > --- a/mm/fremap.c
> > +++ b/mm/fremap.c
> > @@ -212,8 +212,8 @@ asmlinkage long sys_remap_file_pages(uns
> >  		spin_unlock(&mapping->i_mmap_lock);
> >  	}
> >  
> > +	err = populate_range(mm, vma, start, size, pgoff);
> >  	mmu_notifier(invalidate_range, mm, start, start + size, 0);
> > -	err = populate_range(mm, vma, start, size, pgoff);
> >  	if (!err && !(flags & MAP_NONBLOCK)) {
> >  		if (unlikely(has_write_lock)) {
> >  			downgrade_write(&mm->mmap_sem);
> 
> We invalidate the range *after* populating it? Isnt it okay to establish 
> references while populate_range() runs?

It's not ok because that function can very well overwrite existing and
present ptes (it's actually the nonlinear common case fast path for
db). With your code the sptes created between invalidate_range and
populate_range, will keep pointing forever to the old physical page
instead of the newly populated one.

I'm also asking myself if it's a smp race not to call
mmu_notifier(invalidate_page) between ptep_clear_flush and set_pte_at
in install_file_pte. Probably not because the guest VM running in a
different thread would need to serialize outside the install_file_pte
code with the task running install_file_pte, if it wants to be sure to
write either all its data to the old or the new page. Certainly doing
the invalidate_page inside the PT lock was obviously safe but I hope
this is safe and this can accommodate your needs too.

> > diff --git a/mm/memory.c b/mm/memory.c
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1639,8 +1639,6 @@ gotten:
> >  	/*
> >  	 * Re-check the pte - we dropped the lock
> >  	 */
> > -	mmu_notifier(invalidate_range, mm, address,
> > -				address + PAGE_SIZE - 1, 0);
> >  	page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
> >  	if (likely(pte_same(*page_table, orig_pte))) {
> >  		if (old_page) {
> 
> What we did is to invalidate the page (?!) before taking the pte lock. In 
> the lock we replace the pte to point to another page. This means that we 
> need to clear stale information. So we zap it before. If another reference 
> is established after taking the spinlock then the pte contents have 
> changed at the cirtical section fails.
> 
> Before the critical section starts we have gotten an extra refcount on the 
> original page so the page cannot vanish from under us.

The problem is the missing invalidate_page/range _after_
ptep_clear_flush. If a spte is built between invalidate_range and
pte_offset_map_lock, it will remain pointing to the old page
forever. Nothing will be called to invalidate that stale spte built
between invalidate_page/range and ptep_clear_flush. This is why for
the last few days I kept saying the mmu notifiers have to be invoked
_after_ ptep_clear_flush and never before (remember the export
notifier?). No idea how you can deal with this in your code, certainly
for KVM sptes that's backwards and unworkable ordering of operation
(exactly as backwards are doing the tlb flush before pte_clear in
ptep_clear_flush, think spte as a tlb, you can't flush the tlb before
clearing/updating the pte or it's smp unsafe).

> > @@ -1676,6 +1674,8 @@ gotten:
> >  		page_cache_release(old_page);
> >  unlock:
> >  	pte_unmap_unlock(page_table, ptl);
> > +	mmu_notifier(invalidate_range, mm, address,
> > +				address + PAGE_SIZE - 1, 0);
> >  	if (dirty_page) {
> >  		if (vma->vm_file)
> >  			file_update_time(vma->vm_file);
> 
> Now we invalidate the page after the transaction is complete. This means 
> external pte can persist while we change the pte? Possibly even dirty the 
> page?

Yes, and the only reason this can be safe is for the reason explained
at the top of the email, if the other cpu wants to serialize to be
sure to write in the "new" page, it has to serialize with the
page-fault but to serialize it has to wait the page fault to return
(example: we're not going to call futex code until the page fault
returns).

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 21:17       ` Andrea Arcangeli
@ 2008-01-29 21:35         ` Christoph Lameter
  2008-01-29 22:02           ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-29 21:35 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, 29 Jan 2008, Andrea Arcangeli wrote:

> > It seems to be okay to invalidate range if you hold mmap_sem writably. In 
> > that case no additional faults can happen that would create new ptes.
> 
> In that place the mmap_sem is taken but in readonly mode. I never rely
> on the mmap_sem in the mmu notifier methods. Not invoking the notifier

Well it seems that we have to rely on mmap_sem otherwise concurrent faults 
can occur. The mmap_sem seems to be acquired for write there.

              if (!has_write_lock) {
                        up_read(&mm->mmap_sem);
                        down_write(&mm->mmap_sem);
                        has_write_lock = 1;
                        goto retry;
                }


> before releasing the PT lock adds quite some uncertainty on the smp
> safety of the spte invalidates, because the pte may be unmapped and
> remapped by a minor fault before invalidate_range is invoked, but I
> didn't figure out a kernel crashing race yet thanks to the pin we take
> through get_user_pages (and only thanks to it). The requirement is
> that invalidate_range is invoked after the last ptep_clear_flush or it
> leaks pins that's why I had to move it at the end.
 
So "pins" means a reference count right? I still do not get why you 
have refcount problems. You take a refcount when you export the page 
through KVM and then drop the refcount in invalidate page right?

So you walk through the KVM ptes and drop the refcount for each spte you 
encounter?



^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 19:55     ` Christoph Lameter
@ 2008-01-29 21:17       ` Andrea Arcangeli
  2008-01-29 21:35         ` Christoph Lameter
  0 siblings, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-29 21:17 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, Jan 29, 2008 at 11:55:10AM -0800, Christoph Lameter wrote:
> I am not sure. AFAICT you wrote that code.

Actually I didn't need to change a single line in do_wp_page because
ptep_clear_flush was already doing everything transparently for
me. This was the memory.c part of my last patch I posted, it only
touches zap_page_range, remap_pfn_range and apply_to_page_range.

diff --git a/mm/memory.c b/mm/memory.c
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -889,6 +889,7 @@ unsigned long zap_page_range(struct vm_a
 	end = unmap_vmas(&tlb, vma, address, end, &nr_accounted, details);
 	if (tlb)
 		tlb_finish_mmu(tlb, address, end);
+	mmu_notifier(invalidate_range, mm, address, end);
 	return end;
 }
 
@@ -1317,7 +1318,7 @@ int remap_pfn_range(struct vm_area_struc
 {
 	pgd_t *pgd;
 	unsigned long next;
-	unsigned long end = addr + PAGE_ALIGN(size);
+	unsigned long start = addr, end = addr + PAGE_ALIGN(size);
 	struct mm_struct *mm = vma->vm_mm;
 	int err;
 
@@ -1358,6 +1359,7 @@ int remap_pfn_range(struct vm_area_struc
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
+	mmu_notifier(invalidate_range, mm, start, end);
 	return err;
 }
 EXPORT_SYMBOL(remap_pfn_range);
@@ -1441,7 +1443,7 @@ int apply_to_page_range(struct mm_struct
 {
 	pgd_t *pgd;
 	unsigned long next;
-	unsigned long end = addr + size;
+	unsigned long start = addr, end = addr + size;
 	int err;
 
 	BUG_ON(addr >= end);
@@ -1452,6 +1454,7 @@ int apply_to_page_range(struct mm_struct
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
+	mmu_notifier(invalidate_range, mm, start, end);
 	return err;
 }
 EXPORT_SYMBOL_GPL(apply_to_page_range);

> It seems to be okay to invalidate range if you hold mmap_sem writably. In 
> that case no additional faults can happen that would create new ptes.

In that place the mmap_sem is taken but in readonly mode. I never rely
on the mmap_sem in the mmu notifier methods. Not invoking the notifier
before releasing the PT lock adds quite some uncertainty on the smp
safety of the spte invalidates, because the pte may be unmapped and
remapped by a minor fault before invalidate_range is invoked, but I
didn't figure out a kernel crashing race yet thanks to the pin we take
through get_user_pages (and only thanks to it). The requirement is
that invalidate_range is invoked after the last ptep_clear_flush or it
leaks pins that's why I had to move it at the end.

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 18:28     ` Andrea Arcangeli
@ 2008-01-29 20:30       ` Christoph Lameter
  2008-01-29 21:36         ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-29 20:30 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, 29 Jan 2008, Andrea Arcangeli wrote:

> diff --git a/mm/fremap.c b/mm/fremap.c
> --- a/mm/fremap.c
> +++ b/mm/fremap.c
> @@ -212,8 +212,8 @@ asmlinkage long sys_remap_file_pages(uns
>  		spin_unlock(&mapping->i_mmap_lock);
>  	}
>  
> +	err = populate_range(mm, vma, start, size, pgoff);
>  	mmu_notifier(invalidate_range, mm, start, start + size, 0);
> -	err = populate_range(mm, vma, start, size, pgoff);
>  	if (!err && !(flags & MAP_NONBLOCK)) {
>  		if (unlikely(has_write_lock)) {
>  			downgrade_write(&mm->mmap_sem);

We invalidate the range *after* populating it? Isnt it okay to establish 
references while populate_range() runs?

> diff --git a/mm/memory.c b/mm/memory.c
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1639,8 +1639,6 @@ gotten:
>  	/*
>  	 * Re-check the pte - we dropped the lock
>  	 */
> -	mmu_notifier(invalidate_range, mm, address,
> -				address + PAGE_SIZE - 1, 0);
>  	page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
>  	if (likely(pte_same(*page_table, orig_pte))) {
>  		if (old_page) {

What we did is to invalidate the page (?!) before taking the pte lock. In 
the lock we replace the pte to point to another page. This means that we 
need to clear stale information. So we zap it before. If another reference 
is established after taking the spinlock then the pte contents have 
changed at the cirtical section fails.

Before the critical section starts we have gotten an extra refcount on the 
original page so the page cannot vanish from under us.

> @@ -1676,6 +1674,8 @@ gotten:
>  		page_cache_release(old_page);
>  unlock:
>  	pte_unmap_unlock(page_table, ptl);
> +	mmu_notifier(invalidate_range, mm, address,
> +				address + PAGE_SIZE - 1, 0);
>  	if (dirty_page) {
>  		if (vma->vm_file)
>  			file_update_time(vma->vm_file);

Now we invalidate the page after the transaction is complete. This means 
external pte can persist while we change the pte? Possibly even dirty the 
page?




^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 16:20   ` Andrea Arcangeli
  2008-01-29 18:28     ` Andrea Arcangeli
@ 2008-01-29 19:55     ` Christoph Lameter
  2008-01-29 21:17       ` Andrea Arcangeli
  1 sibling, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-29 19:55 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Tue, 29 Jan 2008, Andrea Arcangeli wrote:

> > +	mmu_notifier(invalidate_range, mm, address,
> > +				address + PAGE_SIZE - 1, 0);
> >  	page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
> >  	if (likely(pte_same(*page_table, orig_pte))) {
> >  		if (old_page) {
> 
> What's the point of invalidate_range when the size is PAGE_SIZE? And
> how can it be right to invalidate_range _before_ ptep_clear_flush?

I am not sure. AFAICT you wrote that code.

It seems to be okay to invalidate range if you hold mmap_sem writably. In 
that case no additional faults can happen that would create new ptes.




^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-29 16:20   ` Andrea Arcangeli
@ 2008-01-29 18:28     ` Andrea Arcangeli
  2008-01-29 20:30       ` Christoph Lameter
  2008-01-29 19:55     ` Christoph Lameter
  1 sibling, 1 reply; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-29 18:28 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

Christoph, the below patch should fix the current leak of the pinned
pages. I hope the page-pin that should be dropped by the
invalidate_range op, is enough to prevent the "physical page" mapped
on that "mm+address" to change before invalidate_range returns. If
that would ever happen, there would be a coherency loss between the
guest VM writes and the writes coming from userland on the same
mm+address from a different thread (qemu, whatever). invalidate_page
before PT lock was obviously safe. Now we entirely relay on the pin to
prevent the page to change before invalidate_range returns. If the pte
is unmapped and the page is mapped back in with a minor fault that's
ok, as long as the physical page remains the same for that mm+address,
until all sptes are gone.

Signed-off-by: Andrea Arcangeli <andrea@qumranet.com>

diff --git a/mm/fremap.c b/mm/fremap.c
--- a/mm/fremap.c
+++ b/mm/fremap.c
@@ -212,8 +212,8 @@ asmlinkage long sys_remap_file_pages(uns
 		spin_unlock(&mapping->i_mmap_lock);
 	}
 
+	err = populate_range(mm, vma, start, size, pgoff);
 	mmu_notifier(invalidate_range, mm, start, start + size, 0);
-	err = populate_range(mm, vma, start, size, pgoff);
 	if (!err && !(flags & MAP_NONBLOCK)) {
 		if (unlikely(has_write_lock)) {
 			downgrade_write(&mm->mmap_sem);
diff --git a/mm/memory.c b/mm/memory.c
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1639,8 +1639,6 @@ gotten:
 	/*
 	 * Re-check the pte - we dropped the lock
 	 */
-	mmu_notifier(invalidate_range, mm, address,
-				address + PAGE_SIZE - 1, 0);
 	page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
 	if (likely(pte_same(*page_table, orig_pte))) {
 		if (old_page) {
@@ -1676,6 +1674,8 @@ gotten:
 		page_cache_release(old_page);
 unlock:
 	pte_unmap_unlock(page_table, ptl);
+	mmu_notifier(invalidate_range, mm, address,
+				address + PAGE_SIZE - 1, 0);
 	if (dirty_page) {
 		if (vma->vm_file)
 			file_update_time(vma->vm_file);

^ permalink raw reply	[flat|nested] 101+ messages in thread

* Re: [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-28 20:28 ` [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges Christoph Lameter
@ 2008-01-29 16:20   ` Andrea Arcangeli
  2008-01-29 18:28     ` Andrea Arcangeli
  2008-01-29 19:55     ` Christoph Lameter
  0 siblings, 2 replies; 101+ messages in thread
From: Andrea Arcangeli @ 2008-01-29 16:20 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

On Mon, Jan 28, 2008 at 12:28:42PM -0800, Christoph Lameter wrote:
> Index: linux-2.6/mm/fremap.c
> ===================================================================
> --- linux-2.6.orig/mm/fremap.c	2008-01-25 19:31:05.000000000 -0800
> +++ linux-2.6/mm/fremap.c	2008-01-25 19:32:49.000000000 -0800
> @@ -15,6 +15,7 @@
>  #include <linux/rmap.h>
>  #include <linux/module.h>
>  #include <linux/syscalls.h>
> +#include <linux/mmu_notifier.h>
>  
>  #include <asm/mmu_context.h>
>  #include <asm/cacheflush.h>
> @@ -211,6 +212,7 @@ asmlinkage long sys_remap_file_pages(uns
>  		spin_unlock(&mapping->i_mmap_lock);
>  	}
>  
> +	mmu_notifier(invalidate_range, mm, start, start + size, 0);
>  	err = populate_range(mm, vma, start, size, pgoff);

How can it be right to invalidate_range _before_ ptep_clear_flush?

> @@ -1634,6 +1639,8 @@ gotten:
>  	/*
>  	 * Re-check the pte - we dropped the lock
>  	 */
> +	mmu_notifier(invalidate_range, mm, address,
> +				address + PAGE_SIZE - 1, 0);
>  	page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
>  	if (likely(pte_same(*page_table, orig_pte))) {
>  		if (old_page) {

What's the point of invalidate_range when the size is PAGE_SIZE? And
how can it be right to invalidate_range _before_ ptep_clear_flush?

^ permalink raw reply	[flat|nested] 101+ messages in thread

* [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges
  2008-01-28 20:28 [patch 0/6] [RFC] MMU Notifiers V2 Christoph Lameter
@ 2008-01-28 20:28 ` Christoph Lameter
  2008-01-29 16:20   ` Andrea Arcangeli
  0 siblings, 1 reply; 101+ messages in thread
From: Christoph Lameter @ 2008-01-28 20:28 UTC (permalink / raw)
  To: Andrea Arcangeli
  Cc: Robin Holt, Avi Kivity, Izik Eidus, Nick Piggin, kvm-devel,
	Benjamin Herrenschmidt, Peter Zijlstra, steiner, linux-kernel,
	linux-mm, daniel.blueman, Hugh Dickins

[-- Attachment #1: mmu_invalidate_range_callbacks --]
[-- Type: text/plain, Size: 4671 bytes --]

The invalidation of address ranges in a mm_struct needs to be
performed when pages are removed or permissions etc change.
Most of the VM address space changes can use the range invalidate
callback.

invalidate_range() is generally called with mmap_sem held but
no spinlocks are active. If invalidate_range() is called with
locks held then we pass a flag into invalidate_range()

Comments state that mmap_sem must be held for
remap_pfn_range() but various drivers do not seem to do this.

Signed-off-by: Andrea Arcangeli <andrea@qumranet.com>
Signed-off-by: Robin Holt <holt@sgi.com>
Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 mm/fremap.c  |    2 ++
 mm/hugetlb.c |    2 ++
 mm/memory.c  |   11 +++++++++--
 mm/mmap.c    |    1 +
 4 files changed, 14 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/fremap.c
===================================================================
--- linux-2.6.orig/mm/fremap.c	2008-01-25 19:31:05.000000000 -0800
+++ linux-2.6/mm/fremap.c	2008-01-25 19:32:49.000000000 -0800
@@ -15,6 +15,7 @@
 #include <linux/rmap.h>
 #include <linux/module.h>
 #include <linux/syscalls.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/mmu_context.h>
 #include <asm/cacheflush.h>
@@ -211,6 +212,7 @@ asmlinkage long sys_remap_file_pages(uns
 		spin_unlock(&mapping->i_mmap_lock);
 	}
 
+	mmu_notifier(invalidate_range, mm, start, start + size, 0);
 	err = populate_range(mm, vma, start, size, pgoff);
 	if (!err && !(flags & MAP_NONBLOCK)) {
 		if (unlikely(has_write_lock)) {
Index: linux-2.6/mm/memory.c
===================================================================
--- linux-2.6.orig/mm/memory.c	2008-01-25 19:31:05.000000000 -0800
+++ linux-2.6/mm/memory.c	2008-01-25 19:32:49.000000000 -0800
@@ -50,6 +50,7 @@
 #include <linux/delayacct.h>
 #include <linux/init.h>
 #include <linux/writeback.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/pgalloc.h>
 #include <asm/uaccess.h>
@@ -891,6 +892,8 @@ unsigned long zap_page_range(struct vm_a
 	end = unmap_vmas(&tlb, vma, address, end, &nr_accounted, details);
 	if (tlb)
 		tlb_finish_mmu(tlb, address, end);
+	mmu_notifier(invalidate_range, mm, address, end,
+		(details ? (details->i_mmap_lock != NULL)  : 0));
 	return end;
 }
 
@@ -1319,7 +1322,7 @@ int remap_pfn_range(struct vm_area_struc
 {
 	pgd_t *pgd;
 	unsigned long next;
-	unsigned long end = addr + PAGE_ALIGN(size);
+	unsigned long start = addr, end = addr + PAGE_ALIGN(size);
 	struct mm_struct *mm = vma->vm_mm;
 	int err;
 
@@ -1360,6 +1363,7 @@ int remap_pfn_range(struct vm_area_struc
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
+	mmu_notifier(invalidate_range, mm, start, end, 0);
 	return err;
 }
 EXPORT_SYMBOL(remap_pfn_range);
@@ -1443,7 +1447,7 @@ int apply_to_page_range(struct mm_struct
 {
 	pgd_t *pgd;
 	unsigned long next;
-	unsigned long end = addr + size;
+	unsigned long start = addr, end = addr + size;
 	int err;
 
 	BUG_ON(addr >= end);
@@ -1454,6 +1458,7 @@ int apply_to_page_range(struct mm_struct
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
+	mmu_notifier(invalidate_range, mm, start, end, 0);
 	return err;
 }
 EXPORT_SYMBOL_GPL(apply_to_page_range);
@@ -1634,6 +1639,8 @@ gotten:
 	/*
 	 * Re-check the pte - we dropped the lock
 	 */
+	mmu_notifier(invalidate_range, mm, address,
+				address + PAGE_SIZE - 1, 0);
 	page_table = pte_offset_map_lock(mm, pmd, address, &ptl);
 	if (likely(pte_same(*page_table, orig_pte))) {
 		if (old_page) {
Index: linux-2.6/mm/mmap.c
===================================================================
--- linux-2.6.orig/mm/mmap.c	2008-01-25 19:31:05.000000000 -0800
+++ linux-2.6/mm/mmap.c	2008-01-25 19:32:49.000000000 -0800
@@ -1748,6 +1748,7 @@ static void unmap_region(struct mm_struc
 	free_pgtables(&tlb, vma, prev? prev->vm_end: FIRST_USER_ADDRESS,
 				 next? next->vm_start: 0);
 	tlb_finish_mmu(tlb, start, end);
+	mmu_notifier(invalidate_range, mm, start, end, 0);
 }
 
 /*
Index: linux-2.6/mm/hugetlb.c
===================================================================
--- linux-2.6.orig/mm/hugetlb.c	2008-01-25 19:33:58.000000000 -0800
+++ linux-2.6/mm/hugetlb.c	2008-01-25 19:34:13.000000000 -0800
@@ -14,6 +14,7 @@
 #include <linux/mempolicy.h>
 #include <linux/cpuset.h>
 #include <linux/mutex.h>
+#include <linux/mmu_notifier.h>
 
 #include <asm/page.h>
 #include <asm/pgtable.h>
@@ -763,6 +764,7 @@ void __unmap_hugepage_range(struct vm_ar
 	}
 	spin_unlock(&mm->page_table_lock);
 	flush_tlb_range(vma, start, end);
+	mmu_notifier(invalidate_range, mm, start, end, 1);
 	list_for_each_entry_safe(page, tmp, &page_list, lru) {
 		list_del(&page->lru);
 		put_page(page);

-- 

^ permalink raw reply	[flat|nested] 101+ messages in thread

end of thread, other threads:[~2008-03-05  0:53 UTC | newest]

Thread overview: 101+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-01-30  2:29 [patch 0/6] [RFC] MMU Notifiers V3 Christoph Lameter
2008-01-30  2:29 ` [patch 1/6] mmu_notifier: Core code Christoph Lameter
2008-01-30 15:37   ` Andrea Arcangeli
2008-01-30 15:53     ` Jack Steiner
2008-01-30 16:38       ` Andrea Arcangeli
2008-01-30 19:19       ` Christoph Lameter
2008-01-30 22:20         ` Robin Holt
2008-01-30 23:38           ` Andrea Arcangeli
2008-01-30 23:55             ` Christoph Lameter
2008-01-31  0:12               ` [kvm-devel] " Andrea Arcangeli
2008-01-31  1:27                 ` Christoph Lameter
2008-01-30 17:10     ` Peter Zijlstra
2008-01-30 19:28       ` Christoph Lameter
2008-01-30 18:02   ` Robin Holt
2008-01-30 19:08     ` Christoph Lameter
2008-01-30 19:14     ` Christoph Lameter
2008-01-30  2:29 ` [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges Christoph Lameter
2008-01-30  2:29 ` [patch 3/6] mmu_notifier: invalidate_page callbacks for subsystems with rmap Christoph Lameter
2008-01-30 18:03   ` Robin Holt
2008-01-30  2:29 ` [patch 4/6] MMU notifier: invalidate_page callbacks using Linux rmaps Christoph Lameter
2008-01-30  2:29 ` [patch 5/6] mmu_notifier: Callbacks for xip_filemap.c Christoph Lameter
2008-01-30  2:29 ` [patch 6/6] mmu_notifier: Add invalidate_all() Christoph Lameter
  -- strict thread matches above, loose matches on Subject: below --
2008-02-15  6:48 [patch 0/6] MMU Notifiers V7 Christoph Lameter
2008-02-15  6:49 ` [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges Christoph Lameter
2008-02-16  3:37   ` Andrew Morton
2008-02-16 19:26     ` Christoph Lameter
2008-02-19  8:54   ` Nick Piggin
2008-02-19 13:34     ` Andrea Arcangeli
2008-02-27 22:23       ` Christoph Lameter
2008-02-27 23:57         ` Andrea Arcangeli
2008-02-19 23:08   ` Nick Piggin
2008-02-20  1:00     ` Andrea Arcangeli
2008-02-20  3:00       ` Robin Holt
2008-02-20  3:11         ` Nick Piggin
2008-02-20  3:19           ` Robin Holt
2008-02-27 22:39       ` Christoph Lameter
2008-02-28  0:38         ` Andrea Arcangeli
2008-02-27 22:35     ` Christoph Lameter
2008-02-27 22:42       ` Jack Steiner
2008-02-28  0:10       ` Christoph Lameter
2008-02-28  0:11       ` Andrea Arcangeli
2008-02-28  0:14         ` Christoph Lameter
2008-02-28  0:52           ` Andrea Arcangeli
2008-02-28  1:03             ` Christoph Lameter
2008-02-28  1:10               ` Andrea Arcangeli
2008-02-28 18:43                 ` Christoph Lameter
2008-02-29  0:55                   ` Andrea Arcangeli
2008-02-29  0:59                     ` Christoph Lameter
2008-02-29 13:13                       ` Andrea Arcangeli
2008-02-29 19:55                         ` Christoph Lameter
2008-02-29 20:17                           ` Andrea Arcangeli
2008-02-29 21:03                             ` Christoph Lameter
2008-02-29 21:23                               ` Andrea Arcangeli
2008-02-29 21:29                                 ` Christoph Lameter
2008-02-29 21:34                                 ` Christoph Lameter
2008-02-29 21:48                                   ` Andrea Arcangeli
2008-02-29 22:12                                     ` Christoph Lameter
2008-02-29 22:41                                       ` Andrea Arcangeli
2008-02-28 10:53             ` Robin Holt
2008-03-03  5:11       ` Nick Piggin
2008-03-03 19:28         ` Christoph Lameter
2008-03-03 19:50           ` Nick Piggin
2008-03-04 18:58             ` Christoph Lameter
2008-03-05  0:52               ` Nick Piggin
2008-02-08 22:06 [patch 0/6] MMU Notifiers V6 Christoph Lameter
2008-02-08 22:06 ` [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges Christoph Lameter
2008-01-28 20:28 [patch 0/6] [RFC] MMU Notifiers V2 Christoph Lameter
2008-01-28 20:28 ` [patch 2/6] mmu_notifier: Callbacks to invalidate address ranges Christoph Lameter
2008-01-29 16:20   ` Andrea Arcangeli
2008-01-29 18:28     ` Andrea Arcangeli
2008-01-29 20:30       ` Christoph Lameter
2008-01-29 21:36         ` Andrea Arcangeli
2008-01-29 21:53           ` Christoph Lameter
2008-01-29 22:35             ` Andrea Arcangeli
2008-01-29 22:55               ` Christoph Lameter
2008-01-29 23:43                 ` Andrea Arcangeli
2008-01-30  0:34                   ` Christoph Lameter
2008-01-29 19:55     ` Christoph Lameter
2008-01-29 21:17       ` Andrea Arcangeli
2008-01-29 21:35         ` Christoph Lameter
2008-01-29 22:02           ` Andrea Arcangeli
2008-01-29 22:39             ` Christoph Lameter
2008-01-30  0:00               ` Andrea Arcangeli
2008-01-30  0:05                 ` Andrea Arcangeli
2008-01-30  0:22                   ` Christoph Lameter
2008-01-30  0:59                     ` Andrea Arcangeli
2008-01-30  8:26                       ` Peter Zijlstra
2008-01-30  0:20                 ` Christoph Lameter
2008-01-30  0:28                   ` Jack Steiner
2008-01-30  0:35                     ` Christoph Lameter
2008-01-30 13:37                     ` Andrea Arcangeli
2008-01-30 14:43                       ` Jack Steiner
2008-01-30 19:41                         ` Christoph Lameter
2008-01-30 20:29                           ` Jack Steiner
2008-01-30 20:55                             ` Christoph Lameter
2008-01-30 16:11                 ` Robin Holt
2008-01-30 17:04                   ` Andrea Arcangeli
2008-01-30 17:30                     ` Robin Holt
2008-01-30 18:25                       ` Andrea Arcangeli
2008-01-30 19:50                         ` Christoph Lameter
2008-01-30 22:18                           ` Robin Holt
2008-01-30 23:52                           ` Andrea Arcangeli
2008-01-31  0:01                             ` Christoph Lameter
2008-01-30 19:35                   ` Christoph Lameter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).