Messages in this thread Patch in this message | | | Date | Wed, 25 Jan 2023 17:31:37 +0100 | From | Oleg Nesterov <> | Subject | Re: [RFC PATCH v2] posix-timers: Support delivery of signals to the current thread |
| |
On 01/25, Dmitry Vyukov wrote: > > > 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)
Yes,
> 2. The signal is still queued into process shared_pending
Yes. But just in case, please note that if this signal is not realtime (sigev_signo < SIGRTMIN) and it is already queued, it will be dropped. And I do not know if this can work for you.
However this is what we already have with SIGEV_SIGNAL w/o SIGEV_THREAD_ID, and the same is true for SIGEV_THREAD_ID if the signal is already pending in target_task->pending.
> 3. If the current task has not blocked the signal (it shouldn't), then > it won't kick any other task
Yes,
> 4. The current task will likely deliver the signal right on the timer > interrupt return to userspace > ?
Yes.
But! I just noticed send_sigqueue() does pid_task(pid, type), so the patch above needs another change
--- a/kernel/signal.c +++ b/kernel/signal.c @@ -1970,7 +1970,8 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type) ret = -1; rcu_read_lock(); - t = pid_task(pid, type); + // comment to explain why don't we use "type" + t = pid_task(pid, PIDTYPE_PID); if (!t || !likely(lock_task_sighand(t, &flags))) goto ret;
> This changes the existing behavior (the "average bear" may be surprised :)) > https://elixir.bootlin.com/linux/v6.2-rc5/source/kernel/signal.c#L1007
this comment looks a bit misleading, s/main thread/target thread/
> But currnently it's also queued into shared_pending and any thread > could get the signal anyway. So I think this should be fine.
Yes.
> 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?
Well, iiuc this was the goal of your patch ? ;) Oleg.
| |