[rfc] x86: optimise page fault path a little

[rfc] x86: optimise page fault path a little

[Nick Piggin]

Hi,

I was just looking around the page fault code for any obvious performance
improvements. I noticed do_page_fault is rather big, uses a lot of stack,
and generates some branch mispredicts.

It's only about 1.1% on the profile of the workload I'm looking at, so my
improvement is pretty close to in the noise, but I wonder if micro
optimisations like the following would be welcome?

This patch adds branch hints and moves error condition code out of line.
It shrinks do_page_fault from 2410 bytes to 603 bytes, and from 352 to 64
bytes of stack. Total text size does grow by about 500 bytes due to the
additional functions added.

---

Index: linux-2.6/arch/x86/mm/fault.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/fault.c	2008-11-13 18:20:33.000000000 +1100
+++ linux-2.6/arch/x86/mm/fault.c	2008-11-13 18:23:26.000000000 +1100
@@ -91,8 +91,8 @@ static inline int notify_page_fault(stru
  *
  * Opcode checker based on code by Richard Brunner
  */
-static int is_prefetch(struct pt_regs *regs, unsigned long addr,
-		       unsigned long error_code)
+static int is_prefetch(struct pt_regs *regs, unsigned long error_code,
+			unsigned long addr)
 {
 	unsigned char *instr;
 	int scan_more = 1;
@@ -409,16 +409,16 @@ static void show_fault_oops(struct pt_re
 }
 
 #ifdef CONFIG_X86_64
-static noinline void pgtable_bad(unsigned long address, struct pt_regs *regs,
-				 unsigned long error_code)
+/* TODO: match order of arguments */
+static noinline void pgtable_bad(struct pt_regs *regs,
+			 unsigned long error_code, unsigned long address)
 {
 	unsigned long flags = oops_begin();
-	struct task_struct *tsk;
+	struct task_struct *tsk = current;
 
 	printk(KERN_ALERT "%s: Corrupted page table at address %lx\n",
-	       current->comm, address);
+	       tsk->comm, address);
 	dump_pagetable(address);
-	tsk = current;
 	tsk->thread.cr2 = address;
 	tsk->thread.trap_no = 14;
 	tsk->thread.error_code = error_code;
@@ -428,6 +428,198 @@ static noinline void pgtable_bad(unsigne
 }
 #endif
 
+static noinline void no_context(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address)
+{
+	struct task_struct *tsk = current;
+#ifdef CONFIG_X86_64
+	unsigned long flags;
+#endif
+
+	/* Are we prepared to handle this kernel fault?  */
+	if (fixup_exception(regs))
+		return;
+
+	/*
+	 * X86_32
+	 * Valid to do another page fault here, because if this fault
+	 * had been triggered by is_prefetch fixup_exception would have
+	 * handled it.
+	 *
+	 * X86_64
+	 * Hall of shame of CPU/BIOS bugs.
+	 */
+	if (is_prefetch(regs, error_code, address))
+		return;
+
+	if (is_errata93(regs, address))
+		return;
+
+	/*
+	 * Oops. The kernel tried to access some bad page. We'll have to
+	 * terminate things with extreme prejudice.
+	 */
+#ifdef CONFIG_X86_32
+	bust_spinlocks(1);
+#else
+	flags = oops_begin();
+#endif
+
+	show_fault_oops(regs, error_code, address);
+
+	tsk->thread.cr2 = address;
+	tsk->thread.trap_no = 14;
+	tsk->thread.error_code = error_code;
+
+#ifdef CONFIG_X86_32
+	die("Oops", regs, error_code);
+	bust_spinlocks(0);
+	do_exit(SIGKILL);
+#else
+	if (__die("Oops", regs, error_code))
+		regs = NULL;
+	/* Executive summary in case the body of the oops scrolled away */
+	printk(KERN_EMERG "CR2: %016lx\n", address);
+	oops_end(flags, regs, SIGKILL);
+#endif
+}
+
+static void __bad_area_nosemaphore(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address,
+			int si_code)
+{
+	struct task_struct *tsk = current;
+
+	/* User mode accesses just cause a SIGSEGV */
+	if (error_code & PF_USER) {
+		/*
+		 * It's possible to have interrupts off here.
+		 */
+		local_irq_enable();
+
+		/*
+		 * Valid to do another page fault here because this one came
+		 * from user space.
+		 */
+		if (is_prefetch(regs, error_code, address))
+			return;
+
+		if (is_errata100(regs, address))
+			return;
+
+		if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
+		    printk_ratelimit()) {
+			printk(
+			"%s%s[%d]: segfault at %lx ip %p sp %p error %lx",
+			task_pid_nr(tsk) > 1 ? KERN_INFO : KERN_EMERG,
+			tsk->comm, task_pid_nr(tsk), address,
+			(void *) regs->ip, (void *) regs->sp, error_code);
+			print_vma_addr(" in ", regs->ip);
+			printk("\n");
+		}
+
+		tsk->thread.cr2 = address;
+		/* Kernel addresses are always protection faults */
+		tsk->thread.error_code = error_code | (address >= TASK_SIZE);
+		tsk->thread.trap_no = 14;
+		force_sig_info_fault(SIGSEGV, si_code, address, tsk);
+		return;
+	}
+
+	if (is_f00f_bug(regs, address))
+		return;
+
+	no_context(regs, error_code, address);
+}
+
+static noinline void bad_area_nosemaphore(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address)
+{
+	__bad_area_nosemaphore(regs, error_code, address, SEGV_MAPERR);
+}
+
+static void __bad_area(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address,
+			int si_code)
+{
+	struct mm_struct *mm = current->mm;
+
+	/*
+	 * Something tried to access memory that isn't in our memory map..
+	 * Fix it, but check if it's kernel or user first..
+	 */
+	up_read(&mm->mmap_sem);
+
+	__bad_area_nosemaphore(regs, error_code, address, si_code);
+}
+
+static noinline void bad_area(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address)
+{
+	__bad_area(regs, error_code, address, SEGV_MAPERR);
+}
+
+static noinline void bad_area_accerr(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address)
+{
+	__bad_area(regs, error_code, address, SEGV_ACCERR);
+}
+
+/* TODO: fixup for oom handling */
+static void out_of_memory(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address)
+{
+	struct task_struct *tsk = current;
+	struct mm_struct *mm = tsk->mm;
+	/*
+	 * We ran out of memory, or some other thing happened to us that made
+	 * us unable to handle the page fault gracefully.
+	 */
+	up_read(&mm->mmap_sem);
+	if (is_global_init(tsk)) {
+		yield();
+		return;
+	}
+
+	printk("VM: killing process %s\n", tsk->comm);
+	if (error_code & PF_USER)
+		do_group_exit(SIGKILL);
+	no_context(regs, error_code, address);
+}
+
+static void do_sigbus(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address)
+{
+	struct task_struct *tsk = current;
+	struct mm_struct *mm = tsk->mm;
+
+	up_read(&mm->mmap_sem);
+
+	/* Kernel mode? Handle exceptions or die */
+	if (!(error_code & PF_USER))
+		no_context(regs, error_code, address);
+#ifdef CONFIG_X86_32
+	/* User space => ok to do another page fault */
+	if (is_prefetch(regs, error_code, address))
+		return;
+#endif
+	tsk->thread.cr2 = address;
+	tsk->thread.error_code = error_code;
+	tsk->thread.trap_no = 14;
+	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk);
+}
+
+static noinline void mm_fault_error(struct pt_regs *regs,
+		unsigned long error_code, unsigned long address, unsigned int fault)
+{
+	if (fault & VM_FAULT_OOM)
+		out_of_memory(regs, error_code, address);
+	else if (fault & VM_FAULT_SIGBUS)
+		do_sigbus(regs, error_code, address);
+	else
+		BUG();
+}
+
 static int spurious_fault_check(unsigned long error_code, pte_t *pte)
 {
 	if ((error_code & PF_WRITE) && !pte_write(*pte))
@@ -447,8 +639,8 @@ static int spurious_fault_check(unsigned
  * There are no security implications to leaving a stale TLB when
  * increasing the permissions on a page.
  */
-static int spurious_fault(unsigned long address,
-			  unsigned long error_code)
+static noinline int spurious_fault(unsigned long error_code,
+				unsigned long address)
 {
 	pgd_t *pgd;
 	pud_t *pud;
@@ -493,7 +685,7 @@ static int spurious_fault(unsigned long 
  *
  * This assumes no large pages in there.
  */
-static int vmalloc_fault(unsigned long address)
+static noinline int vmalloc_fault(unsigned long address)
 {
 #ifdef CONFIG_X86_32
 	unsigned long pgd_paddr;
@@ -582,15 +774,12 @@ asmlinkage
 #endif
 void __kprobes do_page_fault(struct pt_regs *regs, unsigned long error_code)
 {
+	unsigned long address;
 	struct task_struct *tsk;
 	struct mm_struct *mm;
 	struct vm_area_struct *vma;
-	unsigned long address;
 	int write, si_code;
 	int fault;
-#ifdef CONFIG_X86_64
-	unsigned long flags;
-#endif
 
 	tsk = current;
 	mm = tsk->mm;
@@ -601,7 +790,7 @@ void __kprobes do_page_fault(struct pt_r
 
 	si_code = SEGV_MAPERR;
 
-	if (notify_page_fault(regs))
+	if (unlikely(notify_page_fault(regs)))
 		return;
 	if (unlikely(kmmio_fault(regs, address)))
 		return;
@@ -636,7 +825,8 @@ void __kprobes do_page_fault(struct pt_r
 		 * Don't take the mm semaphore here. If we fixup a prefetch
 		 * fault we could otherwise deadlock.
 		 */
-		goto bad_area_nosemaphore;
+		bad_area_nosemaphore(regs, error_code, address);
+		return;
 	}
 
 
@@ -655,17 +845,18 @@ void __kprobes do_page_fault(struct pt_r
 
 #ifdef CONFIG_X86_64
 	if (unlikely(error_code & PF_RSVD))
-		pgtable_bad(address, regs, error_code);
+		pgtable_bad(regs, error_code, address);
 #endif
 
 	/*
 	 * 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 (unlikely(in_atomic() || !mm))
-		goto bad_area_nosemaphore;
+	if (unlikely(in_atomic() || !mm)) {
+		bad_area_nosemaphore(regs, error_code, address);
+		return;
+	}
 
-again:
 	/*
 	 * When running in the kernel we expect faults to occur only to
 	 * addresses in user space.  All other faults represent errors in the
@@ -682,20 +873,26 @@ again:
 	 * source.  If this is invalid we can skip the address space check,
 	 * thus avoiding the deadlock.
 	 */
-	if (!down_read_trylock(&mm->mmap_sem)) {
+	if (unlikely(!down_read_trylock(&mm->mmap_sem))) {
 		if ((error_code & PF_USER) == 0 &&
-		    !search_exception_tables(regs->ip))
-			goto bad_area_nosemaphore;
+		    !search_exception_tables(regs->ip)) {
+			bad_area_nosemaphore(regs, error_code, address);
+			return;
+		}
 		down_read(&mm->mmap_sem);
 	}
 
 	vma = find_vma(mm, address);
-	if (!vma)
-		goto bad_area;
-	if (vma->vm_start <= address)
+	if (unlikely(!vma)) {
+		bad_area(regs, error_code, address);
+		return;
+	}
+	if (likely(vma->vm_start <= address))
 		goto good_area;
-	if (!(vma->vm_flags & VM_GROWSDOWN))
-		goto bad_area;
+	if (unlikely(!(vma->vm_flags & VM_GROWSDOWN))) {
+		bad_area(regs, error_code, address);
+		return;
+	}
 	if (error_code & PF_USER) {
 		/*
 		 * Accessing the stack below %sp is always a bug.
@@ -703,31 +900,34 @@ again:
 		 * and pusha to work.  ("enter $65535,$31" pushes
 		 * 32 pointers and then decrements %sp by 65535.)
 		 */
-		if (address + 65536 + 32 * sizeof(unsigned long) < regs->sp)
-			goto bad_area;
+		if (unlikely(address + 65536 + 32 * sizeof(unsigned long) < regs->sp)) {
+			bad_area(regs, error_code, address);
+			return;
+		}
 	}
-	if (expand_stack(vma, address))
-		goto bad_area;
-/*
- * Ok, we have a good vm_area for this memory access, so
- * we can handle it..
- */
+	if (unlikely(expand_stack(vma, address))) {
+		bad_area(regs, error_code, address);
+		return;
+	}
+
+	/*
+	 * Ok, we have a good vm_area for this memory access, so
+	 * we can handle it..
+	 */
 good_area:
 	si_code = SEGV_ACCERR;
-	write = 0;
-	switch (error_code & (PF_PROT|PF_WRITE)) {
-	default:	/* 3: write, present */
-		/* fall through */
-	case PF_WRITE:		/* write, not present */
-		if (!(vma->vm_flags & VM_WRITE))
-			goto bad_area;
-		write++;
-		break;
-	case PF_PROT:		/* read, present */
-		goto bad_area;
-	case 0:			/* read, not present */
-		if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))
-			goto bad_area;
+	write = error_code & PF_WRITE;
+	if (write) {
+		if (unlikely(!(vma->vm_flags & VM_WRITE))) {
+			bad_area_accerr(regs, error_code, address);
+			return;
+		}
+	} else if (unlikely(error_code & PF_PROT)) {
+		bad_area_accerr(regs, error_code, address);
+		return;
+	} else if (unlikely(!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))) {
+		bad_area_accerr(regs, error_code, address);
+		return;
 	}
 
 	/*
@@ -737,11 +937,8 @@ good_area:
 	 */
 	fault = handle_mm_fault(mm, vma, address, write);
 	if (unlikely(fault & VM_FAULT_ERROR)) {
-		if (fault & VM_FAULT_OOM)
-			goto out_of_memory;
-		else if (fault & VM_FAULT_SIGBUS)
-			goto do_sigbus;
-		BUG();
+		mm_fault_error(regs, error_code, address, fault);
+		return;
 	}
 	if (fault & VM_FAULT_MAJOR)
 		tsk->maj_flt++;
@@ -752,145 +949,13 @@ good_area:
 	/*
 	 * Did it hit the DOS screen memory VA from vm86 mode?
 	 */
-	if (v8086_mode(regs)) {
+	if (unlikely(v8086_mode(regs))) {
 		unsigned long bit = (address - 0xA0000) >> PAGE_SHIFT;
 		if (bit < 32)
 			tsk->thread.screen_bitmap |= 1 << bit;
 	}
 #endif
 	up_read(&mm->mmap_sem);
-	return;
-
-/*
- * Something tried to access memory that isn't in our memory map..
- * Fix it, but check if it's kernel or user first..
- */
-bad_area:
-	up_read(&mm->mmap_sem);
-
-bad_area_nosemaphore:
-	/* User mode accesses just cause a SIGSEGV */
-	if (error_code & PF_USER) {
-		/*
-		 * It's possible to have interrupts off here.
-		 */
-		local_irq_enable();
-
-		/*
-		 * Valid to do another page fault here because this one came
-		 * from user space.
-		 */
-		if (is_prefetch(regs, address, error_code))
-			return;
-
-		if (is_errata100(regs, address))
-			return;
-
-		if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
-		    printk_ratelimit()) {
-			printk(
-			"%s%s[%d]: segfault at %lx ip %p sp %p error %lx",
-			task_pid_nr(tsk) > 1 ? KERN_INFO : KERN_EMERG,
-			tsk->comm, task_pid_nr(tsk), address,
-			(void *) regs->ip, (void *) regs->sp, error_code);
-			print_vma_addr(" in ", regs->ip);
-			printk("\n");
-		}
-
-		tsk->thread.cr2 = address;
-		/* Kernel addresses are always protection faults */
-		tsk->thread.error_code = error_code | (address >= TASK_SIZE);
-		tsk->thread.trap_no = 14;
-		force_sig_info_fault(SIGSEGV, si_code, address, tsk);
-		return;
-	}
-
-	if (is_f00f_bug(regs, address))
-		return;
-
-no_context:
-	/* Are we prepared to handle this kernel fault?  */
-	if (fixup_exception(regs))
-		return;
-
-	/*
-	 * X86_32
-	 * Valid to do another page fault here, because if this fault
-	 * had been triggered by is_prefetch fixup_exception would have
-	 * handled it.
-	 *
-	 * X86_64
-	 * Hall of shame of CPU/BIOS bugs.
-	 */
-	if (is_prefetch(regs, address, error_code))
-		return;
-
-	if (is_errata93(regs, address))
-		return;
-
-/*
- * Oops. The kernel tried to access some bad page. We'll have to
- * terminate things with extreme prejudice.
- */
-#ifdef CONFIG_X86_32
-	bust_spinlocks(1);
-#else
-	flags = oops_begin();
-#endif
-
-	show_fault_oops(regs, error_code, address);
-
-	tsk->thread.cr2 = address;
-	tsk->thread.trap_no = 14;
-	tsk->thread.error_code = error_code;
-
-#ifdef CONFIG_X86_32
-	die("Oops", regs, error_code);
-	bust_spinlocks(0);
-	do_exit(SIGKILL);
-#else
-	if (__die("Oops", regs, error_code))
-		regs = NULL;
-	/* Executive summary in case the body of the oops scrolled away */
-	printk(KERN_EMERG "CR2: %016lx\n", address);
-	oops_end(flags, regs, SIGKILL);
-#endif
-
-/*
- * We ran out of memory, or some other thing happened to us that made
- * us unable to handle the page fault gracefully.
- */
-out_of_memory:
-	up_read(&mm->mmap_sem);
-	if (is_global_init(tsk)) {
-		yield();
-		/*
-		 * Re-lookup the vma - in theory the vma tree might
-		 * have changed:
-		 */
-		goto again;
-	}
-
-	printk("VM: killing process %s\n", tsk->comm);
-	if (error_code & PF_USER)
-		do_group_exit(SIGKILL);
-	goto no_context;
-
-do_sigbus:
-	up_read(&mm->mmap_sem);
-
-	/* Kernel mode? Handle exceptions or die */
-	if (!(error_code & PF_USER))
-		goto no_context;
-#ifdef CONFIG_X86_32
-	/* User space => ok to do another page fault */
-	if (is_prefetch(regs, address, error_code))
-		return;
-#endif
-	tsk->thread.cr2 = address;
-	tsk->thread.error_code = error_code;
-	tsk->thread.trap_no = 14;
-	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk);
 }
 
 DEFINE_SPINLOCK(pgd_lock);

Re: [rfc] x86: optimise page fault path a little

[Ingo Molnar]

* Nick Piggin <npiggin@suse.de> wrote:

> Hi,
> 
> I was just looking around the page fault code for any obvious 
> performance improvements. I noticed do_page_fault is rather big, 
> uses a lot of stack, and generates some branch mispredicts.
> 
> It's only about 1.1% on the profile of the workload I'm looking at, 
> so my improvement is pretty close to in the noise, but I wonder if 
> micro optimisations like the following would be welcome?

it's definitely welcome!

> This patch adds branch hints and moves error condition code out of 
> line. It shrinks do_page_fault from 2410 bytes to 603 bytes, and 
> from 352 to 64 bytes of stack. Total text size does grow by about 
> 500 bytes due to the additional functions added.

Some small cleanliness nits:

> +/* TODO: match order of arguments */

please fix TODO ;)

> +static noinline void no_context(struct pt_regs *regs,
> +			unsigned long error_code, unsigned long address)
> +{
> +	struct task_struct *tsk = current;
> +#ifdef CONFIG_X86_64
> +	unsigned long flags;
> +#endif

we should just do this unconditionally on 32-bit too.

> +	/*
> +	 * Oops. The kernel tried to access some bad page. We'll have to
> +	 * terminate things with extreme prejudice.
> +	 */
> +#ifdef CONFIG_X86_32
> +	bust_spinlocks(1);
> +#else
> +	flags = oops_begin();
> +#endif

32-bit should use oops_begin() too. Solves the previous comment as 
well.

> +#ifdef CONFIG_X86_32
> +	die("Oops", regs, error_code);
> +	bust_spinlocks(0);
> +	do_exit(SIGKILL);
> +#else
> +	if (__die("Oops", regs, error_code))
> +		regs = NULL;
> +	/* Executive summary in case the body of the oops scrolled away */
> +	printk(KERN_EMERG "CR2: %016lx\n", address);
> +	oops_end(flags, regs, SIGKILL);
> +#endif

this difference seems unnecessary too - 32-bit should use oops_end() 
too.

> +/* TODO: fixup for oom handling */

please fix todo ;-)

this flow could be cleaned up further:

[...]
> +		bad_area(regs, error_code, address);
> +		return;
[...]
> +		bad_area(regs, error_code, address);
> +		return;
[...]
> +			bad_area(regs, error_code, address);
> +			return;
[...]
> +		bad_area(regs, error_code, address);
> +		return;

Any reason why that pattern shouldnt be changed to an appropriate goto 
bad_area? (probably the same goes for the nosemaphore error paths too)

	Ingo

Re: [rfc] x86: optimise page fault path a little

[Nick Piggin]

On Thu, Nov 13, 2008 at 08:41:09AM +0100, Ingo Molnar wrote:
> 
> * Nick Piggin <npiggin@suse.de> wrote:
> 
> > Hi,
> > 
> > I was just looking around the page fault code for any obvious 
> > performance improvements. I noticed do_page_fault is rather big, 
> > uses a lot of stack, and generates some branch mispredicts.
> > 
> > It's only about 1.1% on the profile of the workload I'm looking at, 
> > so my improvement is pretty close to in the noise, but I wonder if 
> > micro optimisations like the following would be welcome?
> 
> it's definitely welcome!
> 
> > This patch adds branch hints and moves error condition code out of 
> > line. It shrinks do_page_fault from 2410 bytes to 603 bytes, and 
> > from 352 to 64 bytes of stack. Total text size does grow by about 
> > 500 bytes due to the additional functions added.
> 
> Some small cleanliness nits:
> 
> > +/* TODO: match order of arguments */
> 
> please fix TODO ;)

OK, I actually already fixed it but forgot to remove that. Basically just
checking whether keeping regs,error_code,address always passed in the
same order reduces code size (there were one or two places already that
did different order for no good reason). Not surprisingly, it saved a
few bytes.

 
> > +static noinline void no_context(struct pt_regs *regs,
> > +			unsigned long error_code, unsigned long address)
> > +{
> > +	struct task_struct *tsk = current;
> > +#ifdef CONFIG_X86_64
> > +	unsigned long flags;
> > +#endif
> 
> we should just do this unconditionally on 32-bit too.
> 
> > +	/*
> > +	 * Oops. The kernel tried to access some bad page. We'll have to
> > +	 * terminate things with extreme prejudice.
> > +	 */
> > +#ifdef CONFIG_X86_32
> > +	bust_spinlocks(1);
> > +#else
> > +	flags = oops_begin();
> > +#endif
> 
> 32-bit should use oops_begin() too. Solves the previous comment as 
> well.
> 
> > +#ifdef CONFIG_X86_32
> > +	die("Oops", regs, error_code);
> > +	bust_spinlocks(0);
> > +	do_exit(SIGKILL);
> > +#else
> > +	if (__die("Oops", regs, error_code))
> > +		regs = NULL;
> > +	/* Executive summary in case the body of the oops scrolled away */
> > +	printk(KERN_EMERG "CR2: %016lx\n", address);
> > +	oops_end(flags, regs, SIGKILL);
> > +#endif
> 
> this difference seems unnecessary too - 32-bit should use oops_end() 
> too.

Probably all 3 good comments, but I didn't want to be tempted into changing
behaviour (modulo adding bugs). Easy to merge them up in a subsequent patch,
however...

 
> > +/* TODO: fixup for oom handling */
> 
> please fix todo ;-)

Oh... actually it's OK (we don't loop back again, but that's OK, the init
task will just retry the fault). The comment was actually just in relation
to my oom-killer page fault patches that are in -mm. I'm not proposing
this for merge just yet, but will wait for Andrew.

 
> this flow could be cleaned up further:
> 
> [...]
> > +		bad_area(regs, error_code, address);
> > +		return;
> [...]
> > +		bad_area(regs, error_code, address);
> > +		return;
> [...]
> > +			bad_area(regs, error_code, address);
> > +			return;
> [...]
> > +		bad_area(regs, error_code, address);
> > +		return;
> 
> Any reason why that pattern shouldnt be changed to an appropriate goto 
> bad_area? (probably the same goes for the nosemaphore error paths too)

No... not really any reason. gcc effectively turns the code into a goto
anyway. Some days I can't make up my mind which one is better :)

Re: [rfc] x86: optimise page fault path a little

[Ingo Molnar]

* Nick Piggin <npiggin@suse.de> wrote:

> [...] The comment was actually just in relation to my oom-killer 
> page fault patches that are in -mm. I'm not proposing this for merge 
> just yet, but will wait for Andrew.

We definitely want to carry and test this in the x86 tree, so we 
should either move the x86 bits of your oom-killer page fault patches 
from -mm into the x86 tree - or (if that's not possible due to 
cross-arch impact) this patch should be rebased to -git and the 
oom-killer patches based ontop of that.

Below is a first (completely untested!) raw attempt at porting your 
patch to -git. (I resolved a good deal of conflicts without testing 
the end result, so take care.)

	Ingo

---------------->
Subject: x86: optimise page fault path a little
From: Nick Piggin <npiggin@suse.de>
Date: Thu, 13 Nov 2008 08:28:21 +0100

I was just looking around the page fault code for any obvious performance
improvements. I noticed do_page_fault is rather big, uses a lot of stack,
and generates some branch mispredicts.

It's only about 1.1% on the profile of the workload I'm looking at, so my
improvement is pretty close to in the noise, but I wonder if micro
optimisations like the following would be welcome?

This patch adds branch hints and moves error condition code out of line.
It shrinks do_page_fault from 2410 bytes to 603 bytes, and from 352 to 64
bytes of stack. Total text size does grow by about 500 bytes due to the
additional functions added.

---
 arch/x86/mm/fault.c |  439 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 251 insertions(+), 188 deletions(-)

Index: tip/arch/x86/mm/fault.c
===================================================================
--- tip.orig/arch/x86/mm/fault.c
+++ tip/arch/x86/mm/fault.c
@@ -93,8 +93,8 @@ static inline int notify_page_fault(stru
  *
  * Opcode checker based on code by Richard Brunner
  */
-static int is_prefetch(struct pt_regs *regs, unsigned long addr,
-		       unsigned long error_code)
+static int is_prefetch(struct pt_regs *regs, unsigned long error_code,
+			unsigned long addr)
 {
 	unsigned char *instr;
 	int scan_more = 1;
@@ -411,17 +411,16 @@ static void show_fault_oops(struct pt_re
 }
 
 #ifdef CONFIG_X86_64
-static noinline void pgtable_bad(unsigned long address, struct pt_regs *regs,
-				 unsigned long error_code)
+static noinline void pgtable_bad(struct pt_regs *regs,
+			 unsigned long error_code, unsigned long address)
 {
 	unsigned long flags = oops_begin();
 	int sig = SIGKILL;
-	struct task_struct *tsk;
+	struct task_struct *tsk = current;
 
 	printk(KERN_ALERT "%s: Corrupted page table at address %lx\n",
-	       current->comm, address);
+	       tsk->comm, address);
 	dump_pagetable(address);
-	tsk = current;
 	tsk->thread.cr2 = address;
 	tsk->thread.trap_no = 14;
 	tsk->thread.error_code = error_code;
@@ -431,6 +430,197 @@ static noinline void pgtable_bad(unsigne
 }
 #endif
 
+static noinline void no_context(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address)
+{
+	struct task_struct *tsk = current;
+#ifdef CONFIG_X86_64
+	unsigned long flags;
+#endif
+
+	/* Are we prepared to handle this kernel fault?  */
+	if (fixup_exception(regs))
+		return;
+
+	/*
+	 * X86_32
+	 * Valid to do another page fault here, because if this fault
+	 * had been triggered by is_prefetch fixup_exception would have
+	 * handled it.
+	 *
+	 * X86_64
+	 * Hall of shame of CPU/BIOS bugs.
+	 */
+	if (is_prefetch(regs, error_code, address))
+		return;
+
+	if (is_errata93(regs, address))
+		return;
+
+	/*
+	 * Oops. The kernel tried to access some bad page. We'll have to
+	 * terminate things with extreme prejudice.
+	 */
+#ifdef CONFIG_X86_32
+	bust_spinlocks(1);
+#else
+	flags = oops_begin();
+#endif
+
+	show_fault_oops(regs, error_code, address);
+
+	tsk->thread.cr2 = address;
+	tsk->thread.trap_no = 14;
+	tsk->thread.error_code = error_code;
+
+#ifdef CONFIG_X86_32
+	die("Oops", regs, error_code);
+	bust_spinlocks(0);
+	do_exit(SIGKILL);
+#else
+	if (__die("Oops", regs, error_code))
+		regs = NULL;
+	/* Executive summary in case the body of the oops scrolled away */
+	printk(KERN_EMERG "CR2: %016lx\n", address);
+	oops_end(flags, regs, SIGKILL);
+#endif
+}
+
+static void __bad_area_nosemaphore(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address,
+			int si_code)
+{
+	struct task_struct *tsk = current;
+
+	/* User mode accesses just cause a SIGSEGV */
+	if (error_code & PF_USER) {
+		/*
+		 * It's possible to have interrupts off here.
+		 */
+		local_irq_enable();
+
+		/*
+		 * Valid to do another page fault here because this one came
+		 * from user space.
+		 */
+		if (is_prefetch(regs, error_code, address))
+			return;
+
+		if (is_errata100(regs, address))
+			return;
+
+		if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
+		    printk_ratelimit()) {
+			printk(
+			"%s%s[%d]: segfault at %lx ip %p sp %p error %lx",
+			task_pid_nr(tsk) > 1 ? KERN_INFO : KERN_EMERG,
+			tsk->comm, task_pid_nr(tsk), address,
+			(void *) regs->ip, (void *) regs->sp, error_code);
+			print_vma_addr(" in ", regs->ip);
+			printk("\n");
+		}
+
+		tsk->thread.cr2 = address;
+		/* Kernel addresses are always protection faults */
+		tsk->thread.error_code = error_code | (address >= TASK_SIZE);
+		tsk->thread.trap_no = 14;
+		force_sig_info_fault(SIGSEGV, si_code, address, tsk);
+		return;
+	}
+
+	if (is_f00f_bug(regs, address))
+		return;
+
+	no_context(regs, error_code, address);
+}
+
+static noinline void bad_area_nosemaphore(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address)
+{
+	__bad_area_nosemaphore(regs, error_code, address, SEGV_MAPERR);
+}
+
+static void __bad_area(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address,
+			int si_code)
+{
+	struct mm_struct *mm = current->mm;
+
+	/*
+	 * Something tried to access memory that isn't in our memory map..
+	 * Fix it, but check if it's kernel or user first..
+	 */
+	up_read(&mm->mmap_sem);
+
+	__bad_area_nosemaphore(regs, error_code, address, si_code);
+}
+
+static noinline void bad_area(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address)
+{
+	__bad_area(regs, error_code, address, SEGV_MAPERR);
+}
+
+static noinline void bad_area_accerr(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address)
+{
+	__bad_area(regs, error_code, address, SEGV_ACCERR);
+}
+
+static void out_of_memory(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address)
+{
+	struct task_struct *tsk = current;
+	struct mm_struct *mm = tsk->mm;
+	/*
+	 * We ran out of memory, or some other thing happened to us that made
+	 * us unable to handle the page fault gracefully.
+	 */
+	up_read(&mm->mmap_sem);
+	if (is_global_init(tsk)) {
+		yield();
+		return;
+	}
+
+	printk("VM: killing process %s\n", tsk->comm);
+	if (error_code & PF_USER)
+		do_group_exit(SIGKILL);
+	no_context(regs, error_code, address);
+}
+
+static void do_sigbus(struct pt_regs *regs,
+			unsigned long error_code, unsigned long address)
+{
+	struct task_struct *tsk = current;
+	struct mm_struct *mm = tsk->mm;
+
+	up_read(&mm->mmap_sem);
+
+	/* Kernel mode? Handle exceptions or die */
+	if (!(error_code & PF_USER))
+		no_context(regs, error_code, address);
+#ifdef CONFIG_X86_32
+	/* User space => ok to do another page fault */
+	if (is_prefetch(regs, error_code, address))
+		return;
+#endif
+	tsk->thread.cr2 = address;
+	tsk->thread.error_code = error_code;
+	tsk->thread.trap_no = 14;
+	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk);
+}
+
+static noinline void mm_fault_error(struct pt_regs *regs,
+		unsigned long error_code, unsigned long address, unsigned int fault)
+{
+	if (fault & VM_FAULT_OOM)
+		out_of_memory(regs, error_code, address);
+	else if (fault & VM_FAULT_SIGBUS)
+		do_sigbus(regs, error_code, address);
+	else
+		BUG();
+}
+
 static int spurious_fault_check(unsigned long error_code, pte_t *pte)
 {
 	if ((error_code & PF_WRITE) && !pte_write(*pte))
@@ -450,8 +640,8 @@ static int spurious_fault_check(unsigned
  * There are no security implications to leaving a stale TLB when
  * increasing the permissions on a page.
  */
-static int spurious_fault(unsigned long address,
-			  unsigned long error_code)
+static noinline int spurious_fault(unsigned long error_code,
+				unsigned long address)
 {
 	pgd_t *pgd;
 	pud_t *pud;
@@ -496,7 +686,7 @@ static int spurious_fault(unsigned long 
  *
  * This assumes no large pages in there.
  */
-static int vmalloc_fault(unsigned long address)
+static noinline int vmalloc_fault(unsigned long address)
 {
 #ifdef CONFIG_X86_32
 	unsigned long pgd_paddr;
@@ -585,18 +775,15 @@ asmlinkage
 #endif
 void __kprobes do_page_fault(struct pt_regs *regs, unsigned long error_code)
 {
+	unsigned long address;
 	struct task_struct *tsk;
 	struct mm_struct *mm;
 	struct vm_area_struct *vma;
-	unsigned long address;
 	int write, si_code;
 	int fault;
 	unsigned long *stackend;
 
-#ifdef CONFIG_X86_64
-	unsigned long flags;
 	int sig;
-#endif
 
 	tsk = current;
 	mm = tsk->mm;
@@ -614,7 +801,7 @@ void __kprobes do_page_fault(struct pt_r
 	if(kmemcheck_active(regs))
 		kmemcheck_hide(regs);
 
-	if (notify_page_fault(regs))
+	if (unlikely(notify_page_fault(regs)))
 		return;
 	if (unlikely(kmmio_fault(regs, address)))
 		return;
@@ -653,7 +840,8 @@ void __kprobes do_page_fault(struct pt_r
 		 * Don't take the mm semaphore here. If we fixup a prefetch
 		 * fault we could otherwise deadlock.
 		 */
-		goto bad_area_nosemaphore;
+		bad_area_nosemaphore(regs, error_code, address);
+		return;
 	}
 
 
@@ -672,17 +860,18 @@ void __kprobes do_page_fault(struct pt_r
 
 #ifdef CONFIG_X86_64
 	if (unlikely(error_code & PF_RSVD))
-		pgtable_bad(address, regs, error_code);
+		pgtable_bad(regs, error_code, address);
 #endif
 
 	/*
 	 * 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 (unlikely(in_atomic() || !mm))
-		goto bad_area_nosemaphore;
+	if (unlikely(in_atomic() || !mm)) {
+		bad_area_nosemaphore(regs, error_code, address);
+		return;
+	}
 
-again:
 	/*
 	 * When running in the kernel we expect faults to occur only to
 	 * addresses in user space.  All other faults represent errors in the
@@ -699,20 +888,26 @@ again:
 	 * source.  If this is invalid we can skip the address space check,
 	 * thus avoiding the deadlock.
 	 */
-	if (!down_read_trylock(&mm->mmap_sem)) {
+	if (unlikely(!down_read_trylock(&mm->mmap_sem))) {
 		if ((error_code & PF_USER) == 0 &&
-		    !search_exception_tables(regs->ip))
-			goto bad_area_nosemaphore;
+		    !search_exception_tables(regs->ip)) {
+			bad_area_nosemaphore(regs, error_code, address);
+			return;
+		}
 		down_read(&mm->mmap_sem);
 	}
 
 	vma = find_vma(mm, address);
-	if (!vma)
-		goto bad_area;
-	if (vma->vm_start <= address)
+	if (unlikely(!vma)) {
+		bad_area(regs, error_code, address);
+		return;
+	}
+	if (likely(vma->vm_start <= address))
 		goto good_area;
-	if (!(vma->vm_flags & VM_GROWSDOWN))
-		goto bad_area;
+	if (unlikely(!(vma->vm_flags & VM_GROWSDOWN))) {
+		bad_area(regs, error_code, address);
+		return;
+	}
 	if (error_code & PF_USER) {
 		/*
 		 * Accessing the stack below %sp is always a bug.
@@ -720,31 +915,34 @@ again:
 		 * and pusha to work.  ("enter $65535,$31" pushes
 		 * 32 pointers and then decrements %sp by 65535.)
 		 */
-		if (address + 65536 + 32 * sizeof(unsigned long) < regs->sp)
-			goto bad_area;
+		if (unlikely(address + 65536 + 32 * sizeof(unsigned long) < regs->sp)) {
+			bad_area(regs, error_code, address);
+			return;
+		}
 	}
-	if (expand_stack(vma, address))
-		goto bad_area;
-/*
- * Ok, we have a good vm_area for this memory access, so
- * we can handle it..
- */
+	if (unlikely(expand_stack(vma, address))) {
+		bad_area(regs, error_code, address);
+		return;
+	}
+
+	/*
+	 * Ok, we have a good vm_area for this memory access, so
+	 * we can handle it..
+	 */
 good_area:
 	si_code = SEGV_ACCERR;
-	write = 0;
-	switch (error_code & (PF_PROT|PF_WRITE)) {
-	default:	/* 3: write, present */
-		/* fall through */
-	case PF_WRITE:		/* write, not present */
-		if (!(vma->vm_flags & VM_WRITE))
-			goto bad_area;
-		write++;
-		break;
-	case PF_PROT:		/* read, present */
-		goto bad_area;
-	case 0:			/* read, not present */
-		if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))
-			goto bad_area;
+	write = error_code & PF_WRITE;
+	if (write) {
+		if (unlikely(!(vma->vm_flags & VM_WRITE))) {
+			bad_area_accerr(regs, error_code, address);
+			return;
+		}
+	} else if (unlikely(error_code & PF_PROT)) {
+		bad_area_accerr(regs, error_code, address);
+		return;
+	} else if (unlikely(!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))) {
+		bad_area_accerr(regs, error_code, address);
+		return;
 	}
 
 	/*
@@ -754,11 +952,8 @@ good_area:
 	 */
 	fault = handle_mm_fault(mm, vma, address, write);
 	if (unlikely(fault & VM_FAULT_ERROR)) {
-		if (fault & VM_FAULT_OOM)
-			goto out_of_memory;
-		else if (fault & VM_FAULT_SIGBUS)
-			goto do_sigbus;
-		BUG();
+		mm_fault_error(regs, error_code, address, fault);
+		return;
 	}
 	if (fault & VM_FAULT_MAJOR)
 		tsk->maj_flt++;
@@ -769,150 +964,18 @@ good_area:
 	/*
 	 * Did it hit the DOS screen memory VA from vm86 mode?
 	 */
-	if (v8086_mode(regs)) {
+	if (unlikely(v8086_mode(regs))) {
 		unsigned long bit = (address - 0xA0000) >> PAGE_SHIFT;
 		if (bit < 32)
 			tsk->thread.screen_bitmap |= 1 << bit;
 	}
 #endif
 	up_read(&mm->mmap_sem);
-	return;
-
-/*
- * Something tried to access memory that isn't in our memory map..
- * Fix it, but check if it's kernel or user first..
- */
-bad_area:
-	up_read(&mm->mmap_sem);
-
-bad_area_nosemaphore:
-	/* User mode accesses just cause a SIGSEGV */
-	if (error_code & PF_USER) {
-		/*
-		 * It's possible to have interrupts off here.
-		 */
-		local_irq_enable();
-
-		/*
-		 * Valid to do another page fault here because this one came
-		 * from user space.
-		 */
-		if (is_prefetch(regs, address, error_code))
-			return;
-
-		if (is_errata100(regs, address))
-			return;
-
-		if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
-		    printk_ratelimit()) {
-			printk(
-			"%s%s[%d]: segfault at %lx ip %p sp %p error %lx",
-			task_pid_nr(tsk) > 1 ? KERN_INFO : KERN_EMERG,
-			tsk->comm, task_pid_nr(tsk), address,
-			(void *) regs->ip, (void *) regs->sp, error_code);
-			print_vma_addr(" in ", regs->ip);
-			printk("\n");
-		}
-
-		tsk->thread.cr2 = address;
-		/* Kernel addresses are always protection faults */
-		tsk->thread.error_code = error_code | (address >= TASK_SIZE);
-		tsk->thread.trap_no = 14;
-		force_sig_info_fault(SIGSEGV, si_code, address, tsk);
-		return;
-	}
-
-	if (is_f00f_bug(regs, address))
-		return;
-
-no_context:
-	/* Are we prepared to handle this kernel fault?  */
-	if (fixup_exception(regs))
-		return;
-
-	/*
-	 * X86_32
-	 * Valid to do another page fault here, because if this fault
-	 * had been triggered by is_prefetch fixup_exception would have
-	 * handled it.
-	 *
-	 * X86_64
-	 * Hall of shame of CPU/BIOS bugs.
-	 */
-	if (is_prefetch(regs, address, error_code))
-		return;
-
-	if (is_errata93(regs, address))
-		return;
-
-/*
- * Oops. The kernel tried to access some bad page. We'll have to
- * terminate things with extreme prejudice.
- */
-#ifdef CONFIG_X86_32
-	bust_spinlocks(1);
-#else
-	flags = oops_begin();
-#endif
-
-	show_fault_oops(regs, error_code, address);
-
  	stackend = end_of_stack(tsk);
 	if (*stackend != STACK_END_MAGIC)
 		printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
 
-	tsk->thread.cr2 = address;
-	tsk->thread.trap_no = 14;
-	tsk->thread.error_code = error_code;
-
-#ifdef CONFIG_X86_32
-	die("Oops", regs, error_code);
-	bust_spinlocks(0);
-	do_exit(SIGKILL);
-#else
 	sig = SIGKILL;
-	if (__die("Oops", regs, error_code))
-		sig = 0;
-	/* Executive summary in case the body of the oops scrolled away */
-	printk(KERN_EMERG "CR2: %016lx\n", address);
-	oops_end(flags, regs, sig);
-#endif
-
-/*
- * We ran out of memory, or some other thing happened to us that made
- * us unable to handle the page fault gracefully.
- */
-out_of_memory:
-	up_read(&mm->mmap_sem);
-	if (is_global_init(tsk)) {
-		yield();
-		/*
-		 * Re-lookup the vma - in theory the vma tree might
-		 * have changed:
-		 */
-		goto again;
-	}
-
-	printk("VM: killing process %s\n", tsk->comm);
-	if (error_code & PF_USER)
-		do_group_exit(SIGKILL);
-	goto no_context;
-
-do_sigbus:
-	up_read(&mm->mmap_sem);
-
-	/* Kernel mode? Handle exceptions or die */
-	if (!(error_code & PF_USER))
-		goto no_context;
-#ifdef CONFIG_X86_32
-	/* User space => ok to do another page fault */
-	if (is_prefetch(regs, address, error_code))
-		return;
-#endif
-	tsk->thread.cr2 = address;
-	tsk->thread.error_code = error_code;
-	tsk->thread.trap_no = 14;
-	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk);
 }
 
 DEFINE_SPINLOCK(pgd_lock);

Re: [rfc] x86: optimise page fault path a little

[Andi Kleen]

> this flow could be cleaned up further:
> 
> [...]
> > +		bad_area(regs, error_code, address);
> > +		return;
> [...]
> > +		bad_area(regs, error_code, address);
> > +		return;
> [...]
> > +			bad_area(regs, error_code, address);
> > +			return;
> [...]
> > +		bad_area(regs, error_code, address);
> > +		return;
> 
> Any reason why that pattern shouldnt be changed to an appropriate goto 
> bad_area? (probably the same goes for the nosemaphore error paths too)

The tail call is already effectively a goto.

-Andi

-- 
ak@linux.intel.com

Re: [rfc] x86: optimise page fault path a little

[Linus Torvalds]

On Thu, 13 Nov 2008, Nick Piggin wrote:
> 
> It's only about 1.1% on the profile of the workload I'm looking at, so my
> improvement is pretty close to in the noise, but I wonder if micro
> optimisations like the following would be welcome?

I think splitting it up is good, but I hate how your split-up ends up also 
splitting the locking (ie now you do a "down_read()" and "up_read()" in 
different functions).

I also think that to some degree you made it less readable, particularly 
this area:

	+       if (write) {
	+               if (unlikely(!(vma->vm_flags & VM_WRITE))) {
	+                       bad_area_accerr(regs, error_code, address);
	+                       return;
	+               }
	+       } else if (unlikely(error_code & PF_PROT)) {
	+               bad_area_accerr(regs, error_code, address);
	+               return;
	+       } else if (unlikely(!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))) {
	+               bad_area_accerr(regs, error_code, address);
	+               return;

makes me go "whaa?" and I wonder if it wouldn't be nicer to have one 
complex conditional hidden in an inline function, and then just have

	if (unlikely(access_error(write, error_code, vma))) {
		bad_area_access_error(regs, error_code, address);
		return;
	}

where the point is that we don't want to duplicate the error case three 
times, and that "accerr" is bad naming.

IOW, I do think that the patch looks like a step in the right direction, 
but cleanliness should be a primary concern.

		Linus

Re: [rfc] x86: optimise page fault path a little

[Ingo Molnar]

* Nick Piggin <npiggin@suse.de> wrote:

> > 32-bit should use oops_begin() too. Solves the previous comment as 
> > well.
> > 
> > > +#ifdef CONFIG_X86_32
> > > +	die("Oops", regs, error_code);
> > > +	bust_spinlocks(0);
> > > +	do_exit(SIGKILL);
> > > +#else
> > > +	if (__die("Oops", regs, error_code))
> > > +		regs = NULL;
> > > +	/* Executive summary in case the body of the oops scrolled away */
> > > +	printk(KERN_EMERG "CR2: %016lx\n", address);
> > > +	oops_end(flags, regs, SIGKILL);
> > > +#endif
> > 
> > this difference seems unnecessary too - 32-bit should use oops_end() 
> > too.
> 
> Probably all 3 good comments, but I didn't want to be tempted into 
> changing behaviour (modulo adding bugs). Easy to merge them up in a 
> subsequent patch, however...

please do feel tempted to clean this code up - and if it changes 
behavior, split it up into smaller steps. The patch is already quite 
large with a flux of 430 lines:

 1 file changed, 251 insertions(+), 188 deletions(-)

... your primary goal is faster code, we also want cleaner code. I 
think we can meet in the middle, have faster _and_ cleaner code, and 
it's a done deal ;)

	Ingo

Re: [rfc] x86: optimise page fault path a little

[Nick Piggin]

On Thu, Nov 13, 2008 at 05:06:33PM +0100, Ingo Molnar wrote:
> 
> * Nick Piggin <npiggin@suse.de> wrote:
> 
> > > 32-bit should use oops_begin() too. Solves the previous comment as 
> > > well.
> > > 
> > > > +#ifdef CONFIG_X86_32
> > > > +	die("Oops", regs, error_code);
> > > > +	bust_spinlocks(0);
> > > > +	do_exit(SIGKILL);
> > > > +#else
> > > > +	if (__die("Oops", regs, error_code))
> > > > +		regs = NULL;
> > > > +	/* Executive summary in case the body of the oops scrolled away */
> > > > +	printk(KERN_EMERG "CR2: %016lx\n", address);
> > > > +	oops_end(flags, regs, SIGKILL);
> > > > +#endif
> > > 
> > > this difference seems unnecessary too - 32-bit should use oops_end() 
> > > too.
> > 
> > Probably all 3 good comments, but I didn't want to be tempted into 
> > changing behaviour (modulo adding bugs). Easy to merge them up in a 
> > subsequent patch, however...
> 
> please do feel tempted to clean this code up - and if it changes 
> behavior, split it up into smaller steps. The patch is already quite 
> large with a flux of 430 lines:
> 
>  1 file changed, 251 insertions(+), 188 deletions(-)
> 
> ... your primary goal is faster code, we also want cleaner code. I 
> think we can meet in the middle, have faster _and_ cleaner code, and 
> it's a done deal ;)

I might. But the flux is "supposed" to just be moving things around so
it compiles better. Any behaviour change should be a bug.

So I definitely won't add any of these types of cleanups in the same
patch. And with or without cleanups, the patch should stand on its own
to get merged -- if it's good, it's good; if not, not :)
 
That, and, I don't really know the subtlties of this code or what
made it so tricky that such a cleanup wasn't done when merging the
two files.

Re: [rfc] x86: optimise page fault path a little

[Nick Piggin]

On Thu, Nov 13, 2008 at 09:02:47AM +0100, Ingo Molnar wrote:
> 
> * Nick Piggin <npiggin@suse.de> wrote:
> 
> > [...] The comment was actually just in relation to my oom-killer 
> > page fault patches that are in -mm. I'm not proposing this for merge 
> > just yet, but will wait for Andrew.
> 
> We definitely want to carry and test this in the x86 tree, so we 
> should either move the x86 bits of your oom-killer page fault patches 
> from -mm into the x86 tree - or (if that's not possible due to 
> cross-arch impact) this patch should be rebased to -git and the 
> oom-killer patches based ontop of that.

The thing is that the oom killer patches solve more of a pressing
problem (that oom from page faults doesn't obey any of our oom
heuristics eg. panic_on_oom or never-kill-this-task, which can cause
things to not failover properly, or X to get killed etc etc.)

So I definitely want that patch in... this patch OTOH is great, but
nobody is actually going to notice, seriously. It is simply
something that we should all be doing all the time, and once 1000
such patches are merged then the kernel might be 1% faster :)

Trust me, there is no shortage of fastpaths that are stupidly bloated
in Linux. Just from gcc being smart, and people not watching the
generated code :)

 
> Below is a first (completely untested!) raw attempt at porting your 
> patch to -git. (I resolved a good deal of conflicts without testing 
> the end result, so take care.)
> 
> 	Ingo
> 
> ---------------->
> Subject: x86: optimise page fault path a little
> From: Nick Piggin <npiggin@suse.de>
> Date: Thu, 13 Nov 2008 08:28:21 +0100
> 
> I was just looking around the page fault code for any obvious performance
> improvements. I noticed do_page_fault is rather big, uses a lot of stack,
> and generates some branch mispredicts.
> 
> It's only about 1.1% on the profile of the workload I'm looking at, so my
> improvement is pretty close to in the noise, but I wonder if micro
> optimisations like the following would be welcome?
> 
> This patch adds branch hints and moves error condition code out of line.
> It shrinks do_page_fault from 2410 bytes to 603 bytes, and from 352 to 64
> bytes of stack. Total text size does grow by about 500 bytes due to the
> additional functions added.
> 
> ---
>  arch/x86/mm/fault.c |  439 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 251 insertions(+), 188 deletions(-)
> 
> Index: tip/arch/x86/mm/fault.c
> ===================================================================
> --- tip.orig/arch/x86/mm/fault.c
> +++ tip/arch/x86/mm/fault.c
> @@ -93,8 +93,8 @@ static inline int notify_page_fault(stru
>   *
>   * Opcode checker based on code by Richard Brunner
>   */
> -static int is_prefetch(struct pt_regs *regs, unsigned long addr,
> -		       unsigned long error_code)
> +static int is_prefetch(struct pt_regs *regs, unsigned long error_code,
> +			unsigned long addr)
>  {
>  	unsigned char *instr;
>  	int scan_more = 1;
> @@ -411,17 +411,16 @@ static void show_fault_oops(struct pt_re
>  }
>  
>  #ifdef CONFIG_X86_64
> -static noinline void pgtable_bad(unsigned long address, struct pt_regs *regs,
> -				 unsigned long error_code)
> +static noinline void pgtable_bad(struct pt_regs *regs,
> +			 unsigned long error_code, unsigned long address)
>  {
>  	unsigned long flags = oops_begin();
>  	int sig = SIGKILL;
> -	struct task_struct *tsk;
> +	struct task_struct *tsk = current;
>  
>  	printk(KERN_ALERT "%s: Corrupted page table at address %lx\n",
> -	       current->comm, address);
> +	       tsk->comm, address);
>  	dump_pagetable(address);
> -	tsk = current;
>  	tsk->thread.cr2 = address;
>  	tsk->thread.trap_no = 14;
>  	tsk->thread.error_code = error_code;
> @@ -431,6 +430,197 @@ static noinline void pgtable_bad(unsigne
>  }
>  #endif
>  
> +static noinline void no_context(struct pt_regs *regs,
> +			unsigned long error_code, unsigned long address)
> +{
> +	struct task_struct *tsk = current;
> +#ifdef CONFIG_X86_64
> +	unsigned long flags;
> +#endif
> +
> +	/* Are we prepared to handle this kernel fault?  */
> +	if (fixup_exception(regs))
> +		return;
> +
> +	/*
> +	 * X86_32
> +	 * Valid to do another page fault here, because if this fault
> +	 * had been triggered by is_prefetch fixup_exception would have
> +	 * handled it.
> +	 *
> +	 * X86_64
> +	 * Hall of shame of CPU/BIOS bugs.
> +	 */
> +	if (is_prefetch(regs, error_code, address))
> +		return;
> +
> +	if (is_errata93(regs, address))
> +		return;
> +
> +	/*
> +	 * Oops. The kernel tried to access some bad page. We'll have to
> +	 * terminate things with extreme prejudice.
> +	 */
> +#ifdef CONFIG_X86_32
> +	bust_spinlocks(1);
> +#else
> +	flags = oops_begin();
> +#endif
> +
> +	show_fault_oops(regs, error_code, address);
> +
> +	tsk->thread.cr2 = address;
> +	tsk->thread.trap_no = 14;
> +	tsk->thread.error_code = error_code;
> +
> +#ifdef CONFIG_X86_32
> +	die("Oops", regs, error_code);
> +	bust_spinlocks(0);
> +	do_exit(SIGKILL);
> +#else
> +	if (__die("Oops", regs, error_code))
> +		regs = NULL;
> +	/* Executive summary in case the body of the oops scrolled away */
> +	printk(KERN_EMERG "CR2: %016lx\n", address);
> +	oops_end(flags, regs, SIGKILL);
> +#endif
> +}
> +
> +static void __bad_area_nosemaphore(struct pt_regs *regs,
> +			unsigned long error_code, unsigned long address,
> +			int si_code)
> +{
> +	struct task_struct *tsk = current;
> +
> +	/* User mode accesses just cause a SIGSEGV */
> +	if (error_code & PF_USER) {
> +		/*
> +		 * It's possible to have interrupts off here.
> +		 */
> +		local_irq_enable();
> +
> +		/*
> +		 * Valid to do another page fault here because this one came
> +		 * from user space.
> +		 */
> +		if (is_prefetch(regs, error_code, address))
> +			return;
> +
> +		if (is_errata100(regs, address))
> +			return;
> +
> +		if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
> +		    printk_ratelimit()) {
> +			printk(
> +			"%s%s[%d]: segfault at %lx ip %p sp %p error %lx",
> +			task_pid_nr(tsk) > 1 ? KERN_INFO : KERN_EMERG,
> +			tsk->comm, task_pid_nr(tsk), address,
> +			(void *) regs->ip, (void *) regs->sp, error_code);
> +			print_vma_addr(" in ", regs->ip);
> +			printk("\n");
> +		}
> +
> +		tsk->thread.cr2 = address;
> +		/* Kernel addresses are always protection faults */
> +		tsk->thread.error_code = error_code | (address >= TASK_SIZE);
> +		tsk->thread.trap_no = 14;
> +		force_sig_info_fault(SIGSEGV, si_code, address, tsk);
> +		return;
> +	}
> +
> +	if (is_f00f_bug(regs, address))
> +		return;
> +
> +	no_context(regs, error_code, address);
> +}
> +
> +static noinline void bad_area_nosemaphore(struct pt_regs *regs,
> +			unsigned long error_code, unsigned long address)
> +{
> +	__bad_area_nosemaphore(regs, error_code, address, SEGV_MAPERR);
> +}
> +
> +static void __bad_area(struct pt_regs *regs,
> +			unsigned long error_code, unsigned long address,
> +			int si_code)
> +{
> +	struct mm_struct *mm = current->mm;
> +
> +	/*
> +	 * Something tried to access memory that isn't in our memory map..
> +	 * Fix it, but check if it's kernel or user first..
> +	 */
> +	up_read(&mm->mmap_sem);
> +
> +	__bad_area_nosemaphore(regs, error_code, address, si_code);
> +}
> +
> +static noinline void bad_area(struct pt_regs *regs,
> +			unsigned long error_code, unsigned long address)
> +{
> +	__bad_area(regs, error_code, address, SEGV_MAPERR);
> +}
> +
> +static noinline void bad_area_accerr(struct pt_regs *regs,
> +			unsigned long error_code, unsigned long address)
> +{
> +	__bad_area(regs, error_code, address, SEGV_ACCERR);
> +}
> +
> +static void out_of_memory(struct pt_regs *regs,
> +			unsigned long error_code, unsigned long address)
> +{
> +	struct task_struct *tsk = current;
> +	struct mm_struct *mm = tsk->mm;
> +	/*
> +	 * We ran out of memory, or some other thing happened to us that made
> +	 * us unable to handle the page fault gracefully.
> +	 */
> +	up_read(&mm->mmap_sem);
> +	if (is_global_init(tsk)) {
> +		yield();
> +		return;
> +	}
> +
> +	printk("VM: killing process %s\n", tsk->comm);
> +	if (error_code & PF_USER)
> +		do_group_exit(SIGKILL);
> +	no_context(regs, error_code, address);
> +}
> +
> +static void do_sigbus(struct pt_regs *regs,
> +			unsigned long error_code, unsigned long address)
> +{
> +	struct task_struct *tsk = current;
> +	struct mm_struct *mm = tsk->mm;
> +
> +	up_read(&mm->mmap_sem);
> +
> +	/* Kernel mode? Handle exceptions or die */
> +	if (!(error_code & PF_USER))
> +		no_context(regs, error_code, address);
> +#ifdef CONFIG_X86_32
> +	/* User space => ok to do another page fault */
> +	if (is_prefetch(regs, error_code, address))
> +		return;
> +#endif
> +	tsk->thread.cr2 = address;
> +	tsk->thread.error_code = error_code;
> +	tsk->thread.trap_no = 14;
> +	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk);
> +}
> +
> +static noinline void mm_fault_error(struct pt_regs *regs,
> +		unsigned long error_code, unsigned long address, unsigned int fault)
> +{
> +	if (fault & VM_FAULT_OOM)
> +		out_of_memory(regs, error_code, address);
> +	else if (fault & VM_FAULT_SIGBUS)
> +		do_sigbus(regs, error_code, address);
> +	else
> +		BUG();
> +}
> +
>  static int spurious_fault_check(unsigned long error_code, pte_t *pte)
>  {
>  	if ((error_code & PF_WRITE) && !pte_write(*pte))
> @@ -450,8 +640,8 @@ static int spurious_fault_check(unsigned
>   * There are no security implications to leaving a stale TLB when
>   * increasing the permissions on a page.
>   */
> -static int spurious_fault(unsigned long address,
> -			  unsigned long error_code)
> +static noinline int spurious_fault(unsigned long error_code,
> +				unsigned long address)
>  {
>  	pgd_t *pgd;
>  	pud_t *pud;
> @@ -496,7 +686,7 @@ static int spurious_fault(unsigned long 
>   *
>   * This assumes no large pages in there.
>   */
> -static int vmalloc_fault(unsigned long address)
> +static noinline int vmalloc_fault(unsigned long address)
>  {
>  #ifdef CONFIG_X86_32
>  	unsigned long pgd_paddr;
> @@ -585,18 +775,15 @@ asmlinkage
>  #endif
>  void __kprobes do_page_fault(struct pt_regs *regs, unsigned long error_code)
>  {
> +	unsigned long address;
>  	struct task_struct *tsk;
>  	struct mm_struct *mm;
>  	struct vm_area_struct *vma;
> -	unsigned long address;
>  	int write, si_code;
>  	int fault;
>  	unsigned long *stackend;
>  
> -#ifdef CONFIG_X86_64
> -	unsigned long flags;
>  	int sig;
> -#endif
>  
>  	tsk = current;
>  	mm = tsk->mm;
> @@ -614,7 +801,7 @@ void __kprobes do_page_fault(struct pt_r
>  	if(kmemcheck_active(regs))
>  		kmemcheck_hide(regs);
>  
> -	if (notify_page_fault(regs))
> +	if (unlikely(notify_page_fault(regs)))
>  		return;
>  	if (unlikely(kmmio_fault(regs, address)))
>  		return;
> @@ -653,7 +840,8 @@ void __kprobes do_page_fault(struct pt_r
>  		 * Don't take the mm semaphore here. If we fixup a prefetch
>  		 * fault we could otherwise deadlock.
>  		 */
> -		goto bad_area_nosemaphore;
> +		bad_area_nosemaphore(regs, error_code, address);
> +		return;
>  	}
>  
>  
> @@ -672,17 +860,18 @@ void __kprobes do_page_fault(struct pt_r
>  
>  #ifdef CONFIG_X86_64
>  	if (unlikely(error_code & PF_RSVD))
> -		pgtable_bad(address, regs, error_code);
> +		pgtable_bad(regs, error_code, address);
>  #endif
>  
>  	/*
>  	 * 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 (unlikely(in_atomic() || !mm))
> -		goto bad_area_nosemaphore;
> +	if (unlikely(in_atomic() || !mm)) {
> +		bad_area_nosemaphore(regs, error_code, address);
> +		return;
> +	}
>  
> -again:
>  	/*
>  	 * When running in the kernel we expect faults to occur only to
>  	 * addresses in user space.  All other faults represent errors in the
> @@ -699,20 +888,26 @@ again:
>  	 * source.  If this is invalid we can skip the address space check,
>  	 * thus avoiding the deadlock.
>  	 */
> -	if (!down_read_trylock(&mm->mmap_sem)) {
> +	if (unlikely(!down_read_trylock(&mm->mmap_sem))) {
>  		if ((error_code & PF_USER) == 0 &&
> -		    !search_exception_tables(regs->ip))
> -			goto bad_area_nosemaphore;
> +		    !search_exception_tables(regs->ip)) {
> +			bad_area_nosemaphore(regs, error_code, address);
> +			return;
> +		}
>  		down_read(&mm->mmap_sem);
>  	}
>  
>  	vma = find_vma(mm, address);
> -	if (!vma)
> -		goto bad_area;
> -	if (vma->vm_start <= address)
> +	if (unlikely(!vma)) {
> +		bad_area(regs, error_code, address);
> +		return;
> +	}
> +	if (likely(vma->vm_start <= address))
>  		goto good_area;
> -	if (!(vma->vm_flags & VM_GROWSDOWN))
> -		goto bad_area;
> +	if (unlikely(!(vma->vm_flags & VM_GROWSDOWN))) {
> +		bad_area(regs, error_code, address);
> +		return;
> +	}
>  	if (error_code & PF_USER) {
>  		/*
>  		 * Accessing the stack below %sp is always a bug.
> @@ -720,31 +915,34 @@ again:
>  		 * and pusha to work.  ("enter $65535,$31" pushes
>  		 * 32 pointers and then decrements %sp by 65535.)
>  		 */
> -		if (address + 65536 + 32 * sizeof(unsigned long) < regs->sp)
> -			goto bad_area;
> +		if (unlikely(address + 65536 + 32 * sizeof(unsigned long) < regs->sp)) {
> +			bad_area(regs, error_code, address);
> +			return;
> +		}
>  	}
> -	if (expand_stack(vma, address))
> -		goto bad_area;
> -/*
> - * Ok, we have a good vm_area for this memory access, so
> - * we can handle it..
> - */
> +	if (unlikely(expand_stack(vma, address))) {
> +		bad_area(regs, error_code, address);
> +		return;
> +	}
> +
> +	/*
> +	 * Ok, we have a good vm_area for this memory access, so
> +	 * we can handle it..
> +	 */
>  good_area:
>  	si_code = SEGV_ACCERR;
> -	write = 0;
> -	switch (error_code & (PF_PROT|PF_WRITE)) {
> -	default:	/* 3: write, present */
> -		/* fall through */
> -	case PF_WRITE:		/* write, not present */
> -		if (!(vma->vm_flags & VM_WRITE))
> -			goto bad_area;
> -		write++;
> -		break;
> -	case PF_PROT:		/* read, present */
> -		goto bad_area;
> -	case 0:			/* read, not present */
> -		if (!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))
> -			goto bad_area;
> +	write = error_code & PF_WRITE;
> +	if (write) {
> +		if (unlikely(!(vma->vm_flags & VM_WRITE))) {
> +			bad_area_accerr(regs, error_code, address);
> +			return;
> +		}
> +	} else if (unlikely(error_code & PF_PROT)) {
> +		bad_area_accerr(regs, error_code, address);
> +		return;
> +	} else if (unlikely(!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))) {
> +		bad_area_accerr(regs, error_code, address);
> +		return;
>  	}
>  
>  	/*
> @@ -754,11 +952,8 @@ good_area:
>  	 */
>  	fault = handle_mm_fault(mm, vma, address, write);
>  	if (unlikely(fault & VM_FAULT_ERROR)) {
> -		if (fault & VM_FAULT_OOM)
> -			goto out_of_memory;
> -		else if (fault & VM_FAULT_SIGBUS)
> -			goto do_sigbus;
> -		BUG();
> +		mm_fault_error(regs, error_code, address, fault);
> +		return;
>  	}
>  	if (fault & VM_FAULT_MAJOR)
>  		tsk->maj_flt++;
> @@ -769,150 +964,18 @@ good_area:
>  	/*
>  	 * Did it hit the DOS screen memory VA from vm86 mode?
>  	 */
> -	if (v8086_mode(regs)) {
> +	if (unlikely(v8086_mode(regs))) {
>  		unsigned long bit = (address - 0xA0000) >> PAGE_SHIFT;
>  		if (bit < 32)
>  			tsk->thread.screen_bitmap |= 1 << bit;
>  	}
>  #endif
>  	up_read(&mm->mmap_sem);
> -	return;
> -
> -/*
> - * Something tried to access memory that isn't in our memory map..
> - * Fix it, but check if it's kernel or user first..
> - */
> -bad_area:
> -	up_read(&mm->mmap_sem);
> -
> -bad_area_nosemaphore:
> -	/* User mode accesses just cause a SIGSEGV */
> -	if (error_code & PF_USER) {
> -		/*
> -		 * It's possible to have interrupts off here.
> -		 */
> -		local_irq_enable();
> -
> -		/*
> -		 * Valid to do another page fault here because this one came
> -		 * from user space.
> -		 */
> -		if (is_prefetch(regs, address, error_code))
> -			return;
> -
> -		if (is_errata100(regs, address))
> -			return;
> -
> -		if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
> -		    printk_ratelimit()) {
> -			printk(
> -			"%s%s[%d]: segfault at %lx ip %p sp %p error %lx",
> -			task_pid_nr(tsk) > 1 ? KERN_INFO : KERN_EMERG,
> -			tsk->comm, task_pid_nr(tsk), address,
> -			(void *) regs->ip, (void *) regs->sp, error_code);
> -			print_vma_addr(" in ", regs->ip);
> -			printk("\n");
> -		}
> -
> -		tsk->thread.cr2 = address;
> -		/* Kernel addresses are always protection faults */
> -		tsk->thread.error_code = error_code | (address >= TASK_SIZE);
> -		tsk->thread.trap_no = 14;
> -		force_sig_info_fault(SIGSEGV, si_code, address, tsk);
> -		return;
> -	}
> -
> -	if (is_f00f_bug(regs, address))
> -		return;
> -
> -no_context:
> -	/* Are we prepared to handle this kernel fault?  */
> -	if (fixup_exception(regs))
> -		return;
> -
> -	/*
> -	 * X86_32
> -	 * Valid to do another page fault here, because if this fault
> -	 * had been triggered by is_prefetch fixup_exception would have
> -	 * handled it.
> -	 *
> -	 * X86_64
> -	 * Hall of shame of CPU/BIOS bugs.
> -	 */
> -	if (is_prefetch(regs, address, error_code))
> -		return;
> -
> -	if (is_errata93(regs, address))
> -		return;
> -
> -/*
> - * Oops. The kernel tried to access some bad page. We'll have to
> - * terminate things with extreme prejudice.
> - */
> -#ifdef CONFIG_X86_32
> -	bust_spinlocks(1);
> -#else
> -	flags = oops_begin();
> -#endif
> -
> -	show_fault_oops(regs, error_code, address);
> -
>   	stackend = end_of_stack(tsk);
>  	if (*stackend != STACK_END_MAGIC)
>  		printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
>  
> -	tsk->thread.cr2 = address;
> -	tsk->thread.trap_no = 14;
> -	tsk->thread.error_code = error_code;
> -
> -#ifdef CONFIG_X86_32
> -	die("Oops", regs, error_code);
> -	bust_spinlocks(0);
> -	do_exit(SIGKILL);
> -#else
>  	sig = SIGKILL;
> -	if (__die("Oops", regs, error_code))
> -		sig = 0;
> -	/* Executive summary in case the body of the oops scrolled away */
> -	printk(KERN_EMERG "CR2: %016lx\n", address);
> -	oops_end(flags, regs, sig);
> -#endif
> -
> -/*
> - * We ran out of memory, or some other thing happened to us that made
> - * us unable to handle the page fault gracefully.
> - */
> -out_of_memory:
> -	up_read(&mm->mmap_sem);
> -	if (is_global_init(tsk)) {
> -		yield();
> -		/*
> -		 * Re-lookup the vma - in theory the vma tree might
> -		 * have changed:
> -		 */
> -		goto again;
> -	}
> -
> -	printk("VM: killing process %s\n", tsk->comm);
> -	if (error_code & PF_USER)
> -		do_group_exit(SIGKILL);
> -	goto no_context;
> -
> -do_sigbus:
> -	up_read(&mm->mmap_sem);
> -
> -	/* Kernel mode? Handle exceptions or die */
> -	if (!(error_code & PF_USER))
> -		goto no_context;
> -#ifdef CONFIG_X86_32
> -	/* User space => ok to do another page fault */
> -	if (is_prefetch(regs, address, error_code))
> -		return;
> -#endif
> -	tsk->thread.cr2 = address;
> -	tsk->thread.error_code = error_code;
> -	tsk->thread.trap_no = 14;
> -	force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk);
>  }
>  
>  DEFINE_SPINLOCK(pgd_lock);

Re: [rfc] x86: optimise page fault path a little

[Nick Piggin]

On Thu, Nov 13, 2008 at 08:00:08AM -0800, Linus Torvalds wrote:
> 
> 
> On Thu, 13 Nov 2008, Nick Piggin wrote:
> > 
> > It's only about 1.1% on the profile of the workload I'm looking at, so my
> > improvement is pretty close to in the noise, but I wonder if micro
> > optimisations like the following would be welcome?
> 
> I think splitting it up is good, but I hate how your split-up ends up also 
> splitting the locking (ie now you do a "down_read()" and "up_read()" in 
> different functions).

True, but is it any better to jam them all into a 300 line function
with gotos? Hmm, this goes to there, which releases mmap_sem, and falls
through to that, then returns... oh wait, no it actually can also
`goto again`.

versus

OK, it calls some error handler, then returns. If I really cared, I
will look at how they work.

 
> I also think that to some degree you made it less readable, particularly 
> this area:
> 
> 	+       if (write) {
> 	+               if (unlikely(!(vma->vm_flags & VM_WRITE))) {
> 	+                       bad_area_accerr(regs, error_code, address);
> 	+                       return;
> 	+               }
> 	+       } else if (unlikely(error_code & PF_PROT)) {
> 	+               bad_area_accerr(regs, error_code, address);
> 	+               return;
> 	+       } else if (unlikely(!(vma->vm_flags & (VM_READ | VM_EXEC | VM_WRITE)))) {
> 	+               bad_area_accerr(regs, error_code, address);
> 	+               return;
> 
> makes me go "whaa?" and I wonder if it wouldn't be nicer to have one 
> complex conditional hidden in an inline function, and then just have
> 
> 	if (unlikely(access_error(write, error_code, vma))) {
> 		bad_area_access_error(regs, error_code, address);
> 		return;
> 	}

That would look much nicer, yes.

 
> where the point is that we don't want to duplicate the error case three 
> times, and that "accerr" is bad naming.

accerr is just the name of the error condition, but I agree your version
reads better.
 

> IOW, I do think that the patch looks like a step in the right direction, 
> but cleanliness should be a primary concern.

I guess I consider it secondary for this function, although I honestly
it is cleaner after the patch anyway... But your suggestions are
appreciated, and made it yet cleaner again I think.

Note that I haven't actually tested all the paths in the patch myself.
In particular, not the oom path or the kernel fault path and I don't
think I tested both main sigbus paths... so I don't really feel
comfortable having this patch put in a public tree until I get around
to that. But thanks everyone for the feedback so far.

Re: [rfc] x86: optimise page fault path a little

[Linus Torvalds]

On Fri, 14 Nov 2008, Nick Piggin wrote:
> 
> True, but is it any better to jam them all into a 300 line function
> with gotos?

That wasn't what I was saying.

Theere are two "good" cases:

 - don't mess with things.

   This is good. Stability is good.

 - Clearly improve things. 

   This is great.

And I'll happily do either of the above.

Your patch had some improvement, but it had some clear not-so-improved 
parts. That makes it INFERIOR to just leaving things well alone.

The thing is, I'm not very much interested in just a micro-optimization 
that seems to be all about just gcc code generation. Long-term, that's 
just bad.

But if it's a clear and undeniable _cleanup_, then long-term, it's 
actually a win. If it also happens to fix some gcc stack allocation issues 
etc, then that is just gravy.

See my point? Cleanup is good. But it had better _be_ a cleanup. Random 
micro-optimization is not so good, especially not if it them makes the 
code do things that good code simply shouldn't be doing.

As it is, I don't think your patch is appropriate to be merged. 

			Linus

Re: [rfc] x86: optimise page fault path a little

[Nick Piggin]

On Thu, Nov 13, 2008 at 06:07:45PM -0800, Linus Torvalds wrote:
> 
> 
> On Fri, 14 Nov 2008, Nick Piggin wrote:
> > 
> > True, but is it any better to jam them all into a 300 line function
> > with gotos?
> 
> That wasn't what I was saying.
> 
> Theere are two "good" cases:
> 
>  - don't mess with things.
> 
>    This is good. Stability is good.
> 
>  - Clearly improve things. 
> 
>    This is great.
> 
> And I'll happily do either of the above.
> 
> Your patch had some improvement, but it had some clear not-so-improved 
> parts. That makes it INFERIOR to just leaving things well alone.
> 
> The thing is, I'm not very much interested in just a micro-optimization 
> that seems to be all about just gcc code generation. Long-term, that's 
> just bad.
> 
> But if it's a clear and undeniable _cleanup_, then long-term, it's 
> actually a win. If it also happens to fix some gcc stack allocation issues 
> etc, then that is just gravy.
> 
> See my point? Cleanup is good. But it had better _be_ a cleanup. Random 
> micro-optimization is not so good, especially not if it them makes the 
> code do things that good code simply shouldn't be doing.

It's not a cleanup, it's an optimisation. I just didn't happen to think
it particularly makes the code *less* clean, unlike you. But I'd *never*
try to merge pointless churn like this as a cleanup because I didn't
think the old code was too bad either.

No, it's purely an optimisation. And it is using knowlege of how gcc
generates code to some extent. But adding branch annotations and
out of lining things isn't really something gcc will be able to
do on its own (it could _guess_, or use PGO, and do better register
allocation, I guess).

Anyway, it saves 300 bytes of stack footprint and seems to save a
couple of lines of icache footprint by packing the fastpaths better.

I don't exactly know what you mean by random micro-optimization. It is
what it is. I like counting cachelines and branches and I thought you
did as well.

Re: [rfc] x86: optimise page fault path a little

[Linus Torvalds]

On Fri, 14 Nov 2008, Nick Piggin wrote:
> 
> I don't exactly know what you mean by random micro-optimization. It is
> what it is. I like counting cachelines and branches and I thought you
> did as well.

You're missing the point.

If you spent a bit more time on it, it would be a cleanup too, and we 
wouldn't need this discussion.

As it is, it's a wash. At which point I'd simply rather not touch the 
code. 

You can have your cake and eat it too, and I tried to explain how. And I 
don't understand why you argue against it.

		Linus

Re: [rfc] x86: optimise page fault path a little

[Nick Piggin]

On Thu, Nov 13, 2008 at 07:18:11PM -0800, Linus Torvalds wrote:
> 
> 
> On Fri, 14 Nov 2008, Nick Piggin wrote:
> > 
> > I don't exactly know what you mean by random micro-optimization. It is
> > what it is. I like counting cachelines and branches and I thought you
> > did as well.
> 
> You're missing the point.
> 
> If you spent a bit more time on it, it would be a cleanup too, and we 
> wouldn't need this discussion.

> As it is, it's a wash. At which point I'd simply rather not touch the 
> code. 
> 
> You can have your cake and eat it too, and I tried to explain how. And I 
> don't understand why you argue against it.

Oh OK. I am going to add those changes that you suggested (in fact, I
already have). I just thought for some reason you wanted it to make
further cleanups or something.

I wasn't arguing against that. Or against refactoring the code in as
clean a way possible if there is any refactoring happening at all for
any reason (eg. performance or functionality as the primary reason).
I just wasn't going to repost the patch because as I said, my level of
testing and the relative urgency of it wouldn't justify it.

Re: [rfc] x86: optimise page fault path a little

[Linus Torvalds]

On Fri, 14 Nov 2008, Nick Piggin wrote:
> 
> Oh OK. I am going to add those changes that you suggested (in fact, I
> already have). I just thought for some reason you wanted it to make
> further cleanups or something.

No, I think it really looked like a potentially very nice cleanup already, 
with just a few nagging things that made me say "that's worse than it used 
to be".

Of course, it would be _really_ nice to see it as a series of smaller 
patches splitting things up a bit more incrementally, but I guess it's not 
a huge one even as-is.

			Linus