Re: [PATCH -mm 3/3] proc: make task_sig() lockless

[Oleg Nesterov]

On 03/24, David Howells wrote:
>
> Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> >
> > Yes. From the changelog:
> >
> > 	Of course, this means we read pending/blocked/etc nonatomically,
> > 	but I hope this is OK for fs/proc.
>
> Ah, yes.  I read that as you meant how procfs accessed the actual data
> structures, not how the user accessed procfs.  It might be worth clarifying
> that.

OK, agreed.

> Acked-by: David Howells <dhowells@xxxxxxxxxx>

Thanks,

> > > Probably we can change do_task_stat() to avod ->siglock too, except
>
> Btw, avoid has an 'i' in it... :-)

Another reason to update the changelog ;)


Andrew, please find the updated changelog for proc-make-task_sig-lockless.patch
If this is not convenient, please ignore or tell me what is the "right" way
to fix the changelog when the patch is already in -mm.

------------------------------------------------------------------------------
Now that task->signal can't go away and collect_sigign_sigcatch() is
rcu-safe, task_sig() doesn't need ->siglock.

Remove lock_task_sighand() and unnecessary sigemptyset's, move
collect_sigign_sigcatch() under rcu_read_lock().

Of course, this means we read pending/blocked/etc nonatomically and we
can report this info in some intermediate state. Say, a signal can be
reported as both pending and ignored, or we can report ->sigpending != 0
while pending/shpending are empty, etc. Hopefully this is OK for proc,
we never promised this info should be atomic.

Probably we can change do_task_stat() to avoid ->siglock too, except we
can't get tty_nr lockless.

Also, remove the "is this correct?" comment.  I think it is safe to
dereference __task_cred(p)->user under rcu lock.  In any case, ->siglock
can't help to protect cred->user.

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