Messages in this thread | | | Date | Fri, 12 Mar 2021 12:35:40 +0100 | From | Sebastian Andrzej Siewior <> | Subject | 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(&t->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(&tsk->pending); > tsk->sighand = NULL; > + > + q = xchg(&tsk->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
| |