RCU Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] exit.c: Fix Sparse errors and warnings
@ 2020-01-30  6:20 madhuparnabhowmik10
  2020-01-30 10:31 ` Christian Brauner
  0 siblings, 1 reply; 9+ messages in thread
From: madhuparnabhowmik10 @ 2020-01-30  6:20 UTC (permalink / raw)
  To: peterz, mingo, oleg, christian.brauner, paulmck
  Cc: linux-kernel, joel, linux-kernel-mentees, rcu, Madhuparna Bhowmik

From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

This patch fixes the following sparse error:
kernel/exit.c:627:25: error: incompatible types in comparison expression

And the following warning:
kernel/exit.c:626:40: warning: incorrect type in assignment

Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
---
 kernel/exit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index bcbd59888e67..daf827a4aa25 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -623,8 +623,8 @@ static void forget_original_parent(struct task_struct *father,
 	reaper = find_new_reaper(father, reaper);
 	list_for_each_entry(p, &father->children, sibling) {
 		for_each_thread(p, t) {
-			t->real_parent = reaper;
-			BUG_ON((!t->ptrace) != (t->parent == father));
+			RCU_INIT_POINTER(t->real_parent, reaper);
+			BUG_ON((!t->ptrace) != (rcu_access_pointer(t->parent) == father));
 			if (likely(!t->ptrace))
 				t->parent = t->real_parent;
 			if (t->pdeath_signal)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] exit.c: Fix Sparse errors and warnings
  2020-01-30  6:20 [PATCH] exit.c: Fix Sparse errors and warnings madhuparnabhowmik10
@ 2020-01-30 10:31 ` Christian Brauner
  2020-01-30 11:33   ` Oleg Nesterov
  0 siblings, 1 reply; 9+ messages in thread
From: Christian Brauner @ 2020-01-30 10:31 UTC (permalink / raw)
  To: madhuparnabhowmik10
  Cc: peterz, mingo, oleg, paulmck, linux-kernel, joel,
	linux-kernel-mentees, rcu

On Thu, Jan 30, 2020 at 11:50:28AM +0530, madhuparnabhowmik10@gmail.com wrote:
> From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> 
> This patch fixes the following sparse error:
> kernel/exit.c:627:25: error: incompatible types in comparison expression
> 
> And the following warning:
> kernel/exit.c:626:40: warning: incorrect type in assignment
> 
> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

I think the previous version was already fine but hopefully
RCU_INIT_POINTER() really saves some overhead. In any case:

Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] exit.c: Fix Sparse errors and warnings
  2020-01-30 10:31 ` Christian Brauner
@ 2020-01-30 11:33   ` Oleg Nesterov
  2020-01-30 11:45     ` Christian Brauner
  0 siblings, 1 reply; 9+ messages in thread
From: Oleg Nesterov @ 2020-01-30 11:33 UTC (permalink / raw)
  To: Christian Brauner
  Cc: madhuparnabhowmik10, peterz, mingo, paulmck, linux-kernel, joel,
	linux-kernel-mentees, rcu

On 01/30, Christian Brauner wrote:
>
> On Thu, Jan 30, 2020 at 11:50:28AM +0530, madhuparnabhowmik10@gmail.com wrote:
> > From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >
> > This patch fixes the following sparse error:
> > kernel/exit.c:627:25: error: incompatible types in comparison expression
> >
> > And the following warning:
> > kernel/exit.c:626:40: warning: incorrect type in assignment
> >
> > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>
> I think the previous version was already fine but hopefully
> RCU_INIT_POINTER() really saves some overhead. In any case:

It is not about overhead, RCU_INIT_POINTER() documents the fact that we
didn't make any changes to the new parent, we only need to change the
pointer.

And btw, I don't really understand the __rcu annotations. Say, according
to sparse this code is wrong:

	int __rcu *P;

	void func(int *p)
	{
		P = p;
	}

OK, although quite possibly it is fine.

However, this code

	int __rcu *P;

	void func(int __rcu *p)
	{
		*p = 10;
	       	P = p;
	}

is almost certainly wrong but sparse is happy, asn is the same.


> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Acked-by: Oleg Nesterov <oleg@redhat.com>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] exit.c: Fix Sparse errors and warnings
  2020-01-30 11:33   ` Oleg Nesterov
@ 2020-01-30 11:45     ` Christian Brauner
  2020-01-30 15:20       ` Christian Brauner
  0 siblings, 1 reply; 9+ messages in thread
From: Christian Brauner @ 2020-01-30 11:45 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: madhuparnabhowmik10, peterz, mingo, paulmck, linux-kernel, joel,
	linux-kernel-mentees, rcu

On January 30, 2020 12:33:41 PM GMT+01:00, Oleg Nesterov <oleg@redhat.com> wrote:
>On 01/30, Christian Brauner wrote:
>>
>> On Thu, Jan 30, 2020 at 11:50:28AM +0530,
>madhuparnabhowmik10@gmail.com wrote:
>> > From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>> >
>> > This patch fixes the following sparse error:
>> > kernel/exit.c:627:25: error: incompatible types in comparison
>expression
>> >
>> > And the following warning:
>> > kernel/exit.c:626:40: warning: incorrect type in assignment
>> >
>> > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
>>
>> I think the previous version was already fine but hopefully
>> RCU_INIT_POINTER() really saves some overhead. In any case:
>
>It is not about overhead, RCU_INIT_POINTER() documents the fact that we
>didn't make any changes to the new parent, we only need to change the
>pointer.

Right, I wasn't complaining.  RCU_INIT_POINTER() claims that it has less overhead than rcu_assign_pointer().
So that is an additional argument for it.

>
>And btw, I don't really understand the __rcu annotations. Say,
>according
>to sparse this code is wrong:
>
>	int __rcu *P;
>
>	void func(int *p)
>	{
>		P = p;
>	}
>
>OK, although quite possibly it is fine.
>
>However, this code
>
>	int __rcu *P;
>
>	void func(int __rcu *p)
>	{
>		*p = 10;
>	       	P = p;
>	}
>
>is almost certainly wrong but sparse is happy, asn is the same.

That's more an argument to fix sparse I guess?
The annotations themselves are rather useful I think.
They at least help me when reading the code.
It's not that rcu lifetimes are trivial and anything that helps remind me that an object wants rcu semantics I'm happy to take it. :)

>
>
>> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
>
>Acked-by: Oleg Nesterov <oleg@redhat.com>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] exit.c: Fix Sparse errors and warnings
  2020-01-30 11:45     ` Christian Brauner
@ 2020-01-30 15:20       ` Christian Brauner
  0 siblings, 0 replies; 9+ messages in thread
From: Christian Brauner @ 2020-01-30 15:20 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: madhuparnabhowmik10, peterz, mingo, paulmck, linux-kernel, joel,
	linux-kernel-mentees, rcu

On Thu, Jan 30, 2020 at 12:45:26PM +0100, Christian Brauner wrote:
> On January 30, 2020 12:33:41 PM GMT+01:00, Oleg Nesterov <oleg@redhat.com> wrote:
> >On 01/30, Christian Brauner wrote:
> >>
> >> On Thu, Jan 30, 2020 at 11:50:28AM +0530,
> >madhuparnabhowmik10@gmail.com wrote:
> >> > From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >> >
> >> > This patch fixes the following sparse error:
> >> > kernel/exit.c:627:25: error: incompatible types in comparison
> >expression
> >> >
> >> > And the following warning:
> >> > kernel/exit.c:626:40: warning: incorrect type in assignment
> >> >
> >> > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> >>
> >> I think the previous version was already fine but hopefully
> >> RCU_INIT_POINTER() really saves some overhead. In any case:
> >
> >It is not about overhead, RCU_INIT_POINTER() documents the fact that we
> >didn't make any changes to the new parent, we only need to change the
> >pointer.
> 
> Right, I wasn't complaining.  RCU_INIT_POINTER() claims that it has less overhead than rcu_assign_pointer().
> So that is an additional argument for it.
> 
> >
> >And btw, I don't really understand the __rcu annotations. Say,
> >according
> >to sparse this code is wrong:
> >
> >	int __rcu *P;
> >
> >	void func(int *p)
> >	{
> >		P = p;
> >	}
> >
> >OK, although quite possibly it is fine.
> >
> >However, this code
> >
> >	int __rcu *P;
> >
> >	void func(int __rcu *p)
> >	{
> >		*p = 10;
> >	       	P = p;
> >	}
> >
> >is almost certainly wrong but sparse is happy, asn is the same.
> 
> That's more an argument to fix sparse I guess?
> The annotations themselves are rather useful I think.
> They at least help me when reading the code.
> It's not that rcu lifetimes are trivial and anything that helps remind me that an object wants rcu semantics I'm happy to take it. :)
> 
> >
> >
> >> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> >
> >Acked-by: Oleg Nesterov <oleg@redhat.com>

Thanks, applied for post -rc1.
Christian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] exit.c: Fix Sparse errors and warnings
  2020-01-29 12:30 ` Oleg Nesterov
@ 2020-01-29 16:02   ` Madhuparna Bhowmik
  0 siblings, 0 replies; 9+ messages in thread
From: Madhuparna Bhowmik @ 2020-01-29 16:02 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: madhuparnabhowmik10, peterz, mingo, christian.brauner, paulmck,
	linux-kernel, joel, linux-kernel-mentees, rcu

On Wed, Jan 29, 2020 at 01:30:47PM +0100, Oleg Nesterov wrote:
> On 01/28, madhuparnabhowmik10@gmail.com wrote:
> >
> > kernel/exit.c:626:40: warning: incorrect type in assignment
> > 
> > Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> > ---
> >  kernel/exit.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/exit.c b/kernel/exit.c
> > index bcbd59888e67..c5a9d6360440 100644
> > --- a/kernel/exit.c
> > +++ b/kernel/exit.c
> > @@ -623,8 +623,8 @@ static void forget_original_parent(struct task_struct *father,
> >  	reaper = find_new_reaper(father, reaper);
> >  	list_for_each_entry(p, &father->children, sibling) {
> >  		for_each_thread(p, t) {
> > -			t->real_parent = reaper;
> > -			BUG_ON((!t->ptrace) != (t->parent == father));
> > +			rcu_assign_pointer(t->real_parent, reaper);
> 
> Another case when RCU_INIT_POINTER() makes more sense (although to me it
> too looks confusing). We didn't modify the new parent.
>
Alright, I will resend this patch with RCU_INIT_POINTER().

Thank you,
Madhuparna

> Oleg.
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] exit.c: Fix Sparse errors and warnings
  2020-01-28 17:20 madhuparnabhowmik10
  2020-01-28 18:01 ` Christian Brauner
@ 2020-01-29 12:30 ` Oleg Nesterov
  2020-01-29 16:02   ` Madhuparna Bhowmik
  1 sibling, 1 reply; 9+ messages in thread
From: Oleg Nesterov @ 2020-01-29 12:30 UTC (permalink / raw)
  To: madhuparnabhowmik10
  Cc: peterz, mingo, christian.brauner, paulmck, linux-kernel, joel,
	linux-kernel-mentees, rcu

On 01/28, madhuparnabhowmik10@gmail.com wrote:
>
> kernel/exit.c:626:40: warning: incorrect type in assignment
> 
> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> ---
>  kernel/exit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/exit.c b/kernel/exit.c
> index bcbd59888e67..c5a9d6360440 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -623,8 +623,8 @@ static void forget_original_parent(struct task_struct *father,
>  	reaper = find_new_reaper(father, reaper);
>  	list_for_each_entry(p, &father->children, sibling) {
>  		for_each_thread(p, t) {
> -			t->real_parent = reaper;
> -			BUG_ON((!t->ptrace) != (t->parent == father));
> +			rcu_assign_pointer(t->real_parent, reaper);

Another case when RCU_INIT_POINTER() makes more sense (although to me it
too looks confusing). We didn't modify the new parent.

Oleg.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] exit.c: Fix Sparse errors and warnings
  2020-01-28 17:20 madhuparnabhowmik10
@ 2020-01-28 18:01 ` Christian Brauner
  2020-01-29 12:30 ` Oleg Nesterov
  1 sibling, 0 replies; 9+ messages in thread
From: Christian Brauner @ 2020-01-28 18:01 UTC (permalink / raw)
  To: madhuparnabhowmik10
  Cc: peterz, mingo, oleg, paulmck, linux-kernel, joel,
	linux-kernel-mentees, rcu

On Tue, Jan 28, 2020 at 10:50:08PM +0530, madhuparnabhowmik10@gmail.com wrote:
> From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
> 
> This patch fixes the following sparse error:
> kernel/exit.c:627:25: error: incompatible types in comparison expression
> caused due to accessing rcu protected pointer without using rcu primitives.
> 
> And the following warning:
> 
> kernel/exit.c:626:40: warning: incorrect type in assignment
> 
> Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

Thanks, looks good to me!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] exit.c: Fix Sparse errors and warnings
@ 2020-01-28 17:20 madhuparnabhowmik10
  2020-01-28 18:01 ` Christian Brauner
  2020-01-29 12:30 ` Oleg Nesterov
  0 siblings, 2 replies; 9+ messages in thread
From: madhuparnabhowmik10 @ 2020-01-28 17:20 UTC (permalink / raw)
  To: peterz, mingo, oleg, christian.brauner, paulmck
  Cc: linux-kernel, joel, linux-kernel-mentees, rcu, Madhuparna Bhowmik

From: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>

This patch fixes the following sparse error:
kernel/exit.c:627:25: error: incompatible types in comparison expression
caused due to accessing rcu protected pointer without using rcu primitives.

And the following warning:

kernel/exit.c:626:40: warning: incorrect type in assignment

Signed-off-by: Madhuparna Bhowmik <madhuparnabhowmik10@gmail.com>
---
 kernel/exit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index bcbd59888e67..c5a9d6360440 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -623,8 +623,8 @@ static void forget_original_parent(struct task_struct *father,
 	reaper = find_new_reaper(father, reaper);
 	list_for_each_entry(p, &father->children, sibling) {
 		for_each_thread(p, t) {
-			t->real_parent = reaper;
-			BUG_ON((!t->ptrace) != (t->parent == father));
+			rcu_assign_pointer(t->real_parent, reaper);
+			BUG_ON((!t->ptrace) != (rcu_access_pointer(t->parent) == father));
 			if (likely(!t->ptrace))
 				t->parent = t->real_parent;
 			if (t->pdeath_signal)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-30  6:20 [PATCH] exit.c: Fix Sparse errors and warnings madhuparnabhowmik10
2020-01-30 10:31 ` Christian Brauner
2020-01-30 11:33   ` Oleg Nesterov
2020-01-30 11:45     ` Christian Brauner
2020-01-30 15:20       ` Christian Brauner
  -- strict thread matches above, loose matches on Subject: below --
2020-01-28 17:20 madhuparnabhowmik10
2020-01-28 18:01 ` Christian Brauner
2020-01-29 12:30 ` Oleg Nesterov
2020-01-29 16:02   ` Madhuparna Bhowmik

RCU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/rcu/0 rcu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 rcu rcu/ https://lore.kernel.org/rcu \
		rcu@vger.kernel.org
	public-inbox-index rcu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.rcu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git