Re: [PATCH] signal: Simplify __lock_task_sighand()

[Oleg Nesterov]

On 05/05, Oleg Nesterov wrote:
>
> Yes, but please consider the cleanup below, on top of your change.
>
> This is subjective of course, but imho the code looks better without
> the extra unlock/restore inside the loop.
>
> -------------------------------------------------------------------------------
> Subject: [PATCH] signal: Simplify __lock_task_sighand()
> 
> __lock_task_sighand() does local_irq_save() to prevent the potential
> deadlock, we can use preempt_disable() with the same effect. And in
> this case we can do preempt_disable/enable + rcu_read_lock/unlock only
> once outside of the main loop and simplify the code. Also shaves 112
> bytes from signal.o.
>
> Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> ---
>  kernel/signal.c |   31 +++++++++++++------------------
>  1 files changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 4368370..03a0fd4 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1260,30 +1260,25 @@ struct sighand_struct *__lock_task_sighand(struct task_struct *tsk,
>  					   unsigned long *flags)
>  {
>  	struct sighand_struct *sighand;
> -
> +	/*
> +	 * We are going to do rcu_read_unlock() under spin_lock_irqsave().
> +	 * Make sure we can not be preempted after rcu_read_unlock(), see
                                                   ^^^^^^^^^^^^^^^
Argh, typo in comment. I meant rcu_read_lock() of course.

I'll send v2 tomorrow unless you dislike this change.

Oleg.

--
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/