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.

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