Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1343209 > unrolled thread

Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical sections

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical  sections Peter Zijlstra <peterz@infradead.org> - 2016-02-25 15:40 +0100
    Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical  sections "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-02-25 16:40 +0100
      Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical  sections Boqun Feng <boqun.feng@gmail.com> - 2016-02-26 04:10 +0100
        Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical  sections Peter Zijlstra <peterz@infradead.org> - 2016-02-26 12:30 +0100
          Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical  sections Boqun Feng <boqun.feng@gmail.com> - 2016-02-29 02:20 +0100
            Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical  sections Peter Zijlstra <peterz@infradead.org> - 2016-02-29 13:50 +0100
              Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical  sections Boqun Feng <boqun.feng@gmail.com> - 2016-03-01 10:40 +0100
                Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical  sections Peter Zijlstra <peterz@infradead.org> - 2016-03-01 11:00 +0100
                  Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical  sections Peter Zijlstra <peterz@infradead.org> - 2016-03-01 11:10 +0100
                    Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical  sections Boqun Feng <boqun.feng@gmail.com> - 2016-03-02 07:40 +0100
                      Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical  sections Peter Zijlstra <peterz@infradead.org> - 2016-03-02 11:20 +0100
                        Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical  sections "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-03-02 15:10 +0100

#1343209 — Re: [RFC v2 0/6] Track RCU dereferences in RCU read-side critical sections

On Tue, Feb 16, 2016 at 01:57:39PM +0800, Boqun Feng wrote:
> As a characteristic of RCU, read-side critical sections have a very
> loose connection with rcu_dereference()s, which is you can only be sure
> about an rcu_dereference() might be called in some read-side critical
> section, but if code gets complex, you may not be sure which read-side
> critical section exactly, this might be also an problem for some other
> locking mechanisms, that is the critical sections protecting data and
> the data accesses protected are not clearly correlated.
> 
> In this series, we are introducing LOCKED_ACCESS framework and based on
> which, we implement the RCU_LOCKED_ACCESS functionality to give us a
> clear hint: which rcu_dereference() happens in which RCU read-side
> critical section. 
> 
> After this series applied, and if CONFIG_RCU_LOCKED_ACCESS=y, the proc
> file /proc/locked_access/rcu will show all relationships collected so
> far for rcu_read_lock() and their friends and rcu_dereference*().
> 

But why !? What does this bring us, why do I want to even look at these
patches?

[toc] | [next] | [standalone]

#1343276

On Thu, Feb 25, 2016 at 03:32:43PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 16, 2016 at 01:57:39PM +0800, Boqun Feng wrote:
> > As a characteristic of RCU, read-side critical sections have a very
> > loose connection with rcu_dereference()s, which is you can only be sure
> > about an rcu_dereference() might be called in some read-side critical
> > section, but if code gets complex, you may not be sure which read-side
> > critical section exactly, this might be also an problem for some other
> > locking mechanisms, that is the critical sections protecting data and
> > the data accesses protected are not clearly correlated.
> > 
> > In this series, we are introducing LOCKED_ACCESS framework and based on
> > which, we implement the RCU_LOCKED_ACCESS functionality to give us a
> > clear hint: which rcu_dereference() happens in which RCU read-side
> > critical section. 
> > 
> > After this series applied, and if CONFIG_RCU_LOCKED_ACCESS=y, the proc
> > file /proc/locked_access/rcu will show all relationships collected so
> > far for rcu_read_lock() and their friends and rcu_dereference*().
> 
> But why !? What does this bring us, why do I want to even look at these
> patches?

There were some complaints about the difficulty of figuring out what
was being protected by a given rcu_read_lock() in cases where the
corresponding rcu_dereference() is several function calls down, and
especially in cases where the function calls are via pointers.
These cases show up in a number of places, perhaps most prominently
in networking.

Boqun's patches therefore use lockdep to make an association between
each rcu_dereference() and the rcu_read_lock() protecting it.

Seem reasonable, or were the complaints just a flash in the pan?

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1343758

[Multipart message — attachments visible in raw view] — view raw

On Thu, Feb 25, 2016 at 07:37:24AM -0800, Paul E. McKenney wrote:
> On Thu, Feb 25, 2016 at 03:32:43PM +0100, Peter Zijlstra wrote:
> > On Tue, Feb 16, 2016 at 01:57:39PM +0800, Boqun Feng wrote:
> > > As a characteristic of RCU, read-side critical sections have a very
> > > loose connection with rcu_dereference()s, which is you can only be sure
> > > about an rcu_dereference() might be called in some read-side critical
> > > section, but if code gets complex, you may not be sure which read-side
> > > critical section exactly, this might be also an problem for some other
> > > locking mechanisms, that is the critical sections protecting data and
> > > the data accesses protected are not clearly correlated.
> > > 
> > > In this series, we are introducing LOCKED_ACCESS framework and based on
> > > which, we implement the RCU_LOCKED_ACCESS functionality to give us a
> > > clear hint: which rcu_dereference() happens in which RCU read-side
> > > critical section. 
> > > 
> > > After this series applied, and if CONFIG_RCU_LOCKED_ACCESS=y, the proc
> > > file /proc/locked_access/rcu will show all relationships collected so
> > > far for rcu_read_lock() and their friends and rcu_dereference*().
> > 
> > But why !? What does this bring us, why do I want to even look at these
> > patches?
> 
> There were some complaints about the difficulty of figuring out what
> was being protected by a given rcu_read_lock() in cases where the
> corresponding rcu_dereference() is several function calls down, and
> especially in cases where the function calls are via pointers.
> These cases show up in a number of places, perhaps most prominently
> in networking.
> 
> Boqun's patches therefore use lockdep to make an association between
> each rcu_dereference() and the rcu_read_lock() protecting it.
> 

Yes, this patch is aiming to provide some information about this,
actually the background of this patchset is a discussion between Ingo
and Paul last year:

http://lkml.kernel.org/g/20150414102505.GA13015@gmail.com

In that discussion, Ingo gave one example about how hard it is to figure
out what a RCU read-side critical section is protecting. Ingo's proposal
of solving this problem was to add an extra parameter for RCU related
primitives, however, Paul seemed to have concerns about that approach,
because there were many RCU users' code that would need to be modified
and there were corner cases where the one extra parameter was not enough
or not necessary.

Therefore I tried to figure out a way for making the association of
rcu_dereference() and rcu_read_lock() automatically without the
modification of code of the RCU users. That's how this patchset
comes.

Regards,
Boqun

> Seem reasonable, or were the complaints just a flash in the pan?
> 
> 							Thanx, Paul
> 

[toc] | [prev] | [next] | [standalone]

#1344182

On Fri, Feb 26, 2016 at 11:06:27AM +0800, Boqun Feng wrote:
> On Thu, Feb 25, 2016 at 07:37:24AM -0800, Paul E. McKenney wrote:
> > On Thu, Feb 25, 2016 at 03:32:43PM +0100, Peter Zijlstra wrote:
> > > On Tue, Feb 16, 2016 at 01:57:39PM +0800, Boqun Feng wrote:
> > > > As a characteristic of RCU, read-side critical sections have a very
> > > > loose connection with rcu_dereference()s, which is you can only be sure
> > > > about an rcu_dereference() might be called in some read-side critical
> > > > section, but if code gets complex, you may not be sure which read-side
> > > > critical section exactly, this might be also an problem for some other
> > > > locking mechanisms, that is the critical sections protecting data and
> > > > the data accesses protected are not clearly correlated.
> > > > 
> > > > In this series, we are introducing LOCKED_ACCESS framework and based on
> > > > which, we implement the RCU_LOCKED_ACCESS functionality to give us a
> > > > clear hint: which rcu_dereference() happens in which RCU read-side
> > > > critical section. 
> > > > 
> > > > After this series applied, and if CONFIG_RCU_LOCKED_ACCESS=y, the proc
> > > > file /proc/locked_access/rcu will show all relationships collected so
> > > > far for rcu_read_lock() and their friends and rcu_dereference*().
> > > 
> > > But why !? What does this bring us, why do I want to even look at these
> > > patches?
> > 
> > There were some complaints about the difficulty of figuring out what
> > was being protected by a given rcu_read_lock() in cases where the
> > corresponding rcu_dereference() is several function calls down, and
> > especially in cases where the function calls are via pointers.
> > These cases show up in a number of places, perhaps most prominently
> > in networking.
> > Boqun's patches therefore use lockdep to make an association between
> > each rcu_dereference() and the rcu_read_lock() protecting it.
> > 
> 
> Yes, this patch is aiming to provide some information about this,
> actually the background of this patchset is a discussion between Ingo
> and Paul last year:
> 
> http://lkml.kernel.org/g/20150414102505.GA13015@gmail.com
> 
> In that discussion, Ingo gave one example about how hard it is to figure
> out what a RCU read-side critical section is protecting. Ingo's proposal
> of solving this problem was to add an extra parameter for RCU related
> primitives, however, Paul seemed to have concerns about that approach,
> because there were many RCU users' code that would need to be modified
> and there were corner cases where the one extra parameter was not enough
> or not necessary.
> 
> Therefore I tried to figure out a way for making the association of
> rcu_dereference() and rcu_read_lock() automatically without the
> modification of code of the RCU users. That's how this patchset
> comes.

So the problem I have with this is that your text didn't have a problem
statement, and if there's no problem, what the heck is all this code
for.

Secondly, I'm not sure a random list of data is going to help anybody.

The whole point of lockdep is that it tells you if you do broken. This
doesn't, and cannot, because it lacks information to verify against.

So I'm still not sure this is useful. Also, I would argue your code has
problems if you cannot even find your rcu_read_lock().

And yes, networking is a special snowflake, they very often push their
rcu_read_lock(),local_bh_disable(),preempt_disable() whatnot, all the
way out, because cycles.

Which makes it entirely impossible to tell what all it guards. That's a
very concious choice they make, but its a royal pain for a number of
things. I'm not sure we can do anything about it.

So no, I'm not convinced. Again, why is this useful?

[toc] | [prev] | [next] | [standalone]

#1345431

[Multipart message — attachments visible in raw view] — view raw

On Fri, Feb 26, 2016 at 12:25:21PM +0100, Peter Zijlstra wrote:
> On Fri, Feb 26, 2016 at 11:06:27AM +0800, Boqun Feng wrote:
> > On Thu, Feb 25, 2016 at 07:37:24AM -0800, Paul E. McKenney wrote:
> > > On Thu, Feb 25, 2016 at 03:32:43PM +0100, Peter Zijlstra wrote:
> > > > On Tue, Feb 16, 2016 at 01:57:39PM +0800, Boqun Feng wrote:
> > > > > As a characteristic of RCU, read-side critical sections have a very
> > > > > loose connection with rcu_dereference()s, which is you can only be sure
> > > > > about an rcu_dereference() might be called in some read-side critical
> > > > > section, but if code gets complex, you may not be sure which read-side
> > > > > critical section exactly, this might be also an problem for some other
> > > > > locking mechanisms, that is the critical sections protecting data and
> > > > > the data accesses protected are not clearly correlated.
> > > > > 
> > > > > In this series, we are introducing LOCKED_ACCESS framework and based on
> > > > > which, we implement the RCU_LOCKED_ACCESS functionality to give us a
> > > > > clear hint: which rcu_dereference() happens in which RCU read-side
> > > > > critical section. 
> > > > > 
> > > > > After this series applied, and if CONFIG_RCU_LOCKED_ACCESS=y, the proc
> > > > > file /proc/locked_access/rcu will show all relationships collected so
> > > > > far for rcu_read_lock() and their friends and rcu_dereference*().
> > > > 
> > > > But why !? What does this bring us, why do I want to even look at these
> > > > patches?
> > > 
> > > There were some complaints about the difficulty of figuring out what
> > > was being protected by a given rcu_read_lock() in cases where the
> > > corresponding rcu_dereference() is several function calls down, and
> > > especially in cases where the function calls are via pointers.
> > > These cases show up in a number of places, perhaps most prominently
> > > in networking.
> > > Boqun's patches therefore use lockdep to make an association between
> > > each rcu_dereference() and the rcu_read_lock() protecting it.
> > > 
> > 
> > Yes, this patch is aiming to provide some information about this,
> > actually the background of this patchset is a discussion between Ingo
> > and Paul last year:
> > 
> > http://lkml.kernel.org/g/20150414102505.GA13015@gmail.com
> > 
> > In that discussion, Ingo gave one example about how hard it is to figure
> > out what a RCU read-side critical section is protecting. Ingo's proposal
> > of solving this problem was to add an extra parameter for RCU related
> > primitives, however, Paul seemed to have concerns about that approach,
> > because there were many RCU users' code that would need to be modified
> > and there were corner cases where the one extra parameter was not enough
> > or not necessary.
> > 
> > Therefore I tried to figure out a way for making the association of
> > rcu_dereference() and rcu_read_lock() automatically without the
> > modification of code of the RCU users. That's how this patchset
> > comes.
> 
> So the problem I have with this is that your text didn't have a problem
> statement, and if there's no problem, what the heck is all this code
> for.
> 

You are right, that's my bad. I should have done this better. Sorry
about that..

Please allow me to restate the problem and what this patchset can
help ;-)

The problem is:

Currently there is no way to know which rcu_dereference() and its
friends a rcu_read_lock() or one of its friends is protecting. And
the lack of such information is a pain for kernel developers and
reviewers to understand the purpose of a certain rcu_read_lock().

And this patchset is trying to provide the information to developers.

> Secondly, I'm not sure a random list of data is going to help anybody.
> 

One possible usage might be:

If someone is going to refactor a piece of code which originally uses
RCU for synchronization, and now he wants to switch to other
synchronization mechanism for some data because of some reason(for
consistency or writer side latency guarantee, etc.). Before he makes the
change, he needs to know if this rcu_read_lock() also protects other
data, and this is not an easy job and I don't think we have anything
helpful for this. With RCU_LOCKED_ACCESS, he can build the kernel with
RCU_LOCKED_ACCESS enabled, run some workloads and get the information
from a /proc file. In this way, RCU_LOCKED_ACCESS is helpful to
developers who want to know the information of the associations between
rcu_read_lock() and rcu_dereference().

> The whole point of lockdep is that it tells you if you do broken. This
> doesn't, and cannot, because it lacks information to verify against.
> 

Yes.. RCU_LOCKED_ACCESS is not a validator, so it cannot be as useful as
lockdep for finding bug.

> So I'm still not sure this is useful. Also, I would argue your code has
> problems if you cannot even find your rcu_read_lock().
> 

I think what you mean here is, for example, the case where we use
preempt_disable() instead of rcu_read_lock_sched() to pair with
synchronize_sched(), right?

If so, sure, this series cannot do anything about this, however, this
series is to correlate rcu_read_lock() and its friends with
rcu_dereference() and its friends. So it may miss some RCU usages if
rcu_read_lock() and its friends are not used. But these missing usages
can be covered in a similar way as this series, I can either add it
in the next version of this series or provide it as a separate
follow-up patchset, of course, if we all agree the problem is worth
solving and this patchset is helpful.

Or I'm missing your point here?

> And yes, networking is a special snowflake, they very often push their
> rcu_read_lock(),local_bh_disable(),preempt_disable() whatnot, all the
> way out, because cycles.
> 
> Which makes it entirely impossible to tell what all it guards. That's a
> very concious choice they make, but its a royal pain for a number of
> things. I'm not sure we can do anything about it.
> 
> So no, I'm not convinced. Again, why is this useful?

Sorry again for not providing enough text for you to review this series,
hope now it's more clear.

Looking forwards to your thoughts, thank you ;-)

If you think this series deserves a reconsideration I will send out a
new version with more text about what's the problem and why this is
useful.

Regards,
Boqun

[toc] | [prev] | [next] | [standalone]

#1345762

On Mon, Feb 29, 2016 at 09:12:20AM +0800, Boqun Feng wrote:

> The problem is:
> 
> Currently there is no way to know which rcu_dereference() and its
> friends a rcu_read_lock() or one of its friends is protecting. And
> the lack of such information is a pain for kernel developers and
> reviewers to understand the purpose of a certain rcu_read_lock().

If I have to go run a kernel and look at /proc data in order to complete
a review the patch will fail review, end of story.

> > Secondly, I'm not sure a random list of data is going to help anybody.
> > 
> 
> One possible usage might be:
> 
> If someone is going to refactor a piece of code which originally uses
> RCU for synchronization, and now he wants to switch to other
> synchronization mechanism for some data because of some reason(for
> consistency or writer side latency guarantee, etc.). Before he makes the
> change, he needs to know if this rcu_read_lock() also protects other
> data, and this is not an easy job and I don't think we have anything
> helpful for this. With RCU_LOCKED_ACCESS, he can build the kernel with
> RCU_LOCKED_ACCESS enabled, run some workloads and get the information
> from a /proc file. In this way, RCU_LOCKED_ACCESS is helpful to
> developers who want to know the information of the associations between
> rcu_read_lock() and rcu_dereference().

So 'possible', but will anyone actually do this? `

> > The whole point of lockdep is that it tells you if you do broken. This
> > doesn't, and cannot, because it lacks information to verify against.
> > 
> 
> Yes.. RCU_LOCKED_ACCESS is not a validator, so it cannot be as useful as
> lockdep for finding bug.

I would try really _really_ hard to make it a validator, those are so
much more useful.

One could for example allow something like:

	rcu_read_lock();
	rcu_annotate(&var->field);

	foo();

	rcu_read_unlock();

As an alternative to the syntax suggested by Ingo. This would allow
keeping the existing rcu_read_lock() signature so you don't have to
force update the entire kernel at once, while also (easily) allowing
multiple variables. Like:

	rcu_read_lock();
	rcu_annotate(&var->field);
	rcu_annotate(&var2->field2);

You can then have a special rule that if a particular RCU section has an
annotation, any rcu_dereference() not matched will field a warning. If
the annotation section is empty, nothing.


> > So I'm still not sure this is useful. Also, I would argue your code has
> > problems if you cannot even find your rcu_read_lock().
> > 
> 
> I think what you mean here is, for example, the case where we use
> preempt_disable() instead of rcu_read_lock_sched() to pair with
> synchronize_sched(), right?

No, I was more like:

	rcu_read_lock();
	foo()
	  bar()
	    var->func();
	      obj->func();
	        whatever();

and you're looking at a change to whatever() and wonder where the heck
the corresponding rcu_read_lock() lives and if we're having it held at
all.

> If so, sure, this series cannot do anything about this, however, this

Why not? Can't you stick whatever you need into preempt_disable() ?
kernel/sched/core.c:preempt_count_{add,sub}() are there for a (debug)
reason.

> If you think this series deserves a reconsideration I will send out a
> new version with more text about what's the problem and why this is
> useful.

So please make it a validator. That's so much more useful.

[toc] | [prev] | [next] | [standalone]

#1346482

[Multipart message — attachments visible in raw view] — view raw

On Mon, Feb 29, 2016 at 01:43:18PM +0100, Peter Zijlstra wrote:
> On Mon, Feb 29, 2016 at 09:12:20AM +0800, Boqun Feng wrote:
> 
> > The problem is:
> > 
> > Currently there is no way to know which rcu_dereference() and its
> > friends a rcu_read_lock() or one of its friends is protecting. And
> > the lack of such information is a pain for kernel developers and
> > reviewers to understand the purpose of a certain rcu_read_lock().
> 
> If I have to go run a kernel and look at /proc data in order to complete
> a review the patch will fail review, end of story.
> 

OK, got it.

> > > Secondly, I'm not sure a random list of data is going to help anybody.
> > > 
> > 
> > One possible usage might be:
> > 
> > If someone is going to refactor a piece of code which originally uses
> > RCU for synchronization, and now he wants to switch to other
> > synchronization mechanism for some data because of some reason(for
> > consistency or writer side latency guarantee, etc.). Before he makes the
> > change, he needs to know if this rcu_read_lock() also protects other
> > data, and this is not an easy job and I don't think we have anything
> > helpful for this. With RCU_LOCKED_ACCESS, he can build the kernel with
> > RCU_LOCKED_ACCESS enabled, run some workloads and get the information
> > from a /proc file. In this way, RCU_LOCKED_ACCESS is helpful to
> > developers who want to know the information of the associations between
> > rcu_read_lock() and rcu_dereference().
> 
> So 'possible', but will anyone actually do this? `
> 

Not sure, but one of the purposes of sending out this RFC is telling
people we have a way to do this and see if they are interested in ;-)

> > > The whole point of lockdep is that it tells you if you do broken. This
> > > doesn't, and cannot, because it lacks information to verify against.
> > > 
> > 
> > Yes.. RCU_LOCKED_ACCESS is not a validator, so it cannot be as useful as
> > lockdep for finding bug.
> 
> I would try really _really_ hard to make it a validator, those are so
> much more useful.
> 

Agreed. I would love to make it a validator too. But I think that
information collection is the first step and an essential part for
making a validator, right? And what RCU_LOCKED_ACCESS does are
collecting information and showing that via a proc file. I really think
it's better to know what information we can collect before we actually
try to make a validator, besides, the proc file output could serve as a
way to debug the validator if we really had a validator.

So maybe we can first discuss about and work on how to collect and show
the information, and then seek a way for making a validator?

> One could for example allow something like:
> 
> 	rcu_read_lock();
> 	rcu_annotate(&var->field);
> 
> 	foo();
> 
> 	rcu_read_unlock();
> 
> As an alternative to the syntax suggested by Ingo. This would allow
> keeping the existing rcu_read_lock() signature so you don't have to
> force update the entire kernel at once, while also (easily) allowing
> multiple variables. Like:
> 
> 	rcu_read_lock();
> 	rcu_annotate(&var->field);
> 	rcu_annotate(&var2->field2);
> 
> You can then have a special rule that if a particular RCU section has an
> annotation, any rcu_dereference() not matched will field a warning. If
> the annotation section is empty, nothing.
> 

Good idea! but I don't think annotating a field in C language is easy,
I will try to see what we can get. Do you have something already in your
mind?

> 
> > > So I'm still not sure this is useful. Also, I would argue your code has
> > > problems if you cannot even find your rcu_read_lock().
> > > 
> > 
> > I think what you mean here is, for example, the case where we use
> > preempt_disable() instead of rcu_read_lock_sched() to pair with
> > synchronize_sched(), right?
> 
> No, I was more like:
> 
> 	rcu_read_lock();
> 	foo()
> 	  bar()
> 	    var->func();
> 	      obj->func();
> 	        whatever();
> 
> and you're looking at a change to whatever() and wonder where the heck
> the corresponding rcu_read_lock() lives and if we're having it held at
> all.
> 

Confused.. RCU_LOCKED_ACCESS has such information, For example, in the
piece of /proc/locked_access/rcu I put in the cover letter, which I will
put in the commit logs for the next version of this series:

ACQCHAIN 0xfe042af3bbfb2605, 3 locks, irq_context 0:
  LOCK at [<ffffffff81094b47>] SyS_kill+0x97/0x2a0
    LOCK at [<ffffffff8109286f>] kill_pid_info+0x1f/0x140
      LOCK at [<ffffffff81092605>] group_send_sig_info+0x5/0x130
        ACCESS TYPE 1 at kernel/signal.c:695

, which actually reflects:

	(in kernel/signal.c)
	SyS_kill()
	  kill_something_info()
	    rcu_read_lock()
	    kill_pid_info()
	      rcu_read_lock()
	      group_send_sig_info()
	        rcu_read_lock()
		check_kill_permission()
		  kill_ok_by_cred()
		    __task_cred()

and __task_cred() is an rcu_dereference(), so if anyone wants to figure
out what rcu_read_lock()s are hold when this deref happens, he can dump
the proc file, and search for the filename and linenumber of
__task_cred() and he will get the symbol + offset of all the
rcu_read_lock()s for this deref.


(Yes, I know there is an inconsistency on symbol + offset versus
filename + linenumber here, the reason of this inconsistency is that
the original purpose of this feature is to show some information to
developers to help understand code, therefore filename + linenumber is
more appropriate for this purpose. So I use filename + linenumber to
indicate the locations for rcu_dereference(), however, as what I can
only get from the task_struct::held_locks is acquire_ip, so I have to
use the symbol + offset for rcu_read_lock(), and there comes the
inconsistency.  Maybe I should make them all symbol + offset, and use
tools like addr2line to convert to filename + linenumber.)

> > If so, sure, this series cannot do anything about this, however, this
> 
> Why not? Can't you stick whatever you need into preempt_disable() ?
> kernel/sched/core.c:preempt_count_{add,sub}() are there for a (debug)
> reason.
> 

Yeah, you are right. I can do something similar to preempt_disable() by
maintaining a stack of callsites of preempt_disable() for each thread,
which means a little much memory cost, but it's worthwhile if we make
it useful.

> > If you think this series deserves a reconsideration I will send out a
> > new version with more text about what's the problem and why this is
> > useful.
> 
> So please make it a validator. That's so much more useful.

I cerntainly have done a bad job of the explanation of this series in
the cover letter and the commit logs, so I'm considering sending the
next version of this series with better text and examples, or you prefer
I should try to make it a validator first and send out as a whole for
reviewing?

Thank you for your thoughts and comments ;-)

Regards,
Boqun

[toc] | [prev] | [next] | [standalone]

#1346491

On Tue, Mar 01, 2016 at 05:32:42PM +0800, Boqun Feng wrote:
> > One could for example allow something like:
> > 
> > 	rcu_read_lock();
> > 	rcu_annotate(&var->field);
> > 
> > 	foo();
> > 
> > 	rcu_read_unlock();
> > 
> > As an alternative to the syntax suggested by Ingo. This would allow
> > keeping the existing rcu_read_lock() signature so you don't have to
> > force update the entire kernel at once, while also (easily) allowing
> > multiple variables. Like:
> > 
> > 	rcu_read_lock();
> > 	rcu_annotate(&var->field);
> > 	rcu_annotate(&var2->field2);
> > 
> > You can then have a special rule that if a particular RCU section has an
> > annotation, any rcu_dereference() not matched will field a warning. If
> > the annotation section is empty, nothing.
> > 
> 
> Good idea! but I don't think annotating a field in C language is easy,
> I will try to see what we can get. Do you have something already in your
> mind?

No, didn't really think about that :-/ The most restrictive version is
taking the absolute address, but that would make things like actual data
structures impossible.



> > > > So I'm still not sure this is useful. Also, I would argue your code has
> > > > problems if you cannot even find your rcu_read_lock().
> > > > 
> > > 
> > > I think what you mean here is, for example, the case where we use
> > > preempt_disable() instead of rcu_read_lock_sched() to pair with
> > > synchronize_sched(), right?
> > 
> > No, I was more like:
> > 
> > 	rcu_read_lock();
> > 	foo()
> > 	  bar()
> > 	    var->func();
> > 	      obj->func();
> > 	        whatever();
> > 
> > and you're looking at a change to whatever() and wonder where the heck
> > the corresponding rcu_read_lock() lives and if we're having it held at
> > all.
> > 
> 
> Confused.. RCU_LOCKED_ACCESS has such information, For example, in the
> piece of /proc/locked_access/rcu I put in the cover letter, which I will
> put in the commit logs for the next version of this series:

Yes, but my point was that if it wasn't obvious from the code, your code
has issues. You should not be needing a tool to figure this out.

[toc] | [prev] | [next] | [standalone]

#1346499

On Tue, Mar 01, 2016 at 10:57:07AM +0100, Peter Zijlstra wrote:
> On Tue, Mar 01, 2016 at 05:32:42PM +0800, Boqun Feng wrote:
> > > One could for example allow something like:
> > > 
> > > 	rcu_read_lock();
> > > 	rcu_annotate(&var->field);
> > > 
> > > 	foo();
> > > 
> > > 	rcu_read_unlock();
> > > 
> > > As an alternative to the syntax suggested by Ingo. This would allow
> > > keeping the existing rcu_read_lock() signature so you don't have to
> > > force update the entire kernel at once, while also (easily) allowing
> > > multiple variables. Like:
> > > 
> > > 	rcu_read_lock();
> > > 	rcu_annotate(&var->field);
> > > 	rcu_annotate(&var2->field2);
> > > 
> > > You can then have a special rule that if a particular RCU section has an
> > > annotation, any rcu_dereference() not matched will field a warning. If
> > > the annotation section is empty, nothing.
> > > 
> > 
> > Good idea! but I don't think annotating a field in C language is easy,
> > I will try to see what we can get. Do you have something already in your
> > mind?
> 
> No, didn't really think about that :-/ The most restrictive version is
> taking the absolute address, but that would make things like actual data
> structures impossible.

So the thing with locks is they get a struct lockdep_map added, in which
we store all kinds of useful. But I don't think we cannot add a similar
structure to each and every RCU dereferencable (is that a word?)
variable.

[toc] | [prev] | [next] | [standalone]

#1347809

[Multipart message — attachments visible in raw view] — view raw

On Tue, Mar 01, 2016 at 11:01:34AM +0100, Peter Zijlstra wrote:
> On Tue, Mar 01, 2016 at 10:57:07AM +0100, Peter Zijlstra wrote:
> > On Tue, Mar 01, 2016 at 05:32:42PM +0800, Boqun Feng wrote:
> > > > One could for example allow something like:
> > > > 
> > > > 	rcu_read_lock();
> > > > 	rcu_annotate(&var->field);
> > > > 
> > > > 	foo();
> > > > 
> > > > 	rcu_read_unlock();
> > > > 
> > > > As an alternative to the syntax suggested by Ingo. This would allow
> > > > keeping the existing rcu_read_lock() signature so you don't have to
> > > > force update the entire kernel at once, while also (easily) allowing
> > > > multiple variables. Like:
> > > > 
> > > > 	rcu_read_lock();
> > > > 	rcu_annotate(&var->field);
> > > > 	rcu_annotate(&var2->field2);
> > > > 
> > > > You can then have a special rule that if a particular RCU section has an
> > > > annotation, any rcu_dereference() not matched will field a warning. If
> > > > the annotation section is empty, nothing.
> > > > 
> > > 
> > > Good idea! but I don't think annotating a field in C language is easy,
> > > I will try to see what we can get. Do you have something already in your
> > > mind?
> > 
> > No, didn't really think about that :-/ The most restrictive version is
> > taking the absolute address, but that would make things like actual data
> > structures impossible.
> 

Another problem of taking the absolute address is the address may change
from rcu_annotate() to rcu_dereference() for example:

	rcu_read_lock();
	rcu_annotate(&var->field);
					// in another thread
					var = new_var;
	
	// the address of var->field is different now.
	rcu_dereference(var->field);
	rcu_read_unlock();


> So the thing with locks is they get a struct lockdep_map added, in which
> we store all kinds of useful. But I don't think we cannot add a similar
> structure to each and every RCU dereferencable (is that a word?)
> variable.
> 

Well, some of them have rcu_head, but not all.. so you're right.

> 

I come up with something you may not like ;-) , which is taking the
strings of the expressions, for example:

	rcu_read_lock();
	rcu_annotate(var->field);	// "var->field" is added for 
					// the current section

	rcu_dereference(var->field);	// OK, because the expression
					// "var->field" is annotated.

	rcu_dereference(var->field2);	// Not OK, because the
					// expression "var->field2" is
					// not annotated, nor is any of
					// its suffix.

	rcu_annotate(field3);		// "field3" is added for the
					// current section
	
	rcu_dereference(var2->field3);	// OK, because the suffix
					// "field3" is annotated.
	rcu_read_unlock();

I think this is more accurate than taking the absolute address because
the address changing situations exist. So.. thoughts?

Regards,
Boqun

[toc] | [prev] | [next] | [standalone]

#1347923

On Wed, Mar 02, 2016 at 02:37:18PM +0800, Boqun Feng wrote:
> I come up with something you may not like ;-) , which is taking the
> strings of the expressions, for example:

Heh, cute. Better than nothing I suppose.

[toc] | [prev] | [next] | [standalone]

#1348055

On Wed, Mar 02, 2016 at 11:18:23AM +0100, Peter Zijlstra wrote:
> On Wed, Mar 02, 2016 at 02:37:18PM +0800, Boqun Feng wrote:
> > I come up with something you may not like ;-) , which is taking the
> > strings of the expressions, for example:
> 
> Heh, cute. Better than nothing I suppose.

It would be very good to get feedback from people using RCU heavily.
Some RCU read-side critical sections access a lot of fields, which
might make this sort of notation a bit of a maintainability issue.

							Thanx, Paul

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web