Received-SPF: pass (google.com: domain of linux-kernel+bounces-213601-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 13 Jun 2024 17:45:42 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, Tejun Heo <tj@kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/1] exit: kill signal_struct->quick_threads
Message-ID: <20240613154541.GD18218@redhat.com>
References: <20240609142342.GA11165@redhat.com>
 <87r0d5t2nt.fsf@email.froward.int.ebiederm.org>
 <20240610152902.GC20640@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240610152902.GC20640@redhat.com>
User-Agent: Mutt/1.5.24 (2015-08-30)

So...

Eric, do you agree with this patch or not?

Tejun, sorry for delay, I'll try to send the patch which cleanups
(at least in my opinion) the ->dying_tasks logic as soon as I have
time. But just in case... no, cgroup_exit() can't rely on group_exit
passed from the caller, I was wrong ;)


On 06/10, Oleg Nesterov wrote:
>
> Hi Eric, thanks for looking at this.
>
> Let me answer your questions out-of-order. But, before anything else,
> do you see anything wrong in 1/1 ?
>
> On 06/10, Eric W. Biederman wrote:
> >
> > May I ask which direction you are coming at this from?  Are you trying
> > to reduce the cost of do_exit?  Are you interested in untangling the
> > mess that is exiting threads in a process?
>
> I am trying to understand why do we need another counter.
>
> And, I'd like to cleanup the usage of task->signal->live, I think it
> should be avoided (if possible) when task != current. IIRC, we even
> discussed this some time ago but I can't find any reference.
>
> See also another thread about css_task_iter_advance().
>
> > > Eric, I can't understand why the commit ("signal: Guarantee that
> > > SIGNAL_GROUP_EXIT is set on process exit") added the new
> > > quick_threads counter. And why, if we forget about --quick_threads,
> > > synchronize_group_exit() has to take siglock unconditionally.
> > > Did I miss something obvious?
> >
> > At a minimum it is the exact same locking as everywhere else that sets
> > signal->flags, signal->group_exit_code, and signal->group_stop_count
> > uses.
> >
> > So it would probably require some significant reason to not use
> > the same locking and complicate reasoning about the code.  I suspect
> > setting those values without siglock held is likely to lead to
> > interesting races.
>
> I guess I was not clear. Of course, SIGNAL_GROUP_EXIT must be always
> set under ->siglock. But I think synchronize_group_exit() can just
> return if SIGNAL_GROUP_EXIT is already set? If nothing else, this is
> what do_group_exit() does.
>
> Or I misunderstood you?
>
> > That is where signal->quick_threads comes from.  In the work it is a
> > part of I wind up moving the decrement up much sooner to the point where
> > individual threads decide to exit.  The decrement of signal->live comes
> > much too late to be useful in that context.
>
> And that is why this patch moves the decrement of signal->live to the
> start of do_exit().
>
> > It is also part of me wanting to be able to uniformly use
> > SIGNAL_GROUP_EXIT and signal->group_exit_code when talking about the
> > process state, and p->exit_code when talking about the per task state.
>
> Agreed,
>
> > At the moment I am staring at wait_task_zombie and trying to understand
> > how:
> >
> > 	status = (p->signal->flags & SIGNAL_GROUP_EXIT)
> > 		? p->signal->group_exit_code : p->exit_code;
> >
> > works without any locks or barriers.
>
> Agreed, at first glance this looks worrying without siglock... I'll try
> to take a look, perhaps we can simply kill the SIGNAL_GROUP_EXIT check,
> not sure.
>
> But this patch should not make any difference ?
>
> Oleg.