Re: [patch 6/12] hold atomic kmaps across generic_file_read

[Andrew Morton <akpm@zip.com.au>]

Linus Torvalds wrote:
> 
> ...
>  - do_page_fault() already does an
> 
>         if (in_interrupt() || !mm)
>                 goto no_context;
> 
>    and the fact is, the "in_interrupt()" should really be an
>    "preempt_count()", since it's illegal to take a page fault not just in
>    interrupts, but while non-preemptible in general.
> 

gargh.  preempt_disable (and, hence, kmap_atomic) do not bump
the preempt counter with CONFIG_PREEMPT=n.

Is there a plan to change this?

If not, I don't think it's worth making this change just for
the highmem read/write thing (calculating `current' at each
spin_lock site...)   I just open coded it.

This works.  I still need to do the other architectures' fault
handlers, do writes and test it for more than seven seconds.

 arch/i386/mm/fault.c    |    6 +++---
 include/linux/preempt.h |   14 ++++++++++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

--- 2.5.30/arch/i386/mm/fault.c~atomic-copy_user	Sat Aug 10 14:44:03 2002
+++ 2.5.30-akpm/arch/i386/mm/fault.c	Sat Aug 10 14:44:52 2002
@@ -189,10 +189,10 @@ asmlinkage void do_page_fault(struct pt_
 	info.si_code = SEGV_MAPERR;
 
 	/*
-	 * If we're in an interrupt or have no user
-	 * context, we must not take the fault..
+	 * If we're in an interrupt, have no user context or are running in an
+	 * atomic region then we must not take the fault..
 	 */
-	if (in_interrupt() || !mm)
+	if (preempt_count() || !mm)
 		goto no_context;
 
 #ifdef CONFIG_X86_REMOTE_DEBUG
--- 2.5.30/include/linux/preempt.h~atomic-copy_user	Sat Aug 10 16:18:50 2002
+++ 2.5.30-akpm/include/linux/preempt.h	Sat Aug 10 16:20:16 2002
@@ -5,19 +5,29 @@
 
 #define preempt_count() (current_thread_info()->preempt_count)
 
+#define inc_preempt_count() \
+do { \
+	preempt_count()++; \
+} while (0)
+
+#define dec_preempt_count() \
+do { \
+	preempt_count()--; \
+} while (0)
+
 #ifdef CONFIG_PREEMPT
 
 extern void preempt_schedule(void);
 
 #define preempt_disable() \
 do { \
-	preempt_count()++; \
+	inc_preempt_count(); \
 	barrier(); \
 } while (0)
 
 #define preempt_enable_no_resched() \
 do { \
-	preempt_count()--; \
+	dec_preempt_count(); \
 	barrier(); \
 } while (0)
 






 filemap.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 49 insertions(+), 2 deletions(-)

--- 2.5.30/mm/filemap.c~kmap_atomic_reads	Sat Aug 10 17:09:47 2002
+++ 2.5.30-akpm/mm/filemap.c	Sat Aug 10 17:27:35 2002
@@ -1020,7 +1020,37 @@ no_cached_page:
 	UPDATE_ATIME(inode);
 }
 
-int file_read_actor(read_descriptor_t * desc, struct page *page, unsigned long offset, unsigned long size)
+/*
+ * Fault a userspace page into pagetables.  Return non-zero on EFAULT.
+ * FIXME: this assumes that two userspace pages are always sufficient.  That's
+ * not true if PAGE_CACHE_SIZE > PAGE_SIZE.
+ */
+static inline int fault_in_page_writeable(char *uaddr, int size)
+{
+	int ret;
+
+	/*
+	 * Writing zeroes into userspace here is OK, because we know that if
+	 * the zero gets there, we'll be overwriting it.
+	 */
+	ret = __put_user(0, uaddr);
+	if (ret == 0) {
+		char *end = uaddr + size - 1;
+
+		/*
+		 * If the page was already mapped, this will get a cache miss
+		 * for sure, so try to avoid doing it.  This is only useful if
+		 * userspace is doing page-aligned IO, which is rare.  Lose it?
+		 */
+		if (((unsigned long)uaddr & PAGE_MASK) !=
+				((unsigned long)end & PAGE_MASK))
+		 	ret = __put_user(0, end);
+	}
+	return ret;
+}
+
+int file_read_actor(read_descriptor_t *desc, struct page *page,
+			unsigned long offset, unsigned long size)
 {
 	char *kaddr;
 	unsigned long left, count = desc->count;
@@ -1028,14 +1058,31 @@ int file_read_actor(read_descriptor_t * 
 	if (size > count)
 		size = count;
 
+	/*
+	 * Faults on the destination of a read are common, so do it before
+	 * taking the kmap.
+	 */
+	if (!fault_in_page_writeable(desc->buf, size)) {
+		kaddr = kmap_atomic(page, KM_USER0);
+		inc_preempt_count();	/* An atomic copy_to_user */
+		left = __copy_to_user(desc->buf, kaddr + offset, size);
+		dec_preempt_count();
+		kunmap_atomic(kaddr, KM_USER0);
+		if (left == 0)
+			goto success;
+		printk("%s: Unexpected page fault\n", __FUNCTION__);
+	}
+
+	/* Do it the slow way */
 	kaddr = kmap(page);
 	left = __copy_to_user(desc->buf, kaddr + offset, size);
 	kunmap(page);
-	
+
 	if (left) {
 		size -= left;
 		desc->error = -EFAULT;
 	}
+success:
 	desc->count = count - size;
 	desc->written += size;
 	desc->buf += size;

.