[PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks - Kernel

This is a discussion on [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks - Kernel ; The signal has no effect (but can provoke the unnecessary wakeup) if the thread group is dying. Let's make this explicit and check SIGNAL_GROUP_EXIT only once in handle_stop_signal() renamed to prepare_signal(). From now the actual signal-delivery path doesn't need to ...

+ Reply to Thread
Results 1 to 7 of 7

Thread: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

  1. [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

    The signal has no effect (but can provoke the unnecessary wakeup) if the
    thread group is dying. Let's make this explicit and check SIGNAL_GROUP_EXIT
    only once in handle_stop_signal() renamed to prepare_signal().

    From now the actual signal-delivery path doesn't need to take the special
    SIGNAL_GROUP_EXIT case into account.

    Signed-off-by: Oleg Nesterov

    --- 25/kernel/signal.c~8_SS_CK_SGE 2008-03-09 20:21:02.000000000 +0300
    +++ 25/kernel/signal.c 2008-03-09 21:18:19.000000000 +0300
    @@ -564,16 +564,16 @@ static void do_notify_parent_cldstop(str
    * actual continuing for SIGCONT, but not the actual stopping for stop
    * signals. The process stop is done as a signal action for SIG_DFL.
    */
    -static void handle_stop_signal(int sig, struct task_struct *p)
    +static int prepare_signal(int sig, struct task_struct *p)
    {
    struct signal_struct *signal = p->signal;
    struct task_struct *t;

    - if (signal->flags & SIGNAL_GROUP_EXIT)
    + if (unlikely(signal->flags & SIGNAL_GROUP_EXIT))
    /*
    * The process is in the middle of dying already.
    */
    - return;
    + return 0;

    if (sig_kernel_stop(sig)) {
    /*
    @@ -644,6 +644,8 @@ static void handle_stop_signal(int sig,
    signal->flags &= ~SIGNAL_STOP_DEQUEUED;
    }
    }
    +
    + return 1;
    }

    /*
    @@ -708,8 +710,7 @@ static void complete_signal(int sig, str
    * Found a killable thread. If the signal will be fatal,
    * then start taking the whole group down immediately.
    */
    - if (sig_fatal(p, sig) && !(signal->flags & SIGNAL_GROUP_EXIT) &&
    - !sigismember(&t->real_blocked, sig) &&
    + if (sig_fatal(p, sig) && !sigismember(&t->real_blocked, sig) &&
    (sig == SIGKILL || !(t->ptrace & PT_PTRACED))) {
    /*
    * This signal will be fatal to the whole group.
    @@ -753,7 +754,8 @@ static int send_signal(int sig, struct s
    struct sigqueue *q;

    assert_spin_locked(&t->sighand->siglock);
    - handle_stop_signal(sig, t);
    + if (!likely(prepare_signal(sig, t)))
    + return 0;

    pending = group ? &t->signal->shared_pending : &t->pending;
    /*
    @@ -1247,9 +1249,10 @@ int send_sigqueue(struct sigqueue *q, st
    if (!likely(lock_task_sighand(t, &flags)))
    goto ret;

    - handle_stop_signal(sig, t);
    -
    ret = 1;
    + if (!likely(prepare_signal(sig, t)))
    + goto out;
    +
    if (sig_ignored(t, sig))
    goto out;


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

    I think your logic is sound. But for paranoia's sake I would add
    a BUG_ON(signal->flags & SIGNAL_GROUP_EXIT) in the group-fatal case.

    I don't really like dropping the signal on the floor. If the posting says
    it succeeded (vs -ESRCH) then I'd like to see it appear later in the
    pending set of the zombie thread, when we look at /proc/pid/status at exit
    tracing or whatnot (and in core dumps). Seeing those pending signal bits
    is sometimes a useful clue about how something died in a strange scenario.


    Thanks,
    Roland
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

    On 03/10, Roland McGrath wrote:
    >
    > I don't really like dropping the signal on the floor.


    To clarify: my opinion is quite opposite, but it is only based on the
    "personal feeling", I don't have any "strong" arguments. If you still
    think we shouldn't do this, please nack this change.

    > If the posting says
    > it succeeded (vs -ESRCH) then I'd like to see it appear later in the
    > pending set of the zombie thread, when we look at /proc/pid/status at exit
    > tracing or whatnot (and in core dumps). Seeing those pending signal bits
    > is sometimes a useful clue about how something died in a strange scenario.


    As for core dumps. Suppose that the task has already started do_coredump()
    and another signal comes. Why should fill_prstatus() report this new signal?
    This doesn't look exactly right for me. The coredump should try to reflect
    the state of the task which it had when it was killed.

    Actually, the same for zombies. If the task sits in Z state, one can look
    at /proc/pid/status to see what signals it had when exited. To me, it looks
    just better if it is visible that a zombie doesn "accept" the new signals,
    because it is dead. (offtopic: currently the single-threaded exit doesn't
    set SIGNAL_GROUP_EXIT. This doesn't matter now, but I think it would be
    nice to be more consistent here).

    Now, suppose that due to some kernel bug the running thread/process doesn't
    want to die despite of SIGNAL_GROUP_EXIT. In that case another kill() may help
    because of additional wakeup, but this will hide the problem which won't be
    reported.

    But again, I see your point, thanks.

    Oleg.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

    There are arguments to be had about deciding that change to the behavior.
    We can discuss it more if you like. But that is rather different from
    quietly rolling in your choices of behavior change to a "cleanup" patch.


    Thanks,
    Roland
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

    Andrew, please drop these patches:

    signals-send_signal-factor-out-signal_group_exit-checks.patch
    signals-fold-sig_ignored-into-prepare_signal.patch
    signals-document-cld_continued-notification-mechanics.patch

    (the last one refers to prepare_signal() introduced in the first one,
    and the comment doesn't match the canonical style)

    On 03/11, Roland McGrath wrote:
    >
    > There are arguments to be had about deciding that change to the behavior.
    > We can discuss it more if you like.


    Of course, since I personally don't agree, I'd like to discuss it more
    if possible.

    > But that is rather different from
    > quietly rolling in your choices of behavior change to a "cleanup" patch.


    "[PATCH 3/6] signals: use __group_complete_signal() for the specific signals too"
    adds a behaviour change too.

    Surely, I don't want to change the behaviour quietly, that is why I am
    cc'ing maintainers.

    (note also these patches are 8/6, 9/6. Originally I was going to send
    them in a separate series).

    Oleg.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

    > Of course, since I personally don't agree, I'd like to discuss it more
    > if possible.


    Certainly, but let's do it under separate cover, and after some of
    these cleanups settle. (I'd rather not try to get into it this week.)

    > "[PATCH 3/6] signals: use __group_complete_signal() for the specific signals too"
    > adds a behaviour change too.


    Your log entry was explicit about the semantics change there. You
    explained it up front and justified it. I also happened to agree
    with it, but that's a separate issue.

    I am certainly not opposed to semantics changes a priori.
    I happen to have reservations about this particular one.

    > Surely, I don't want to change the behaviour quietly, that is why I am
    > cc'ing maintainers.


    The point is that, whenever possible, a semantics change should be
    isolated into a patch separate from any related cleanups. More
    important than that, no semantics change should go unmentioned so
    it's only documented as a result of someone's careful review of
    the change. (Of course when a change is inadvertent, then only
    review is going to notice it--that's what review is for.)

    Your new pair of patches dated 2008-3-12 look like they are doing
    exactly this (just the cleanup first). After those are in, the
    semantics change you want is a one-liner and easy to review and
    discuss on its own.

    > (note also these patches are 8/6, 9/6. Originally I was going to send
    > them in a separate series).


    I had noticed that your wholes sometimes go up to 1.5; I just
    figured it's because you're 50% more thorough than the rest of us.
    ;-)


    Thanks,
    Roland
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [PATCH 8/6] signals: send_signal: factor out SIGNAL_GROUP_EXIT checks

    On 03/12, Roland McGrath wrote:
    >
    > > Of course, since I personally don't agree, I'd like to discuss it more
    > > if possible.

    >
    > The point is that, whenever possible, a semantics change should be
    > isolated into a patch separate from any related cleanups. More
    > important than that, no semantics change should go unmentioned so
    > it's only documented as a result of someone's careful review of
    > the change.


    Yes. You are very right.

    And. I must admit. I didn't even realize that the added semantics change
    worth at least a special note in changelog because it is easily visible.
    This was really wrong in any case. Thanks for pointing out this.

    Oleg.

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread