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

[Frederic Weisbecker]

On Mon, Jul 30, 2012 at 05:08:12PM +0200, Peter Zijlstra wrote:
> 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.

I really don't mind changing that to int. I just like them as bool because
they better describe the purpose of the field.

hooking means that the hooks are set (the TIF flag is set on the current task
and we also handle the exception hooks).

I can call that is_hooking instead? And/or add a comment to explain the
purpose of this.

> 
> > +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();
>   }

Ok, I'll replace.

> 
> > +       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?

Because the hooks are per cpu. If we activate the hooks on CPU 1 but not
on CPU 2 and prev was running on CPU 1 and migrates on CPU 2, it's going
to keep the hook on that CPU 2 if we don't clear the flag.
--
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/