1) spinlock held is not equivalent to rcu_read_lock()
2) remove the stale signal code snippet
Signed-off-by: Thomas Gleixner <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Vegard Nossum <[email protected]>
Cc: James Morris <[email protected]>
---
Documentation/credentials.txt | 3 ---
Documentation/kmemcheck.txt | 3 +--
2 files changed, 1 insertion(+), 5 deletions(-)
Index: linux-2.6-tip/Documentation/credentials.txt
===================================================================
--- linux-2.6-tip.orig/Documentation/credentials.txt
+++ linux-2.6-tip/Documentation/credentials.txt
@@ -408,9 +408,6 @@ This should be used inside the RCU read
...
}
-A function need not get RCU read lock to use __task_cred() if it is holding a
-spinlock at the time as this implicitly holds the RCU read lock.
-
Should it be necessary to hold another task's credentials for a long period of
time, and possibly to sleep whilst doing so, then the caller should get a
reference on them using:
Index: linux-2.6-tip/Documentation/kmemcheck.txt
===================================================================
--- linux-2.6-tip.orig/Documentation/kmemcheck.txt
+++ linux-2.6-tip/Documentation/kmemcheck.txt
@@ -429,8 +429,7 @@ Let's take a look at it:
193 /*
194 * We won't get problems with the target's UID changing under us
195 * because changing it requires RCU be used, and if t != current, the
-196 * caller must be holding the RCU readlock (by way of a spinlock) and
-197 * we use RCU protection here
+196 * caller must be holding the RCU readlocke
198 */
199 user = get_uid(__task_cred(t)->user);
200 atomic_inc(&user->sigpending);
[trimmed Cc]
>2) remove the stale signal code snippet
...
2009/12/10 Thomas Gleixner <[email protected]>:
> Index: linux-2.6-tip/Documentation/kmemcheck.txt
> ===================================================================
> --- linux-2.6-tip.orig/Documentation/kmemcheck.txt
> +++ linux-2.6-tip/Documentation/kmemcheck.txt
> @@ -429,8 +429,7 @@ Let's take a look at it:
> 193 /*
> 194 * We won't get problems with the target's UID changing under us
> 195 * because changing it requires RCU be used, and if t != current, the
> -196 * caller must be holding the RCU readlock (by way of a spinlock) and
> -197 * we use RCU protection here
> +196 * caller must be holding the RCU readlocke
> 198 */
> 199 user = get_uid(__task_cred(t)->user);
> 200 atomic_inc(&user->sigpending);
I am not sure that I really agree with this change. This is not a code
example for the sake of showing how to do a particular thing, it's an
example of real code from the tree.
I don't remember if the document is referring to a particular git
version of the code, but I think it might not, in which case it
doesn't REALLY matter even on the microscopic level.
But I won't make a big fuss about it :-)
Vegard
PS: Upon closer inspection, I noticed that one line (line 197) goes
completely missing, there seems to be a typo there too, "readlocke".
Still it's not a huge deal, I admit.
Thomas Gleixner <[email protected]> wrote:
> -197 * we use RCU protection here
> +196 * caller must be holding the RCU readlocke
You mean "readlock" I suspect.
Otherwise, feel free to add:
Acked-by: David Howells <[email protected]>
On Fri, 11 Dec 2009, David Howells wrote:
> Thomas Gleixner <[email protected]> wrote:
>
> > -197 * we use RCU protection here
> > +196 * caller must be holding the RCU readlocke
>
> You mean "readlock" I suspect.
Or maybe he's talking about ye olde readlocke, used widely for OS research
throughout the middle ages. You still find that spelling in some really
old CS literature.
Linus
On Fri, Dec 11, 2009 at 08:07:45AM -0800, Linus Torvalds wrote:
>
>
> On Fri, 11 Dec 2009, David Howells wrote:
>
> > Thomas Gleixner <[email protected]> wrote:
> >
> > > -197 * we use RCU protection here
> > > +196 * caller must be holding the RCU readlocke
> >
> > You mean "readlock" I suspect.
>
> Or maybe he's talking about ye olde readlocke, used widely for OS research
> throughout the middle ages. You still find that spelling in some really
> old CS literature.
Interestingly enough, they also tended to split it into two words and
capitalize it, as can be seen by searching for "Read Locke" at
http://faculty.uml.edu/enelson/modern07.htm
Thanx, Paul
On Fri, 11 Dec 2009, Paul E. McKenney wrote:
> On Fri, Dec 11, 2009 at 08:07:45AM -0800, Linus Torvalds wrote:
> >
> >
> > On Fri, 11 Dec 2009, David Howells wrote:
> >
> > > Thomas Gleixner <[email protected]> wrote:
> > >
> > > > -197 * we use RCU protection here
> > > > +196 * caller must be holding the RCU readlocke
> > >
> > > You mean "readlock" I suspect.
> >
> > Or maybe he's talking about ye olde readlocke, used widely for OS research
> > throughout the middle ages. You still find that spelling in some really
> > old CS literature.
>
> Interestingly enough, they also tended to split it into two words and
> capitalize it, as can be seen by searching for "Read Locke" at
>
> http://faculty.uml.edu/enelson/modern07.htm
The good thing about "Read Locke" is: there is no "Write Locke".
Thanks,
tglx
On Thursday 10 December 2009 00:53:26 Thomas Gleixner wrote:
> Index: linux-2.6-tip/Documentation/credentials.txt
> ===================================================================
> --- linux-2.6-tip.orig/Documentation/credentials.txt
> +++ linux-2.6-tip/Documentation/credentials.txt
> @@ -408,9 +408,6 @@ This should be used inside the RCU read
> ...
> }
>
> -A function need not get RCU read lock to use __task_cred() if it is holding a
> -spinlock at the time as this implicitly holds the RCU read lock.
> -
> Should it be necessary to hold another task's credentials for a long period of
> time, and possibly to sleep whilst doing so, then the caller should get a
> reference on them using:
How about changing the documentation to explain why you can't just use a spinlock
or local_irq_disable instead of rcu_read_lock? You explained it well in your
[patch 0/9], but that part had not occurred to me yet and having it in the kernel
sources might prevent more people from getting it wrong in the future.
Arnd
On Fri, Dec 11, 2009 at 09:28:25PM +0000, Arnd Bergmann wrote:
> On Thursday 10 December 2009 00:53:26 Thomas Gleixner wrote:
> > Index: linux-2.6-tip/Documentation/credentials.txt
> > ===================================================================
> > --- linux-2.6-tip.orig/Documentation/credentials.txt
> > +++ linux-2.6-tip/Documentation/credentials.txt
> > @@ -408,9 +408,6 @@ This should be used inside the RCU read
> > ...
> > }
> >
> > -A function need not get RCU read lock to use __task_cred() if it is holding a
> > -spinlock at the time as this implicitly holds the RCU read lock.
> > -
> > Should it be necessary to hold another task's credentials for a long period of
> > time, and possibly to sleep whilst doing so, then the caller should get a
> > reference on them using:
>
> How about changing the documentation to explain why you can't just use a spinlock
> or local_irq_disable instead of rcu_read_lock? You explained it well in your
> [patch 0/9], but that part had not occurred to me yet and having it in the kernel
> sources might prevent more people from getting it wrong in the future.
That does make a lot of sense... Will add that in.
Thanx, Paul