MIME-Version: 1.0
References: <20221216171807.760147-1-dvyukov@google.com> <20230112112411.813356-1-dvyukov@google.com>
 <20230125124304.GA13746@redhat.com> <20230125151717.GB13746@redhat.com>
In-Reply-To: <20230125151717.GB13746@redhat.com>
From:   Dmitry Vyukov <dvyukov@google.com>
Date:   Wed, 25 Jan 2023 16:34:35 +0100
Message-ID: <CACT4Y+YKy_4mBLYomr49+fTm31Y6Q_kXhJz8O-_RTjMe=B-6eg@mail.gmail.com>
Subject: Re: [RFC PATCH v2] posix-timers: Support delivery of signals to the
 current thread
To:     Oleg Nesterov <oleg@redhat.com>
Cc:     tglx@linutronix.de, linux-kernel@vger.kernel.org,
        "Eric W . Biederman" <ebiederm@xmission.com>,
        Frederic Weisbecker <frederic@kernel.org>,
        Marco Elver <elver@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

 On Wed, 25 Jan 2023 at 16:17, Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 01/25, Oleg Nesterov wrote:
> >
> > Other than that I see nothing wrong in this patch, but I forgot everything
> > about posix timers many years ago ;)
>
> Stupid question. Am I right that in posix_timer_event()
>
>         same_thread_group(current, pid_task(timr->it_pid, PIDTYPE_PID))
>
> is always true?
>
> If yes, perhaps we can do a much simpler change which doesn't even affect API?
> See the trivial patch below.
>
> send_sigqueue(PIDTYPE_TGID) notify any thread in thread group, so this doesn't
> really change the semantics, just complete_signal() will likely choose "current"
> as a target for signal_wake_up() and iiuc this is what we want for balancing?
>
> Oleg.
>
>
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index 5dead89308b7..e38b53a0f814 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -336,6 +336,7 @@ void posixtimer_rearm(struct kernel_siginfo *info)
>  int posix_timer_event(struct k_itimer *timr, int si_private)
>  {
>         enum pid_type type;
> +       struct pid *pid;
>         int ret;
>         /*
>          * FIXME: if ->sigq is queued we can race with
> @@ -350,8 +351,9 @@ int posix_timer_event(struct k_itimer *timr, int si_private)
>          */
>         timr->sigq->info.si_sys_private = si_private;
>
> -       type = !(timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_TGID : PIDTYPE_PID;
> -       ret = send_sigqueue(timr->sigq, timr->it_pid, type);
> +       type = (timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_PID : PIDTYPE_TGID;
> +       pid = (type == PIDTYPE_PID) ? timr->it_pid : task_pid(current);
> +       ret = send_sigqueue(timr->sigq, pid, type);
>         /* If we failed to send the signal the timer stops. */
>         return ret > 0;
>  }

Hi Oleg,

This is indeed much simpler!

Do I understand correctly that:
1. I would need to use SIGEV_SIGNAL (without SIGEV_THREAD_ID)
2. The signal is still queued into process shared_pending
3. If the current task has not blocked the signal (it shouldn't), then
it won't kick any other task
4. The current task will likely deliver the signal right on the timer
interrupt return to userspace
?

This changes the existing behavior (the "average bear" may be surprised :))
https://elixir.bootlin.com/linux/v6.2-rc5/source/kernel/signal.c#L1007
But currnently it's also queued into shared_pending and any thread
could get the signal anyway. So I think this should be fine.

On the positive side: it should improve performance. Delivering to the
currently running task is better on all fronts (no kicking,
rescheduling, IPIs, better locality), right?

Let me test that it works for my use-case.