diff mbox series

[v4,3/5] pid: sprinkle tasklist_lock asserts

Message ID 20250205193221.402150-4-mjguzik@gmail.com (mailing list archive)
State New
Headers show
Series reduce tasklist_lock hold time on exit and do some pid cleanup | expand

Commit Message

Mateusz Guzik Feb. 5, 2025, 7:32 p.m. UTC 
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 kernel/pid.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Liam R. Howlett Feb. 5, 2025, 8:26 p.m. UTC | #1 
* Mateusz Guzik <mjguzik@gmail.com> [250205 14:33]:

If a patch is worth doing, it's worth explaining.  I'm surprised you
didn't add a comment after the last revision missed a comment on patch
2/6.  This is literally in the submitting patches document [1].

I don't mean to delay this series, but I do want to know why things are
done when I'm hunting through git logs.  Having a change log isn't
optional, and now you know that Andrews script won't fix this problem
[2].

I see you are upset by this considering the terse and lack of
punctuation in patch 2, but please try to understand these comments
serve a purpose in maintaining the code years later.

[1]. https://www.kernel.org/doc/html/v6.12/process/submitting-patches.html#describe-your-changes
[2]. https://lore.kernel.org/all/20250203175128.80319b42c9739f0d420080a4@linux-foundation.org/

> Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---
>  kernel/pid.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 924084713be8..2ae872f689a7 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -339,17 +339,23 @@ static struct pid **task_pid_ptr(struct task_struct *task, enum pid_type type)
>   */
>  void attach_pid(struct task_struct *task, enum pid_type type)
>  {
> -	struct pid *pid = *task_pid_ptr(task, type);
> +	struct pid *pid;
> +
> +	lockdep_assert_held_write(&tasklist_lock);
> +
> +	pid = *task_pid_ptr(task, type);
>  	hlist_add_head_rcu(&task->pid_links[type], &pid->tasks[type]);
>  }
>  
>  static void __change_pid(struct task_struct *task, enum pid_type type,
>  			struct pid *new)
>  {
> -	struct pid **pid_ptr = task_pid_ptr(task, type);
> -	struct pid *pid;
> +	struct pid **pid_ptr, *pid;
>  	int tmp;
>  
> +	lockdep_assert_held_write(&tasklist_lock);
> +
> +	pid_ptr = task_pid_ptr(task, type);
>  	pid = *pid_ptr;
>  
>  	hlist_del_rcu(&task->pid_links[type]);
> @@ -386,6 +392,8 @@ void exchange_tids(struct task_struct *left, struct task_struct *right)
>  	struct hlist_head *head1 = &pid1->tasks[PIDTYPE_PID];
>  	struct hlist_head *head2 = &pid2->tasks[PIDTYPE_PID];
>  
> +	lockdep_assert_held_write(&tasklist_lock);
> +
>  	/* Swap the single entry tid lists */
>  	hlists_swap_heads_rcu(head1, head2);
>  
> @@ -403,6 +411,7 @@ void transfer_pid(struct task_struct *old, struct task_struct *new,
>  			   enum pid_type type)
>  {
>  	WARN_ON_ONCE(type == PIDTYPE_PID);
> +	lockdep_assert_held_write(&tasklist_lock);
>  	hlist_replace_rcu(&old->pid_links[type], &new->pid_links[type]);
>  }
>  
> -- 
> 2.43.0
> 
>
Mateusz Guzik Feb. 5, 2025, 8:34 p.m. UTC | #2 
On Wed, Feb 5, 2025 at 9:27 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Mateusz Guzik <mjguzik@gmail.com> [250205 14:33]:
>
> If a patch is worth doing, it's worth explaining.  I'm surprised you
> didn't add a comment after the last revision missed a comment on patch
> 2/6.  This is literally in the submitting patches document [1].
>
> I don't mean to delay this series, but I do want to know why things are
> done when I'm hunting through git logs.  Having a change log isn't
> optional, and now you know that Andrews script won't fix this problem
> [2].
>
> I see you are upset by this considering the terse and lack of
> punctuation in patch 2, but please try to understand these comments
> serve a purpose in maintaining the code years later.
>

I'm not upset.

For this specific case I don't know what can be written in the body
given the really self-explanatory nature of the change, other than to
spell it out(?).

Does this work for you:
The routines need to be called with the tasklist_lock, the asserts
validate at runtime that this holds.

I also git log a lot and like to know what's up, to that end I
appreciate *short* commit messages so that I know there is nothing
more to the patch than meets the eye. In particular if there is
nothing of value to add in the body, I appreciate if there is none.

But that's me, I'm not going to insist one way or the other.

> [1]. https://www.kernel.org/doc/html/v6.12/process/submitting-patches.html#describe-your-changes
> [2]. https://lore.kernel.org/all/20250203175128.80319b42c9739f0d420080a4@linux-foundation.org/
>
> > Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> > ---
> >  kernel/pid.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/pid.c b/kernel/pid.c
> > index 924084713be8..2ae872f689a7 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -339,17 +339,23 @@ static struct pid **task_pid_ptr(struct task_struct *task, enum pid_type type)
> >   */
> >  void attach_pid(struct task_struct *task, enum pid_type type)
> >  {
> > -     struct pid *pid = *task_pid_ptr(task, type);
> > +     struct pid *pid;
> > +
> > +     lockdep_assert_held_write(&tasklist_lock);
> > +
> > +     pid = *task_pid_ptr(task, type);
> >       hlist_add_head_rcu(&task->pid_links[type], &pid->tasks[type]);
> >  }
> >
> >  static void __change_pid(struct task_struct *task, enum pid_type type,
> >                       struct pid *new)
> >  {
> > -     struct pid **pid_ptr = task_pid_ptr(task, type);
> > -     struct pid *pid;
> > +     struct pid **pid_ptr, *pid;
> >       int tmp;
> >
> > +     lockdep_assert_held_write(&tasklist_lock);
> > +
> > +     pid_ptr = task_pid_ptr(task, type);
> >       pid = *pid_ptr;
> >
> >       hlist_del_rcu(&task->pid_links[type]);
> > @@ -386,6 +392,8 @@ void exchange_tids(struct task_struct *left, struct task_struct *right)
> >       struct hlist_head *head1 = &pid1->tasks[PIDTYPE_PID];
> >       struct hlist_head *head2 = &pid2->tasks[PIDTYPE_PID];
> >
> > +     lockdep_assert_held_write(&tasklist_lock);
> > +
> >       /* Swap the single entry tid lists */
> >       hlists_swap_heads_rcu(head1, head2);
> >
> > @@ -403,6 +411,7 @@ void transfer_pid(struct task_struct *old, struct task_struct *new,
> >                          enum pid_type type)
> >  {
> >       WARN_ON_ONCE(type == PIDTYPE_PID);
> > +     lockdep_assert_held_write(&tasklist_lock);
> >       hlist_replace_rcu(&old->pid_links[type], &new->pid_links[type]);
> >  }
> >
> > --
> > 2.43.0
> >
> >
Liam R. Howlett Feb. 5, 2025, 8:42 p.m. UTC | #3 
* Mateusz Guzik <mjguzik@gmail.com> [250205 15:34]:
> On Wed, Feb 5, 2025 at 9:27 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> >
> > * Mateusz Guzik <mjguzik@gmail.com> [250205 14:33]:
> >
> > If a patch is worth doing, it's worth explaining.  I'm surprised you
> > didn't add a comment after the last revision missed a comment on patch
> > 2/6.  This is literally in the submitting patches document [1].
> >
> > I don't mean to delay this series, but I do want to know why things are
> > done when I'm hunting through git logs.  Having a change log isn't
> > optional, and now you know that Andrews script won't fix this problem
> > [2].
> >
> > I see you are upset by this considering the terse and lack of
> > punctuation in patch 2, but please try to understand these comments
> > serve a purpose in maintaining the code years later.
> >
> 
> I'm not upset.

Good, thanks - that wasn't my intention.

> 
> For this specific case I don't know what can be written in the body
> given the really self-explanatory nature of the change, other than to
> spell it out(?).

You could say why you added it?  Is this something that was seen
happening?

> 
> Does this work for you:
> The routines need to be called with the tasklist_lock, the asserts
> validate at runtime that this holds.

Well, you are checking this lock because it is protecting something
that's being changed.  You could say "the tasklist_lock protects X, make
sure that it's held"?

> 
> I also git log a lot and like to know what's up, to that end I
> appreciate *short* commit messages so that I know there is nothing
> more to the patch than meets the eye. In particular if there is
> nothing of value to add in the body, I appreciate if there is none.
> 
> But that's me, I'm not going to insist one way or the other.
> 
> > [1]. https://www.kernel.org/doc/html/v6.12/process/submitting-patches.html#describe-your-changes
> > [2]. https://lore.kernel.org/all/20250203175128.80319b42c9739f0d420080a4@linux-foundation.org/
> >
> > > Reviewed-by: Oleg Nesterov <oleg@redhat.com>
> > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> > > ---
> > >  kernel/pid.c | 15 ++++++++++++---
> > >  1 file changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/pid.c b/kernel/pid.c
> > > index 924084713be8..2ae872f689a7 100644
> > > --- a/kernel/pid.c
> > > +++ b/kernel/pid.c
> > > @@ -339,17 +339,23 @@ static struct pid **task_pid_ptr(struct task_struct *task, enum pid_type type)
> > >   */
> > >  void attach_pid(struct task_struct *task, enum pid_type type)
> > >  {
> > > -     struct pid *pid = *task_pid_ptr(task, type);
> > > +     struct pid *pid;
> > > +
> > > +     lockdep_assert_held_write(&tasklist_lock);
> > > +
> > > +     pid = *task_pid_ptr(task, type);
> > >       hlist_add_head_rcu(&task->pid_links[type], &pid->tasks[type]);
> > >  }
> > >
> > >  static void __change_pid(struct task_struct *task, enum pid_type type,
> > >                       struct pid *new)
> > >  {
> > > -     struct pid **pid_ptr = task_pid_ptr(task, type);
> > > -     struct pid *pid;
> > > +     struct pid **pid_ptr, *pid;
> > >       int tmp;
> > >
> > > +     lockdep_assert_held_write(&tasklist_lock);
> > > +
> > > +     pid_ptr = task_pid_ptr(task, type);
> > >       pid = *pid_ptr;
> > >
> > >       hlist_del_rcu(&task->pid_links[type]);
> > > @@ -386,6 +392,8 @@ void exchange_tids(struct task_struct *left, struct task_struct *right)
> > >       struct hlist_head *head1 = &pid1->tasks[PIDTYPE_PID];
> > >       struct hlist_head *head2 = &pid2->tasks[PIDTYPE_PID];
> > >
> > > +     lockdep_assert_held_write(&tasklist_lock);
> > > +
> > >       /* Swap the single entry tid lists */
> > >       hlists_swap_heads_rcu(head1, head2);
> > >
> > > @@ -403,6 +411,7 @@ void transfer_pid(struct task_struct *old, struct task_struct *new,
> > >                          enum pid_type type)
> > >  {
> > >       WARN_ON_ONCE(type == PIDTYPE_PID);
> > > +     lockdep_assert_held_write(&tasklist_lock);
> > >       hlist_replace_rcu(&old->pid_links[type], &new->pid_links[type]);
> > >  }
> > >
> > > --
> > > 2.43.0
> > >
> > >
> 
> 
> 
> -- 
> Mateusz Guzik <mjguzik gmail.com>
Mateusz Guzik Feb. 5, 2025, 8:58 p.m. UTC | #4 
On Wed, Feb 5, 2025 at 9:42 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Mateusz Guzik <mjguzik@gmail.com> [250205 15:34]:
> > For this specific case I don't know what can be written in the body
> > given the really self-explanatory nature of the change, other than to
> > spell it out(?).
>
> You could say why you added it?  Is this something that was seen
> happening?
>

I guess this is a cultural discrepancy, if you will.

I spent most of my time in a codebase which is very assert-heavy and
if anything you would need to justify *not* adding some, let alone for
locking.

Plugging a gap of the sort would not require any explanation.

The kernel has numerous examples of mere comments stating that a given
lock is required or no information whatsoever, which one can only
infer from context. I'm assuming this predates lockdep. Given that
lockdep asserts are nops on production kernels there is no legitimate
reason to continue like that (or *avoid* asserting on lock state) that
I can see.

I'm going to sleep on it, type up a sentence or two, maybe reword
other commit messages and resend.
diff mbox series

Patch

diff --git a/kernel/pid.c b/kernel/pid.c
index 924084713be8..2ae872f689a7 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -339,17 +339,23 @@  static struct pid **task_pid_ptr(struct task_struct *task, enum pid_type type)
  */
 void attach_pid(struct task_struct *task, enum pid_type type)
 {
-	struct pid *pid = *task_pid_ptr(task, type);
+	struct pid *pid;
+
+	lockdep_assert_held_write(&tasklist_lock);
+
+	pid = *task_pid_ptr(task, type);
 	hlist_add_head_rcu(&task->pid_links[type], &pid->tasks[type]);
 }
 
 static void __change_pid(struct task_struct *task, enum pid_type type,
 			struct pid *new)
 {
-	struct pid **pid_ptr = task_pid_ptr(task, type);
-	struct pid *pid;
+	struct pid **pid_ptr, *pid;
 	int tmp;
 
+	lockdep_assert_held_write(&tasklist_lock);
+
+	pid_ptr = task_pid_ptr(task, type);
 	pid = *pid_ptr;
 
 	hlist_del_rcu(&task->pid_links[type]);
@@ -386,6 +392,8 @@  void exchange_tids(struct task_struct *left, struct task_struct *right)
 	struct hlist_head *head1 = &pid1->tasks[PIDTYPE_PID];
 	struct hlist_head *head2 = &pid2->tasks[PIDTYPE_PID];
 
+	lockdep_assert_held_write(&tasklist_lock);
+
 	/* Swap the single entry tid lists */
 	hlists_swap_heads_rcu(head1, head2);
 
@@ -403,6 +411,7 @@  void transfer_pid(struct task_struct *old, struct task_struct *new,
 			   enum pid_type type)
 {
 	WARN_ON_ONCE(type == PIDTYPE_PID);
+	lockdep_assert_held_write(&tasklist_lock);
 	hlist_replace_rcu(&old->pid_links[type], &new->pid_links[type]);
 }