Hello, After document-while_each_thread-change-first_tid-to-use-for_each_thread.patch in mm tree + this series 1. We have only one lockless user of next_thread(), task_group_seq_get_next(). I think it should be changed too. 2. We have only one user of task_struct->thread_group, thread_group_empty(). The next patches will change thread_group_empty() and kill ->thread_group. Oleg.
On Thu, 24 Aug 2023 at 07:32, Oleg Nesterov <oleg@redhat.com> wrote:
>
> After document-while_each_thread-change-first_tid-to-use-for_each_thread.patch
> in mm tree + this series
Looking at your patch 2/2, I started looking at users ("Maybe we
*want* NULL for the end case, and make next_thread() and __next_thread
be the same?").
One of the main users is while_each_thread(), which certainly wants
that NULL case, both for an easier loop condition, but also because
the only user that uses the 't' pointer after the loop is
fs/proc/base.c, which wants it to be NULL.
And kernel/bpf/task_iter.c seems to *expect* NULL at the end?
End result: if you're changing next_thread() anyway, please just
change it to be a completely new thing that returns NULL at the end,
which is what everybody really seems to want, and don't add a new
__next_thread() helper. Ok?
Linus
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Thu, 24 Aug 2023 at 07:32, Oleg Nesterov <oleg@redhat.com> wrote:
>>
>> After document-while_each_thread-change-first_tid-to-use-for_each_thread.patch
>> in mm tree + this series
>
> Looking at your patch 2/2, I started looking at users ("Maybe we
> *want* NULL for the end case, and make next_thread() and __next_thread
> be the same?").
>
> One of the main users is while_each_thread(), which certainly wants
> that NULL case, both for an easier loop condition, but also because
> the only user that uses the 't' pointer after the loop is
> fs/proc/base.c, which wants it to be NULL.
Sort of.
I have found 3 loops that want to loop through all of the threads of
a process starting with the current thread.
The loop in do_wait.
The loop finding the thread to signal in complete_signal.
The loop in retarget_shared_pending finding which threads
to wake up.
For the signal case that is just quality of implementation,
and starting somewhere else would just decrease that quality.
For the loop in do_wait it is a correctness issue that the code
starts first with the threads own children before looking for
children of other threads.
There are 4 users of next_thread outside of while_each_thread.
- next_tid -- wants NULL
- task_group_seq_get_next -- same as next_tid
- __exit_signal -- wants any thread on the list after __unhash_process
- complete_signal -- wants the same loop as do_wait.
> And kernel/bpf/task_iter.c seems to *expect* NULL at the end?
>
> End result: if you're changing next_thread() anyway, please just
> change it to be a completely new thing that returns NULL at the end,
> which is what everybody really seems to want, and don't add a new
> __next_thread() helper. Ok?
So I would say Oleg please build the helper that do_wait wants
and use it in do_wait, complete_signal, and retarget_shared_pending.
Change the rest of the loops can use for_each_thread (skipping
the current task if needed) or for_each_process_thread.
Change __exit_signal to use signal->group_leader instead of next_thread.
Change next_thread to be your __next_thread, and update the 2 callers
appropriately.
Eric
On 08/25, Eric W. Biederman wrote: > > Linus Torvalds <torvalds@linux-foundation.org> writes: > > > One of the main users is while_each_thread(), which certainly wants > > that NULL case, both for an easier loop condition, but also because > > the only user that uses the 't' pointer after the loop is > > fs/proc/base.c, which wants it to be NULL. > > Sort of. > > I have found 3 loops that want to loop through all of the threads of > a process starting with the current thread. > > The loop in do_wait. > The loop finding the thread to signal in complete_signal. > The loop in retarget_shared_pending finding which threads > to wake up. Yes, plus check_unsafe_exec() and zap_other_threads() which want to skip the initial thread. > > And kernel/bpf/task_iter.c seems to *expect* NULL at the end? Yes. I'll (try to) send the patches today. This code needs cleanups first. > > End result: if you're changing next_thread() anyway, please just > > change it to be a completely new thing that returns NULL at the end, > > which is what everybody really seems to want, and don't add a new > > __next_thread() helper. Ok? > > So I would say Oleg please build the helper that do_wait wants > and use it in do_wait, complete_signal, and retarget_shared_pending. Later. But so far I am not 100% sure this makes sense... I guess we will need to discuss this again. > Change the rest of the loops can use for_each_thread (skipping > the current task if needed) or for_each_process_thread. Yes, I was going to do this. > Change next_thread to be your __next_thread, and update the 2 callers > appropriately. But I can't do this until I change the current users of next_thread() and while_each_thread(). Oleg.
On 08/24, Linus Torvalds wrote:
>
> On Thu, 24 Aug 2023 at 07:32, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > After document-while_each_thread-change-first_tid-to-use-for_each_thread.patch
> > in mm tree + this series
>
> Looking at your patch 2/2, I started looking at users ("Maybe we
> *want* NULL for the end case, and make next_thread() and __next_thread
> be the same?").
Yes, but see below.
> One of the main users is while_each_thread(), which certainly wants
> that NULL case, both for an easier loop condition,
No. Please note that, say,
do {
do_something(t);
} while_each_thread(current, t);
differs from for_each_thread() in that it loops starting from current,
not current->parent. I guess in most cases the order doesn't matter,
and I am going to audit the users and change them to use
for_each_thread() when possible.
Or,
while_each_thread(current, t)
do_something(t);
means do_something for every thread except current. And this have a
couple of valid users (say, zap_other_threads), but perhaps we can
change them too.
> but also because
> the only user that uses the 't' pointer after the loop is
> fs/proc/base.c, which wants it to be NULL.
Do you mean first_tid() ? Not only it is the only user that uses
the 't' pointer after the loop, it is the only user of lockless
while_each_thread() which (in general) is NOT rcu-safe.
But I have already changed it to use for_each_thread(), see
https://lore.kernel.org/all/20230823170806.GA11724@redhat.com/
This is
document-while_each_thread-change-first_tid-to-use-for_each_thread.patch
in mm tree.
> And kernel/bpf/task_iter.c seems to *expect* NULL at the end?
Yes! I think the same and I even documented this in 1/2.
To me this code looks simply wrong, but so far I don't understand
it enough. Currently I am trying to push the initial cleanups into
this code. See the
https://lore.kernel.org/all/20230821150909.GA2431@redhat.com/
thread.
> End result: if you're changing next_thread() anyway, please just
> change it to be a completely new thing that returns NULL at the end,
See above.
I'd prefer to audit/change the current users of while_each_thread()
and next_thread(), then (perhaps) kill while_each_thread() and/or
next_thread().
Oleg.
Argh...
sorry for noise,
On 08/24, Oleg Nesterov wrote:
>
> No. Please note that, say,
>
> do {
> do_something(t);
> } while_each_thread(current, t);
>
> differs from for_each_thread() in that it loops starting from current,
> not current->parent.
^^^^^^
just in case, of course I meant current->group_leader
Oleg.
Damn. Peter Zijlstra's email was wrong. Fix it. Peter, sorry, you can find this short series on kernel.org, https://lore.kernel.org/all/20230824143112.GA31208@redhat.com/ or I can resend with your email fixed. On 08/24, Oleg Nesterov wrote: > > Hello, > > After document-while_each_thread-change-first_tid-to-use-for_each_thread.patch > in mm tree + this series > > 1. We have only one lockless user of next_thread(), task_group_seq_get_next(). > I think it should be changed too. > > 2. We have only one user of task_struct->thread_group, thread_group_empty(). > The next patches will change thread_group_empty() and kill ->thread_group. > > Oleg.
© 2016 - 2025 Red Hat, Inc.