Re: [RFC][PATCH 0/5] Signal scalability series

[Oleg Nesterov <oleg@redhat.com>]

On 09/30, Matt Fleming wrote:
>
> However, it's a good first step and
> hopefully by keeping it relatively simple it'll make it easier to
> review.

Cough. I'll try to read this series next week, but currently I feel
I will never able to understand this code. It surely compliacates
things a lot.

But. All I can do is to _try_ to check this series from the correctness
pov. I can't believe (at least at first glance) this worth the trouble,
but otoh I won't argue unless I'll find the bugs.

>  arch/ia64/kernel/signal.c           |    4 +-
>  drivers/block/nbd.c                 |    2 +-
>  drivers/usb/gadget/f_mass_storage.c |    2 +-
>  drivers/usb/gadget/file_storage.c   |    2 +-
>  fs/autofs4/waitq.c                  |    5 +-
>  fs/exec.c                           |   17 +-
>  fs/jffs2/background.c               |    2 +-
>  fs/ncpfs/sock.c                     |    2 +
>  fs/proc/array.c                     |    2 +
>  fs/signalfd.c                       |   11 +-
>  include/linux/init_task.h           |    4 +
>  include/linux/sched.h               |   23 +-
>  kernel/exit.c                       |   29 +-
>  kernel/fork.c                       |    4 +
>  kernel/freezer.c                    |   10 +-
>  kernel/kmod.c                       |    8 +-
>  kernel/posix-timers.c               |    5 +-
>  kernel/ptrace.c                     |   68 ++--
>  kernel/signal.c                     |  737 +++++++++++++++++++++++++++--------
>  net/9p/client.c                     |    6 +-
>  net/sunrpc/svc.c                    |    3 -
>  security/selinux/hooks.c            |   11 +-
>  22 files changed, 677 insertions(+), 280 deletions(-)

And, this patch adds 4 new locks:

	sighand_struct->action_lock

	signal_struct->ctrl_lock
	signal_struct->shared_siglock

	task_struct->siglock

Nice ;) For what? This should be justified, imho.


Hmm. Just out of curiosity, I blindly applied the whole series and poke
the _random_ function to look at, dequeue_signal(). And it looks wrong.

	spin_lock_irqsave(&current->signal->ctrl_lock, flags);
	current->jobctl |= JOBCTL_STOP_DEQUEUED;

This signal->ctrl_lock can't help. A sig_kernel_stop() should be
dequeued under the same lock, and we shouldn't release it unless we
set JOBCTL_STOP_DEQUEUED. Otherwise we race with SIGCONT.

May be do_signal_stop() does something special? At first flance it doesn't.
But wait, it does while_each_thread() under ->ctrl_lock, why this is safe?

May be I was just lucky ;)

Oleg.