Re: [patch V2 3/3] signal: Allow tasks to cache one sigqueue struct
On Sat, Mar 13 2021 at 17:49, Oleg Nesterov wrote: > On 03/12, Thomas Gleixner wrote: >> >> On Fri, Mar 12 2021 at 20:26, Thomas Gleixner wrote: >> > On Fri, Mar 12 2021 at 17:11, Oleg Nesterov wrote: >> >> On 03/11, Thomas Gleixner wrote: >> >>> >> >>> @@ -456,7 +460,12 @@ static void __sigqueue_free(struct sigqu >> >>> return; >> >>> if (atomic_dec_and_test(>user->sigpending)) >> >>> free_uid(q->user); >> >>> -kmem_cache_free(sigqueue_cachep, q); >> >>> + >> >>> +/* Cache one sigqueue per task */ >> >>> +if (!current->sigqueue_cache) >> >>> +current->sigqueue_cache = q; >> >>> +else >> >>> +kmem_cache_free(sigqueue_cachep, q); >> >>> } >> >> >> >> This doesn't look right, note that __exit_signal() does >> >> flush_sigqueue(>shared_pending) at the end, after exit_task_sighand() >> >> was already called. >> >> >> >> I'd suggest to not add the new exit_task_sighand() helper and simply free >> >> current->sigqueue_cache at the end of __exit_signal(). >> > >> > Ooops. Thanks for spotting this! >> >> Hrm. >> >> The task which is released is obviously not current, so even if there >> are still sigqueues in shared_pending then they wont end up in the >> released tasks sigqueue_cache. They can only ever end up in >> current->sigqueue_cache. > > The task which is released can be "current" if this task autoreaps. > For example, if its parent ignores SIGCHLD. In this case exit_notify() > does release_task(current). Bah. Let me stare at it moar.
Re: [patch V2 3/3] signal: Allow tasks to cache one sigqueue struct
On 03/12, Thomas Gleixner wrote: > > On Fri, Mar 12 2021 at 20:26, Thomas Gleixner wrote: > > On Fri, Mar 12 2021 at 17:11, Oleg Nesterov wrote: > >> On 03/11, Thomas Gleixner wrote: > >>> > >>> @@ -456,7 +460,12 @@ static void __sigqueue_free(struct sigqu > >>> return; > >>> if (atomic_dec_and_test(>user->sigpending)) > >>> free_uid(q->user); > >>> - kmem_cache_free(sigqueue_cachep, q); > >>> + > >>> + /* Cache one sigqueue per task */ > >>> + if (!current->sigqueue_cache) > >>> + current->sigqueue_cache = q; > >>> + else > >>> + kmem_cache_free(sigqueue_cachep, q); > >>> } > >> > >> This doesn't look right, note that __exit_signal() does > >> flush_sigqueue(>shared_pending) at the end, after exit_task_sighand() > >> was already called. > >> > >> I'd suggest to not add the new exit_task_sighand() helper and simply free > >> current->sigqueue_cache at the end of __exit_signal(). > > > > Ooops. Thanks for spotting this! > > Hrm. > > The task which is released is obviously not current, so even if there > are still sigqueues in shared_pending then they wont end up in the > released tasks sigqueue_cache. They can only ever end up in > current->sigqueue_cache. The task which is released can be "current" if this task autoreaps. For example, if its parent ignores SIGCHLD. In this case exit_notify() does release_task(current). > But that brings my memory back why I had cmpxchg() in the original > version. This code runs without current->sighand->siglock held. Yes, I was wrong too. This code runs without exiting_task->sighand->siglock and this is fine in that it can not race with send_signal(exiting_task), but somehow I missed the fact that it always populates current->sigqueue_cache, not exiting_task->sigqueue_cache. Oleg.
Re: [patch V2 3/3] signal: Allow tasks to cache one sigqueue struct
On Fri, Mar 12 2021 at 20:26, Thomas Gleixner wrote: > On Fri, Mar 12 2021 at 17:11, Oleg Nesterov wrote: >> On 03/11, Thomas Gleixner wrote: >>> >>> @@ -456,7 +460,12 @@ static void __sigqueue_free(struct sigqu >>> return; >>> if (atomic_dec_and_test(>user->sigpending)) >>> free_uid(q->user); >>> - kmem_cache_free(sigqueue_cachep, q); >>> + >>> + /* Cache one sigqueue per task */ >>> + if (!current->sigqueue_cache) >>> + current->sigqueue_cache = q; >>> + else >>> + kmem_cache_free(sigqueue_cachep, q); >>> } >> >> This doesn't look right, note that __exit_signal() does >> flush_sigqueue(>shared_pending) at the end, after exit_task_sighand() >> was already called. >> >> I'd suggest to not add the new exit_task_sighand() helper and simply free >> current->sigqueue_cache at the end of __exit_signal(). > > Ooops. Thanks for spotting this! Hrm. The task which is released is obviously not current, so even if there are still sigqueues in shared_pending then they wont end up in the released tasks sigqueue_cache. They can only ever end up in current->sigqueue_cache. But that brings my memory back why I had cmpxchg() in the original version. This code runs without current->sighand->siglock held. So we need READ/WRITE_ONCE() for that on both sides which is sufficient. Thanks, tglx
Re: [patch V2 3/3] signal: Allow tasks to cache one sigqueue struct
On Fri, Mar 12 2021 at 17:11, Oleg Nesterov wrote: > On 03/11, Thomas Gleixner wrote: >> >> @@ -456,7 +460,12 @@ static void __sigqueue_free(struct sigqu >> return; >> if (atomic_dec_and_test(>user->sigpending)) >> free_uid(q->user); >> -kmem_cache_free(sigqueue_cachep, q); >> + >> +/* Cache one sigqueue per task */ >> +if (!current->sigqueue_cache) >> +current->sigqueue_cache = q; >> +else >> +kmem_cache_free(sigqueue_cachep, q); >> } > > This doesn't look right, note that __exit_signal() does > flush_sigqueue(>shared_pending) at the end, after exit_task_sighand() > was already called. > > I'd suggest to not add the new exit_task_sighand() helper and simply free > current->sigqueue_cache at the end of __exit_signal(). Ooops. Thanks for spotting this!
Re: [patch V2 3/3] signal: Allow tasks to cache one sigqueue struct
On Fri, Mar 12 2021 at 17:18, Oleg Nesterov wrote:
> On 03/12, Sebastian Andrzej Siewior wrote:
>>
>> On 2021-03-11 14:20:39 [+0100], Thomas Gleixner wrote:
>> > --- a/kernel/signal.c
>> > +++ b/kernel/signal.c
>> > @@ -433,7 +433,11 @@ static struct sigqueue *
>> >rcu_read_unlock();
>> >
>> >if (override_rlimit || likely(sigpending <= task_rlimit(t,
>> > RLIMIT_SIGPENDING))) {
>> > - q = kmem_cache_alloc(sigqueue_cachep, gfp_flags);
>> > + /* Preallocation does not hold sighand::siglock */
>> > + if (sigqueue_flags || !t->sigqueue_cache)
>> > + q = kmem_cache_alloc(sigqueue_cachep, gfp_flags);
>> > + else
>> > + q = xchg(>sigqueue_cache, NULL);
>>
>> Could it happen that two tasks saw t->sigqueue_cache != NULL, the first
>> one got the pointer via xchg() and the second got NULL via xchg()?
>
> It is called with sighand::siglock held, we don't even need xchg().
Yes, it was me being lazy. Lemme open code it as it's actually resulting
in a locked instruction.
Thanks,
tglx
Re: [patch V2 3/3] signal: Allow tasks to cache one sigqueue struct
On 03/12, Sebastian Andrzej Siewior wrote:
>
> On 2021-03-11 14:20:39 [+0100], Thomas Gleixner wrote:
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -433,7 +433,11 @@ static struct sigqueue *
> > rcu_read_unlock();
> >
> > if (override_rlimit || likely(sigpending <= task_rlimit(t,
> > RLIMIT_SIGPENDING))) {
> > - q = kmem_cache_alloc(sigqueue_cachep, gfp_flags);
> > + /* Preallocation does not hold sighand::siglock */
> > + if (sigqueue_flags || !t->sigqueue_cache)
> > + q = kmem_cache_alloc(sigqueue_cachep, gfp_flags);
> > + else
> > + q = xchg(>sigqueue_cache, NULL);
>
> Could it happen that two tasks saw t->sigqueue_cache != NULL, the first
> one got the pointer via xchg() and the second got NULL via xchg()?
It is called with sighand::siglock held, we don't even need xchg().
Oleg.
Re: [patch V2 3/3] signal: Allow tasks to cache one sigqueue struct
On 03/11, Thomas Gleixner wrote: > > @@ -456,7 +460,12 @@ static void __sigqueue_free(struct sigqu > return; > if (atomic_dec_and_test(>user->sigpending)) > free_uid(q->user); > - kmem_cache_free(sigqueue_cachep, q); > + > + /* Cache one sigqueue per task */ > + if (!current->sigqueue_cache) > + current->sigqueue_cache = q; > + else > + kmem_cache_free(sigqueue_cachep, q); > } This doesn't look right, note that __exit_signal() does flush_sigqueue(>shared_pending) at the end, after exit_task_sighand() was already called. I'd suggest to not add the new exit_task_sighand() helper and simply free current->sigqueue_cache at the end of __exit_signal(). Oleg.
Re: [patch V2 3/3] signal: Allow tasks to cache one sigqueue struct
On 2021-03-11 14:20:39 [+0100], Thomas Gleixner wrote:
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -433,7 +433,11 @@ static struct sigqueue *
> rcu_read_unlock();
>
> if (override_rlimit || likely(sigpending <= task_rlimit(t,
> RLIMIT_SIGPENDING))) {
> - q = kmem_cache_alloc(sigqueue_cachep, gfp_flags);
> + /* Preallocation does not hold sighand::siglock */
> + if (sigqueue_flags || !t->sigqueue_cache)
> + q = kmem_cache_alloc(sigqueue_cachep, gfp_flags);
> + else
> + q = xchg(>sigqueue_cache, NULL);
Could it happen that two tasks saw t->sigqueue_cache != NULL, the first
one got the pointer via xchg() and the second got NULL via xchg()?
> } else {
> print_dropped_signal(sig);
> }
> @@ -472,12 +481,19 @@ void flush_sigqueue(struct sigpending *q
> }
>
> /*
> - * Called from __exit_signal. Flush tsk->pending and clear tsk->sighand.
> + * Called from __exit_signal. Flush tsk->pending, clear tsk->sighand and
> + * free tsk->sigqueue_cache.
> */
> void exit_task_sighand(struct task_struct *tsk)
> {
> + struct sigqueue *q;
> +
> flush_sigqueue(>pending);
> tsk->sighand = NULL;
> +
> + q = xchg(>sigqueue_cache, NULL);
> + if (q)
> + kmem_cache_free(sigqueue_cachep, q);
Do we need this xchg() here? Only the task itself adds something here
and the task is on its way out so it should not add an entry to the
cache.
> }
>
> /*
Sebastian
