Re: [PATCH] mm: pagefault_{disable,enable}()

Re: [PATCH] mm: pagefault_{disable,enable}()

[Geert Uytterhoeven]

On Thu, 7 Dec 2006, Linux Kernel Mailing List wrote:
> Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=a866374aecc90c7d90619727ccd851ac096b2fc7
> Commit:     a866374aecc90c7d90619727ccd851ac096b2fc7
> Parent:     6edaf68a87d17570790fd55f0c451a29ec1d6703
> Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
> AuthorDate: Wed Dec 6 20:32:20 2006 -0800
> Committer:  Linus Torvalds <torvalds@woody.osdl.org>
> CommitDate: Thu Dec 7 08:39:21 2006 -0800
> 
>     [PATCH] mm: pagefault_{disable,enable}()
>     
>     Introduce pagefault_{disable,enable}() and use these where previously we did
>     manual preempt increments/decrements to make the pagefault handler do the
>     atomic thing.
>     
>     Currently they still rely on the increased preempt count, but do not rely on
>     the disabled preemption, this might go away in the future.
>     
>     (NOTE: the extra barrier() in pagefault_disable might fix some holes on
>            machines which have too many registers for their own good)
>     
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index a48d7f1..67918c2 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -1,8 +1,43 @@
>  #ifndef __LINUX_UACCESS_H__
>  #define __LINUX_UACCESS_H__
>  
> +#include <linux/preempt.h>
>  #include <asm/uaccess.h>
>  
> +/*
> + * These routines enable/disable the pagefault handler in that
> + * it will not take any locks and go straight to the fixup table.
> + *
> + * They have great resemblance to the preempt_disable/enable calls
> + * and in fact they are identical; this is because currently there is
> + * no other way to make the pagefault handlers do this. So we do
> + * disable preemption but we don't necessarily care about that.
> + */
> +static inline void pagefault_disable(void)
> +{
> +	inc_preempt_count();
> +	/*
> +	 * make sure to have issued the store before a pagefault
> +	 * can hit.
> +	 */
> +	barrier();
> +}
> +
> +static inline void pagefault_enable(void)
> +{
> +	/*
> +	 * make sure to issue those last loads/stores before enabling
> +	 * the pagefault handler again.
> +	 */
> +	barrier();
> +	dec_preempt_count();
> +	/*
> +	 * make sure we do..
> +	 */
> +	barrier();
> +	preempt_check_resched();
> +}
> +
>  #ifndef ARCH_HAS_NOCACHE_UACCESS
>  
>  static inline unsigned long __copy_from_user_inatomic_nocache(void *to,

This change causes lots of compile errors of the following form on m68k:

| linux-2.6.20-rc4/include/linux/uaccess.h: In function `pagefault_disable':
| linux-2.6.20-rc4/include/linux/uaccess.h:18: error: dereferencing pointer to incomplete type
| linux-2.6.20-rc4/include/linux/uaccess.h: In function `pagefault_enable':
| linux-2.6.20-rc4/include/linux/uaccess.h:33: error: dereferencing pointer to incomplete type

    -> inc_preempt_count
    -> add_preempt_count
    -> preempt_count
    -> current_thread_info
    -> task_thread_info
which needs the definition of struct task_struct.

The patch below fixes it by including <linux/sched.h> in
include/linux/uaccess.h. But perhaps this is the right time to move
struct task_struct to its own include instead?

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 67918c2..ae73f32 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -2,6 +2,7 @@
 #define __LINUX_UACCESS_H__
 
 #include <linux/preempt.h>
+#include <linux/sched.h>
 #include <asm/uaccess.h>
 
 /*

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] mm: pagefault_{disable,enable}()

[Linus Torvalds]

On Wed, 10 Jan 2007, Geert Uytterhoeven wrote:
> 
> This change causes lots of compile errors of the following form on m68k:
> 
> | linux-2.6.20-rc4/include/linux/uaccess.h: In function `pagefault_disable':
> | linux-2.6.20-rc4/include/linux/uaccess.h:18: error: dereferencing pointer to incomplete type
> | linux-2.6.20-rc4/include/linux/uaccess.h: In function `pagefault_enable':
> | linux-2.6.20-rc4/include/linux/uaccess.h:33: error: dereferencing pointer to incomplete type

Ouch. However, I think your patch is bogus.

You're fixing somethign that doesn't need fixing: <linux/uaccess.h> 
already includes <linux/preempt.h> for the preemption functions.

The REAL problem seems to be that the m68k preempt.h (or rather, to be 
exact, asm/thread_info.h) doesn't do things right, and while it exposes 
"inc_preempt_count()", it doesn't expose enough information to actually 
use it.

I think your "current_thread_info()" is broken.

		Linus

Re: [PATCH] mm: pagefault_{disable,enable}()

[Geert Uytterhoeven]

On Wed, 10 Jan 2007, Linus Torvalds wrote:
> On Wed, 10 Jan 2007, Geert Uytterhoeven wrote:
> > This change causes lots of compile errors of the following form on m68k:
> > 
> > | linux-2.6.20-rc4/include/linux/uaccess.h: In function `pagefault_disable':
> > | linux-2.6.20-rc4/include/linux/uaccess.h:18: error: dereferencing pointer to incomplete type
> > | linux-2.6.20-rc4/include/linux/uaccess.h: In function `pagefault_enable':
> > | linux-2.6.20-rc4/include/linux/uaccess.h:33: error: dereferencing pointer to incomplete type
> 
> Ouch. However, I think your patch is bogus.
> 
> You're fixing somethign that doesn't need fixing: <linux/uaccess.h> 
> already includes <linux/preempt.h> for the preemption functions.

Indeed.

> The REAL problem seems to be that the m68k preempt.h (or rather, to be 
> exact, asm/thread_info.h) doesn't do things right, and while it exposes 
> "inc_preempt_count()", it doesn't expose enough information to actually 
> use it.
> 
> I think your "current_thread_info()" is broken.

But struct task_struct is defined in <linux/sched.h>, which cannot be included
in <asm/thread_info.h> due to include recursion hell.

The alternative is to move struct task_struct to its own file, which may be a
bit too intrusive for 2.6.20.

So I'm afraid my patch is the least intrusive solution. Or am I missing
something?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] mm: pagefault_{disable,enable}()

[Linus Torvalds]

On Wed, 10 Jan 2007, Geert Uytterhoeven wrote:
>
> > The REAL problem seems to be that the m68k preempt.h (or rather, to be 
> > exact, asm/thread_info.h) doesn't do things right, and while it exposes 
> > "inc_preempt_count()", it doesn't expose enough information to actually 
> > use it.
> > 
> > I think your "current_thread_info()" is broken.
> 
> But struct task_struct is defined in <linux/sched.h>, which cannot be included
> in <asm/thread_info.h> due to include recursion hell.

But why do you need "struct task_struct" at all?

The reason this doesn't happen on other platforms is that they don't use 
"struct task_struct". They use "struct thread_info", which is where the 
preemption counter is.

The problem on m68k i sthat broken indirection through "current", which is 
unnecessary. Isn't the thread structure on the stack on m68k too? So you 
could do what x86 does, and just do

	movel %a7,%d0
	andl $STACK_MASK,%d0

or something, and thus go directly to the thread-info rather than load it 
off the task pointer.

Or, if worst comes to worst, you can just hardcode the offset of the 
thread-info pointer in the "struct task_struct". It's the second word 
after "state". Ugly, but less so than forcing everybody who does NOT want 
to have that big <linux/sched.h> dependency to get it.

		Linus

Re: [PATCH] mm: pagefault_{disable,enable}()

[Roman Zippel]

Hi,

On Wed, 10 Jan 2007, Linus Torvalds wrote:

> > > I think your "current_thread_info()" is broken.

No, it's not. What is broken are our header dependencies - we happily mix 
declarations with implementations.
Let's take a simple example like <linux/list.h>, which is practically 
included everywhere, but even that already includes 57 other files and 
depends on 49 config symbols (by comparison for <linux/types.h> it's 7 
other files and 7 symbols).

> > But struct task_struct is defined in <linux/sched.h>, which cannot be included
> > in <asm/thread_info.h> due to include recursion hell.
> 
> But why do you need "struct task_struct" at all?
> 
> The reason this doesn't happen on other platforms is that they don't use 
> "struct task_struct". They use "struct thread_info", which is where the 
> preemption counter is.
> 
> The problem on m68k i sthat broken indirection through "current", which is 
> unnecessary. Isn't the thread structure on the stack on m68k too? So you 
> could do what x86 does, and just do
> 
> 	movel %a7,%d0
> 	andl $STACK_MASK,%d0

That's exactly want I want to avoid, thread info and task struct can be 
accessed via a single register (pointer).

> Or, if worst comes to worst, you can just hardcode the offset of the 
> thread-info pointer in the "struct task_struct". It's the second word 
> after "state". Ugly, but less so than forcing everybody who does NOT want 
> to have that big <linux/sched.h> dependency to get it.

That still generates worse code, the best solution would be to leave the 
work to gcc, but that requires it can actually recognize that task struct 
and thread info need only a single pointer.
One possibility would be to do something like ia64 and abuse 
asm-offsets.h, but then one gets other funny build problems.

By far the cleanest solution would be to clean up our damned header 
dependencies, separating task_struct from sched.h is not that difficult 
and would give archs far more flexibility how to place task struct and 
thread info.

bye, Roman