Re: [PATCH 1/5] user_hooks: New user hooks subsystem

[Peter Zijlstra <peterz@infradead.org>]

On Fri, 2012-07-27 at 17:40 +0200, Frederic Weisbecker wrote:
> +++ b/kernel/user_hooks.c
> @@ -0,0 +1,56 @@
> +#include <linux/user_hooks.h>
> +#include <linux/rcupdate.h>
> +#include <linux/sched.h>
> +#include <linux/percpu.h>
> +
> +struct user_hooks {
> +       bool hooking;
> +       bool in_user;
> +};

I really detest using bool in structures.. but that's just me. Also this
really wants a comment as to wtf 'hooking' means. in_user I can just
about guess.

> +DEFINE_PER_CPU(struct user_hooks, user_hooks) = {
> +#ifdef CONFIG_USER_HOOKS_FORCE
> +       .hooking = true,
> +#endif
> +};
> +
> +void user_enter(void)
> +{
> +       unsigned long flags;
> +       struct user_hooks *uh;
> +
> +       WARN_ON_ONCE(!current->mm);
> +       local_irq_save(flags);
> +       uh = &__get_cpu_var(user_hooks);
> +       if (uh->hooking && !uh->in_user) {
> +               uh->in_user = true;
> +               rcu_user_enter();
> +       }

By not using __get_cpu_var() but __this_cpu_*() you generate much better
code (esp. on x86).

IOW. something like:

  if (__this_cpu_read(uh.hooking) && !__this_cpu_read(uh.in_user)) {
	__this_cpu_write(uh.in_user, true);
	rcu_user_enter();
  }

> +       local_irq_restore(flags);
> +}
> +
> +void user_exit(void)
> +{
> +       unsigned long flags;
> +       struct user_hooks *uh;
> +
> +       local_irq_save(flags);
> +       uh = &__get_cpu_var(user_hooks);
> +       if (uh->in_user) {
> +               uh->in_user = false;
> +               rcu_user_exit();
> +       }
> +       local_irq_restore(flags);
> +}
> +
> +void user_hooks_switch(struct task_struct *prev,
> +                      struct task_struct *next)
> +{
> +       struct user_hooks *uh;
> +
> +       uh = &__get_cpu_var(user_hooks);
> +       if (uh->hooking) {
> +               clear_tsk_thread_flag(prev, TIF_NOHZ);
> +               set_tsk_thread_flag(next, TIF_NOHZ);
> +       }

This seems pointless to me.. why are we flipping that flag on context
switch instead of keeping it enabled at all times? This are two atomic
ops in the context switch path, why?

> +}