Re: [PATCH 05/48] percpu: Add preemption checks to __this_cpu ops
On Tue, 4 Mar 2014 21:27:14 -0600 (CST) Christoph Lameter
wrote:
> > > +notrace void __this_cpu_preempt_check(const char *op)
> > > +{
> > > + char text[40];
> > > +
> > > + snprintf(text, sizeof(text), "__this_cpu_%s()", op);
> > > + check_preemption_disabled(text);
> > > +}
> >
> > I'd like to see a comment here telling scared readers why this can
> > never overflow text[].
>
> Ok. I can also add VM_BUG_ON(strlen(op) >= sizeof(text)) ?
I misread the code - snprintf() will dtrt and we'll just end up with
truncated debug text. Not worth worrying about.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 05/48] percpu: Add preemption checks to __this_cpu ops
On Tue, 4 Mar 2014 21:27:14 -0600 (CST) Christoph Lameter c...@linux.com
wrote:
+notrace void __this_cpu_preempt_check(const char *op)
+{
+ char text[40];
+
+ snprintf(text, sizeof(text), __this_cpu_%s(), op);
+ check_preemption_disabled(text);
+}
I'd like to see a comment here telling scared readers why this can
never overflow text[].
Ok. I can also add VM_BUG_ON(strlen(op) = sizeof(text)) ?
I misread the code - snprintf() will dtrt and we'll just end up with
truncated debug text. Not worth worrying about.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 05/48] percpu: Add preemption checks to __this_cpu ops
On Tue, 4 Mar 2014, Andrew Morton wrote:
> > print_symbol("caller is %s\n", (long)__builtin_return_address(0));
> > dump_stack();
>
> I wonder if there's any point in printing __builtin_return_address.
> Doesn't dump_stack() tell us the same thing?
Yes it does. However, it was there before and software may scan the logs
for it.
> > +notrace void __this_cpu_preempt_check(const char *op)
> > +{
> > + char text[40];
> > +
> > + snprintf(text, sizeof(text), "__this_cpu_%s()", op);
> > + check_preemption_disabled(text);
> > +}
>
> I'd like to see a comment here telling scared readers why this can
> never overflow text[].
Ok. I can also add VM_BUG_ON(strlen(op) >= sizeof(text)) ?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 05/48] percpu: Add preemption checks to __this_cpu ops
On Tue, 4 Mar 2014 14:27:27 -0800
Andrew Morton wrote:
> On Fri, 14 Feb 2014 14:18:46 -0600 Christoph Lameter wrote:
>
> > [Patch depends on another patch in this series that introduces raw_cpu_ops]
> >
> > We define a check function in order to avoid trouble with the
> > include files. Then the higher level __this_cpu macros are
> > modified to invoke the preemption check.
> >
> > --- linux.orig/lib/smp_processor_id.c 2014-01-30 14:40:50.936519233
> > -0600
> > +++ linux/lib/smp_processor_id.c2014-01-30 14:40:50.936519233 -0600
> > @@ -7,7 +7,7 @@
> > #include
> > #include
> >
> > -notrace unsigned int debug_smp_processor_id(void)
> > +notrace static unsigned int check_preemption_disabled(char *what)
> > {
> > int this_cpu = raw_smp_processor_id();
> >
> > @@ -38,9 +38,9 @@
> > if (!printk_ratelimit())
> > goto out_enable;
> >
> > - printk(KERN_ERR "BUG: using smp_processor_id() in preemptible [%08x] "
> > - "code: %s/%d\n",
> > - preempt_count() - 1, current->comm, current->pid);
> > + printk(KERN_ERR "BUG: using %s in preemptible [%08x] code: %s/%d\n",
> > + what, preempt_count() - 1, current->comm, current->pid);
> > +
> > print_symbol("caller is %s\n", (long)__builtin_return_address(0));
> > dump_stack();
>
> I wonder if there's any point in printing __builtin_return_address.
> Doesn't dump_stack() tell us the same thing?
When frame pointers are enabled, sure. But without frame pointers, I'm
not so sure.
-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 05/48] percpu: Add preemption checks to __this_cpu ops
On Fri, 14 Feb 2014 14:18:46 -0600 Christoph Lameter wrote:
> [Patch depends on another patch in this series that introduces raw_cpu_ops]
>
> We define a check function in order to avoid trouble with the
> include files. Then the higher level __this_cpu macros are
> modified to invoke the preemption check.
>
> --- linux.orig/lib/smp_processor_id.c 2014-01-30 14:40:50.936519233 -0600
> +++ linux/lib/smp_processor_id.c 2014-01-30 14:40:50.936519233 -0600
> @@ -7,7 +7,7 @@
> #include
> #include
>
> -notrace unsigned int debug_smp_processor_id(void)
> +notrace static unsigned int check_preemption_disabled(char *what)
> {
> int this_cpu = raw_smp_processor_id();
>
> @@ -38,9 +38,9 @@
> if (!printk_ratelimit())
> goto out_enable;
>
> - printk(KERN_ERR "BUG: using smp_processor_id() in preemptible [%08x] "
> - "code: %s/%d\n",
> - preempt_count() - 1, current->comm, current->pid);
> + printk(KERN_ERR "BUG: using %s in preemptible [%08x] code: %s/%d\n",
> + what, preempt_count() - 1, current->comm, current->pid);
> +
> print_symbol("caller is %s\n", (long)__builtin_return_address(0));
> dump_stack();
I wonder if there's any point in printing __builtin_return_address.
Doesn't dump_stack() tell us the same thing?
> @@ -50,5 +50,17 @@
> return this_cpu;
> }
>
> +notrace unsigned int debug_smp_processor_id(void)
> +{
> + return check_preemption_disabled("smp_processor_id()");
> +}
> EXPORT_SYMBOL(debug_smp_processor_id);
>
> +notrace void __this_cpu_preempt_check(const char *op)
> +{
> + char text[40];
> +
> + snprintf(text, sizeof(text), "__this_cpu_%s()", op);
> + check_preemption_disabled(text);
> +}
I'd like to see a comment here telling scared readers why this can
never overflow text[].
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 05/48] percpu: Add preemption checks to __this_cpu ops
On Fri, 14 Feb 2014 14:18:46 -0600 Christoph Lameter c...@linux.com wrote:
[Patch depends on another patch in this series that introduces raw_cpu_ops]
We define a check function in order to avoid trouble with the
include files. Then the higher level __this_cpu macros are
modified to invoke the preemption check.
--- linux.orig/lib/smp_processor_id.c 2014-01-30 14:40:50.936519233 -0600
+++ linux/lib/smp_processor_id.c 2014-01-30 14:40:50.936519233 -0600
@@ -7,7 +7,7 @@
#include linux/kallsyms.h
#include linux/sched.h
-notrace unsigned int debug_smp_processor_id(void)
+notrace static unsigned int check_preemption_disabled(char *what)
{
int this_cpu = raw_smp_processor_id();
@@ -38,9 +38,9 @@
if (!printk_ratelimit())
goto out_enable;
- printk(KERN_ERR BUG: using smp_processor_id() in preemptible [%08x]
- code: %s/%d\n,
- preempt_count() - 1, current-comm, current-pid);
+ printk(KERN_ERR BUG: using %s in preemptible [%08x] code: %s/%d\n,
+ what, preempt_count() - 1, current-comm, current-pid);
+
print_symbol(caller is %s\n, (long)__builtin_return_address(0));
dump_stack();
I wonder if there's any point in printing __builtin_return_address.
Doesn't dump_stack() tell us the same thing?
@@ -50,5 +50,17 @@
return this_cpu;
}
+notrace unsigned int debug_smp_processor_id(void)
+{
+ return check_preemption_disabled(smp_processor_id());
+}
EXPORT_SYMBOL(debug_smp_processor_id);
+notrace void __this_cpu_preempt_check(const char *op)
+{
+ char text[40];
+
+ snprintf(text, sizeof(text), __this_cpu_%s(), op);
+ check_preemption_disabled(text);
+}
I'd like to see a comment here telling scared readers why this can
never overflow text[].
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 05/48] percpu: Add preemption checks to __this_cpu ops
On Tue, 4 Mar 2014 14:27:27 -0800
Andrew Morton a...@linux-foundation.org wrote:
On Fri, 14 Feb 2014 14:18:46 -0600 Christoph Lameter c...@linux.com wrote:
[Patch depends on another patch in this series that introduces raw_cpu_ops]
We define a check function in order to avoid trouble with the
include files. Then the higher level __this_cpu macros are
modified to invoke the preemption check.
--- linux.orig/lib/smp_processor_id.c 2014-01-30 14:40:50.936519233
-0600
+++ linux/lib/smp_processor_id.c2014-01-30 14:40:50.936519233 -0600
@@ -7,7 +7,7 @@
#include linux/kallsyms.h
#include linux/sched.h
-notrace unsigned int debug_smp_processor_id(void)
+notrace static unsigned int check_preemption_disabled(char *what)
{
int this_cpu = raw_smp_processor_id();
@@ -38,9 +38,9 @@
if (!printk_ratelimit())
goto out_enable;
- printk(KERN_ERR BUG: using smp_processor_id() in preemptible [%08x]
- code: %s/%d\n,
- preempt_count() - 1, current-comm, current-pid);
+ printk(KERN_ERR BUG: using %s in preemptible [%08x] code: %s/%d\n,
+ what, preempt_count() - 1, current-comm, current-pid);
+
print_symbol(caller is %s\n, (long)__builtin_return_address(0));
dump_stack();
I wonder if there's any point in printing __builtin_return_address.
Doesn't dump_stack() tell us the same thing?
When frame pointers are enabled, sure. But without frame pointers, I'm
not so sure.
-- Steve
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 05/48] percpu: Add preemption checks to __this_cpu ops
On Tue, 4 Mar 2014, Andrew Morton wrote:
print_symbol(caller is %s\n, (long)__builtin_return_address(0));
dump_stack();
I wonder if there's any point in printing __builtin_return_address.
Doesn't dump_stack() tell us the same thing?
Yes it does. However, it was there before and software may scan the logs
for it.
+notrace void __this_cpu_preempt_check(const char *op)
+{
+ char text[40];
+
+ snprintf(text, sizeof(text), __this_cpu_%s(), op);
+ check_preemption_disabled(text);
+}
I'd like to see a comment here telling scared readers why this can
never overflow text[].
Ok. I can also add VM_BUG_ON(strlen(op) = sizeof(text)) ?
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
