| Message ID | 20250201163106.28912-3-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | reduce tasklist_lock hold time on exit and do some | expand |
* Mateusz Guzik <mjguzik@gmail.com> [250201 11:31]: > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> Change log is a bit sparse? I get that the subject spells out what is done, but anything to say here at all? Reduces lock contention by reducing lock time or something? Maybe even some numbers you have in the cover letter? > --- > kernel/exit.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/exit.c b/kernel/exit.c > index 1eb2e7d36ce4..257dd8ed45ea 100644 > --- a/kernel/exit.c > +++ b/kernel/exit.c > @@ -248,9 +248,10 @@ void release_task(struct task_struct *p) > > cgroup_release(p); > > + thread_pid = get_pid(p->thread_pid); > + > write_lock_irq(&tasklist_lock); > ptrace_release_task(p); > - thread_pid = get_pid(p->thread_pid); > __exit_signal(p); > > /* > -- > 2.43.0 > >
On Mon, Feb 3, 2025 at 8:27 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > * Mateusz Guzik <mjguzik@gmail.com> [250201 11:31]: > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > > Change log is a bit sparse? I get that the subject spells out what is > done, but anything to say here at all? Reduces lock contention by > reducing lock time or something? Maybe even some numbers you have in > the cover letter? > I did not measure this bit *specifically*. As one can expect get_pid issues an atomic and that's slow. And since it can happen *prior* to taking the global lock it seems like an obvious little nit to sort out. I would argue the change is self-explanatory given the cover-letter.
* Mateusz Guzik <mjguzik@gmail.com> [250203 14:36]: > On Mon, Feb 3, 2025 at 8:27 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > * Mateusz Guzik <mjguzik@gmail.com> [250201 11:31]: > > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > > > > Change log is a bit sparse? I get that the subject spells out what is > > done, but anything to say here at all? Reduces lock contention by > > reducing lock time or something? Maybe even some numbers you have in > > the cover letter? > > > > I did not measure this bit *specifically*. > > As one can expect get_pid issues an atomic and that's slow. And since > it can happen *prior* to taking the global lock it seems like an > obvious little nit to sort out. > > I would argue the change is self-explanatory given the cover-letter. But when you git blame on the file, you will not see that cover letter. > > -- > Mateusz Guzik <mjguzik gmail.com>
On Mon, Feb 3, 2025 at 9:14 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > * Mateusz Guzik <mjguzik@gmail.com> [250203 14:36]: > > On Mon, Feb 3, 2025 at 8:27 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > > > * Mateusz Guzik <mjguzik@gmail.com> [250201 11:31]: > > > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > > > > > > Change log is a bit sparse? I get that the subject spells out what is > > > done, but anything to say here at all? Reduces lock contention by > > > reducing lock time or something? Maybe even some numbers you have in > > > the cover letter? > > > > > > > I did not measure this bit *specifically*. > > > > As one can expect get_pid issues an atomic and that's slow. And since > > it can happen *prior* to taking the global lock it seems like an > > obvious little nit to sort out. > > > > I would argue the change is self-explanatory given the cover-letter. > > But when you git blame on the file, you will not see that cover letter. if this lands, I presume it is going to go through Andrew who uses tooling pulling in the cover letter for each commit but i'm not going to argue this bit, just provide with a commit message which you think works and I'll use it
* Mateusz Guzik <mjguzik@gmail.com> [250203 15:22]: > On Mon, Feb 3, 2025 at 9:14 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > * Mateusz Guzik <mjguzik@gmail.com> [250203 14:36]: > > > On Mon, Feb 3, 2025 at 8:27 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > > > > > * Mateusz Guzik <mjguzik@gmail.com> [250201 11:31]: > > > > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > > > > > > > > Change log is a bit sparse? I get that the subject spells out what is > > > > done, but anything to say here at all? Reduces lock contention by > > > > reducing lock time or something? Maybe even some numbers you have in > > > > the cover letter? > > > > > > > > > > I did not measure this bit *specifically*. > > > > > > As one can expect get_pid issues an atomic and that's slow. And since > > > it can happen *prior* to taking the global lock it seems like an > > > obvious little nit to sort out. > > > > > > I would argue the change is self-explanatory given the cover-letter. > > > > But when you git blame on the file, you will not see that cover letter. > > if this lands, I presume it is going to go through Andrew who uses > tooling pulling in the cover letter for each commit > > but i'm not going to argue this bit, just provide with a commit > message which you think works and I'll use it Your comment above states why this is beneficial. Why not use a variation of your statement of get_pid issuing an atomic which can be removed from the locking area? This seems like an important detail to capture. > > -- > Mateusz Guzik <mjguzik gmail.com>
On Mon, 3 Feb 2025 21:22:31 +0100 Mateusz Guzik <mjguzik@gmail.com> wrote: > On Mon, Feb 3, 2025 at 9:14 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > * Mateusz Guzik <mjguzik@gmail.com> [250203 14:36]: > > > On Mon, Feb 3, 2025 at 8:27 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > > > > > > > * Mateusz Guzik <mjguzik@gmail.com> [250201 11:31]: > > > > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > > > > > > > > Change log is a bit sparse? I get that the subject spells out what is > > > > done, but anything to say here at all? Reduces lock contention by > > > > reducing lock time or something? Maybe even some numbers you have in > > > > the cover letter? > > > > > > > > > > I did not measure this bit *specifically*. > > > > > > As one can expect get_pid issues an atomic and that's slow. And since > > > it can happen *prior* to taking the global lock it seems like an > > > obvious little nit to sort out. > > > > > > I would argue the change is self-explanatory given the cover-letter. > > > > But when you git blame on the file, you will not see that cover letter. > > if this lands, I presume it is going to go through Andrew who uses > tooling pulling in the cover letter for each commit No, I copy the [0/n] description into the [1/n] changelog only. > but i'm not going to argue this bit, just provide with a commit > message which you think works and I'll use it It's rather irregular to ask Liam to explain your patch! The Subject: describes what the patch does (which was obvious from the code anyway) but the changelog fails to explain *why* the change was made. You've explained "why" adequately within this discussion so I suggest you condense those words into this patch's changelog. General muse: if a reviewer asks questions regarding a patch then we should treat those questions as bug reports against the changelogs and code comments: required information is missing. So please let's go through reviewer questions as we prepare the next revision of a patchset and make sure that all those questions are fully answered,
diff --git a/kernel/exit.c b/kernel/exit.c index 1eb2e7d36ce4..257dd8ed45ea 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -248,9 +248,10 @@ void release_task(struct task_struct *p) cgroup_release(p); + thread_pid = get_pid(p->thread_pid); + write_lock_irq(&tasklist_lock); ptrace_release_task(p); - thread_pid = get_pid(p->thread_pid); __exit_signal(p); /*
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> --- kernel/exit.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)