| Message ID | 1415992771-9071-1-git-send-email-elder@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, 14 Nov 2014 13:19:31 -0600 Alex Elder <elder@linaro.org> wrote: > Pranith Kumar posted a patch in which removed the "volatile" > qualifier for the "logbuf_cpu" variable in vprintk_emit(). > https://lkml.org/lkml/2014/11/13/894 > In his patch, he used ACCESS_ONCE() for all references to > that symbol to provide whatever protection was intended. > > There was some discussion that followed, and in the end > Stephen Rostedt concluded that not only was "volatile" not I don't know of a Stephen Rostedt. J > needed, neither was it required to use ACCESS_ONCE(). I > offered an elaborate description that concluded Stephen Who is this Stephen you talk about? > was right, and Pranith asked me to submit an alternative > patch. And this is it. > > The basic reason "volatile" is not needed is that "logbuf_cpu" has > static storage duration, and vprintk_emit() is an exported > interface. This means that the value of logbuf_cpu must be read > from memory the first time it is used in a particular call of > vprintk_emit(). The variable's value is read only once in that > function, when it's read it'll be the copy from memory (or cache). > > In addition, the value of "logbuf_cpu" is only ever written under > protection of a spinlock. So the value that is read is the "real" > value (and not an out-of-date cached one). If its value is not > UINT_MAX, it is the current CPU's processor id, and it will have > been last written by the running CPU. > > Reported-by: Pranith Kumar <bobby.prani@gmail.com> > Fix-suggested-by: Steven Rostedt <rostedt@goodmis.org> Ah, I know of him! > Signed-off-by: Alex Elder <elder@linaro.org> > --- > Note!!! I am not able to stress test this patch. I know the guys at SuSE are stressing printk a bit. Maybe they can come up with something. (Cc'd). Thanks, -- Steve > > kernel/printk/printk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index ced2b84..fefc8d0 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -1627,7 +1627,7 @@ asmlinkage int vprintk_emit(int facility, int level, > int printed_len = 0; > bool in_sched = false; > /* cpu currently holding logbuf_lock in this function */ > - static volatile unsigned int logbuf_cpu = UINT_MAX; > + static unsigned int logbuf_cpu = UINT_MAX; > > if (level == SCHED_MESSAGE_LOGLEVEL) { > level = -1; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 11/14/2014 01:45 PM, Steven Rostedt wrote: > On Fri, 14 Nov 2014 13:19:31 -0600 > Alex Elder <elder@linaro.org> wrote: > >> Pranith Kumar posted a patch in which removed the "volatile" >> qualifier for the "logbuf_cpu" variable in vprintk_emit(). >> https://lkml.org/lkml/2014/11/13/894 >> In his patch, he used ACCESS_ONCE() for all references to >> that symbol to provide whatever protection was intended. >> >> There was some discussion that followed, and in the end >> Stephen Rostedt concluded that not only was "volatile" not > > I don't know of a Stephen Rostedt. J Sorry. I'm sure that's a source of annoyance. -Alex >> needed, neither was it required to use ACCESS_ONCE(). I >> offered an elaborate description that concluded Stephen > > Who is this Stephen you talk about? . . . -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Fri, 14 Nov 2014 13:46:44 -0600 Alex Elder <elder@linaro.org> wrote: > On 11/14/2014 01:45 PM, Steven Rostedt wrote: > > On Fri, 14 Nov 2014 13:19:31 -0600 > > Alex Elder <elder@linaro.org> wrote: > > > >> Pranith Kumar posted a patch in which removed the "volatile" > >> qualifier for the "logbuf_cpu" variable in vprintk_emit(). > >> https://lkml.org/lkml/2014/11/13/894 > >> In his patch, he used ACCESS_ONCE() for all references to > >> that symbol to provide whatever protection was intended. > >> > >> There was some discussion that followed, and in the end > >> Stephen Rostedt concluded that not only was "volatile" not > > > > I don't know of a Stephen Rostedt. J > > Sorry. I'm sure that's a source of annoyance. -Alex No problem. If you are wondering about that "J". https://www.facebook.com/mcgrof/posts/10104459749108919 It's my new thing. Maybe I can start a trend. J -- Steve > > >> needed, neither was it required to use ACCESS_ONCE(). I > >> offered an elaborate description that concluded Stephen > > > > Who is this Stephen you talk about? > > . . . -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 11/14/2014 01:55 PM, Steven Rostedt wrote: > On Fri, 14 Nov 2014 13:46:44 -0600 > Alex Elder <elder@linaro.org> wrote: > >> On 11/14/2014 01:45 PM, Steven Rostedt wrote: >>> On Fri, 14 Nov 2014 13:19:31 -0600 >>> Alex Elder <elder@linaro.org> wrote: >>> >>>> Pranith Kumar posted a patch in which removed the "volatile" >>>> qualifier for the "logbuf_cpu" variable in vprintk_emit(). >>>> https://lkml.org/lkml/2014/11/13/894 >>>> In his patch, he used ACCESS_ONCE() for all references to >>>> that symbol to provide whatever protection was intended. >>>> >>>> There was some discussion that followed, and in the end >>>> Stephen Rostedt concluded that not only was "volatile" not >>> >>> I don't know of a Stephen Rostedt. J >> >> Sorry. I'm sure that's a source of annoyance. -Alex > > No problem. If you are wondering about that "J". Yes, as a matter of fact, I did wonder. > https://www.facebook.com/mcgrof/posts/10104459749108919 > > It's my new thing. Maybe I can start a trend. J I'm not big on smileys. L But I'll remain neutral in this case. K -Alex > -- Steve > >> >>>> needed, neither was it required to use ACCESS_ONCE(). I >>>> offered an elaborate description that concluded Stephen >>> >>> Who is this Stephen you talk about? >> >> . . . > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Fri 14-11-14 13:19:31, Alex Elder wrote: > Pranith Kumar posted a patch in which removed the "volatile" > qualifier for the "logbuf_cpu" variable in vprintk_emit(). > https://lkml.org/lkml/2014/11/13/894 > In his patch, he used ACCESS_ONCE() for all references to > that symbol to provide whatever protection was intended. > > There was some discussion that followed, and in the end > Stephen Rostedt concluded that not only was "volatile" not > needed, neither was it required to use ACCESS_ONCE(). I > offered an elaborate description that concluded Stephen > was right, and Pranith asked me to submit an alternative > patch. And this is it. > > The basic reason "volatile" is not needed is that "logbuf_cpu" has > static storage duration, and vprintk_emit() is an exported > interface. This means that the value of logbuf_cpu must be read > from memory the first time it is used in a particular call of > vprintk_emit(). The variable's value is read only once in that > function, when it's read it'll be the copy from memory (or cache). > > In addition, the value of "logbuf_cpu" is only ever written under > protection of a spinlock. So the value that is read is the "real" > value (and not an out-of-date cached one). If its value is not > UINT_MAX, it is the current CPU's processor id, and it will have > been last written by the running CPU. > > Reported-by: Pranith Kumar <bobby.prani@gmail.com> > Fix-suggested-by: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Alex Elder <elder@linaro.org> > --- > Note!!! I am not able to stress test this patch. The patch looks good to me. You can add: Reviewed-by: Jan Kara <jack@suse.cz> BTW, stress-testing this is tough. I never saw printk recursion happening in practice - only when I screwed up something in printk code when experimenting. Honza > kernel/printk/printk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index ced2b84..fefc8d0 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -1627,7 +1627,7 @@ asmlinkage int vprintk_emit(int facility, int level, > int printed_len = 0; > bool in_sched = false; > /* cpu currently holding logbuf_lock in this function */ > - static volatile unsigned int logbuf_cpu = UINT_MAX; > + static unsigned int logbuf_cpu = UINT_MAX; > > if (level == SCHED_MESSAGE_LOGLEVEL) { > level = -1; > -- > 1.9.1 >
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index ced2b84..fefc8d0 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -1627,7 +1627,7 @@ asmlinkage int vprintk_emit(int facility, int level, int printed_len = 0; bool in_sched = false; /* cpu currently holding logbuf_lock in this function */ - static volatile unsigned int logbuf_cpu = UINT_MAX; + static unsigned int logbuf_cpu = UINT_MAX; if (level == SCHED_MESSAGE_LOGLEVEL) { level = -1;
Pranith Kumar posted a patch in which removed the "volatile" qualifier for the "logbuf_cpu" variable in vprintk_emit(). https://lkml.org/lkml/2014/11/13/894 In his patch, he used ACCESS_ONCE() for all references to that symbol to provide whatever protection was intended. There was some discussion that followed, and in the end Stephen Rostedt concluded that not only was "volatile" not needed, neither was it required to use ACCESS_ONCE(). I offered an elaborate description that concluded Stephen was right, and Pranith asked me to submit an alternative patch. And this is it. The basic reason "volatile" is not needed is that "logbuf_cpu" has static storage duration, and vprintk_emit() is an exported interface. This means that the value of logbuf_cpu must be read from memory the first time it is used in a particular call of vprintk_emit(). The variable's value is read only once in that function, when it's read it'll be the copy from memory (or cache). In addition, the value of "logbuf_cpu" is only ever written under protection of a spinlock. So the value that is read is the "real" value (and not an out-of-date cached one). If its value is not UINT_MAX, it is the current CPU's processor id, and it will have been last written by the running CPU. Reported-by: Pranith Kumar <bobby.prani@gmail.com> Fix-suggested-by: Steven Rostedt <rostedt@goodmis.org> Signed-off-by: Alex Elder <elder@linaro.org> --- Note!!! I am not able to stress test this patch. kernel/printk/printk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)