On Tue, Feb 4, 2025 at 12:23 PM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> On 02/03, Mateusz Guzik wrote:
> >
> > On Mon, Feb 3, 2025 at 7:07 PM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> > >
> > > On 02/01, Mateusz Guzik wrote:
> > > >
> > > > Instead of smuggling the tty pointer directly, use a struct so that more
> > > > things can be added later.
> > >
> > > I am not sure this particular change worth the effort, but I won't argue.
> > > I'd like to know what Eric thinks.
> > >
> >
> > it trivially whacks an atomic from an area protected by a global lock
>
> Yes, but I am not sure this can make a noticeable difference... Let
> me repeat, I am not really arguing, I leave this to you and Eric.
> The patch looks correct and I have already acked it. Just it looks
> "less obvious" to me than other changes in this series.
Reducing lock hold time is basic multicore hygiene, doubly so for global locks.
Atomics are notoriously slow even if cache-hot.
I agree this change in isolation is not a big deal, but there are
several other "not a big deal"s in the area which will add up if
sorted out.
Given the triviality of the change as far as complexity goes, I would
argue that's a routine patch not warranting much of a discussion
(modulo the new struct, for that see below).
>
> And even if we do this, I am not sure we need the new
> "struct release_task_post", it seems we can simply move
> tty_kref_put() from __exit_signal() to release_task(), see the
> patch at the end.
>
> Nobody can touch signal->tty after the group leader passes __exit_signal(),
> if nothing else lock_task_sighand() can't succeed.
>
I wanted to keep "whack the tty" logic intact in order reduce any discussion. ;)
This brings me to the release_task_post struct.
Suppose the tty thing does not get patched at all or gets patched the
way you are proposing here.
I still need a way to handle the pidmap stuff. Suppose to that end a
pid array gets passed directly.
Some time later another thing might pop up which gets determined
within the lock and wants to return the state to clean up after the
unlock. Then someone will need to add a mechanism of the same sort I'm
adding here. iow I'm future-proofing this and tty just happens to be
the first (even if not necessary) consumer. iow preferably the struct
(or an equivalent) would still be there without tty stuff.
Given the nature of the change there is a lot of opinionated
handwaving possible and "happy to change" vs "feel free to ignore" is
not a productive discussion, thus I would like to clarify: my primary
interest is to whack the pidmap thing out of tasklist_lock-protected
area and this bit I'm going to argue about. Everything else is minor
touch ups which I can drop without much resistance (I'll note though
there is more I can submit in the area later :P), but if you genuinely
want something changed, it would be best explicitly say it.
As is, given the prior ack, I intend to submit v4 with minor touch ups.
> But again, feel free to ignore.
>
> Oleg.
>
> --- x/kernel/exit.c
> +++ x/kernel/exit.c
> @@ -146,7 +146,6 @@ static void __exit_signal(struct task_st
> struct signal_struct *sig = tsk->signal;
> bool group_dead = thread_group_leader(tsk);
> struct sighand_struct *sighand;
> - struct tty_struct *tty;
> u64 utime, stime;
>
> sighand = rcu_dereference_check(tsk->sighand,
> @@ -159,10 +158,7 @@ static void __exit_signal(struct task_st
> posix_cpu_timers_exit_group(tsk);
> #endif
>
> - if (group_dead) {
> - tty = sig->tty;
> - sig->tty = NULL;
> - } else {
> + if (!group_dead) {
> /*
> * If there is any task waiting for the group exit
> * then notify it:
> @@ -210,10 +206,8 @@ static void __exit_signal(struct task_st
>
> __cleanup_sighand(sighand);
> clear_tsk_thread_flag(tsk, TIF_SIGPENDING);
> - if (group_dead) {
> + if (group_dead)
> flush_sigqueue(&sig->shared_pending);
> - tty_kref_put(tty);
> - }
> }
>
> static void delayed_put_task_struct(struct rcu_head *rhp)
> @@ -279,6 +273,10 @@ repeat:
> proc_flush_pid(thread_pid);
> put_pid(thread_pid);
> release_thread(p);
> +
> + if (p == leader)
> + tty_kref_put(p->signal->tty);
> +
> put_task_struct_rcu_user(p);
>
> p = leader;
>
--
Mateusz Guzik <mjguzik gmail.com>
