Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

[RFC][PATCH] printk: Add %pb to print bitmaps

 

 

Linux kernel RSS feed   Index | Next | Previous | View Threaded


peterz at infradead

May 9, 2012, 6:27 AM

Post #1 of 24 (265 views)
Permalink
[RFC][PATCH] printk: Add %pb to print bitmaps

On Wed, 2012-05-09 at 14:30 +0200, Peter Zijlstra wrote:
> on a related note, we should add a printk-%p modifier for cpulist,
> this cpulist_scnprintf() stuff gets annoying.

Something like so I guess.. I'll try and convert some of the sched_debug
code to test it..

---
lib/vsprintf.c | 36 +++++++++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index abbabec..90bfcd1 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -25,6 +25,9 @@
#include <linux/kallsyms.h>
#include <linux/uaccess.h>
#include <linux/ioport.h>
+#include <linux/bitmap.h>
+#include <linux/cpumask.h>
+#include <linux/numa.h>
#include <net/addrconf.h>

#include <asm/page.h> /* for PAGE_SIZE */
@@ -857,15 +860,19 @@ int kptr_restrict __read_mostly;
* correctness of the format string and va_list arguments.
* - 'K' For a kernel pointer that should be hidden from unprivileged users
* - 'NF' For a netdev_features_t
+ * - 'b' For a bitmap, consumes 2 args, second is int
+ * - 'bc' For a cpumask
+ * - 'bn' For a nodemask
*
* Note: The difference between 'S' and 'F' is that on ia64 and ppc64
* function pointers are really function descriptors, which contain a
* pointer to the real address.
*/
-static noinline_for_stack
-char *pointer(const char *fmt, char *buf, char *end, void *ptr,
- struct printf_spec spec)
+static noinline_for_stack
+char *pointer(const char *fmt, char *buf, char *end, va_list args, struct printf_spec spec)
{
+ void *ptr = va_arg(args, void *);
+
if (!ptr && *fmt != 'K') {
/*
* Print (null) with the same width as a pointer so it makes
@@ -941,6 +948,26 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
return netdev_feature_string(buf, end, ptr, spec);
}
break;
+ case 'b':
+ {
+ int bits, len;
+
+ switch (fmt[1]) {
+ case 'c':
+ bits = nr_cpumask_bits;
+ break;
+ case 'n':
+ bits = MAX_NUMNODES;
+ break;
+ default:
+ bits = va_arg(args, int);
+ break;
+ }
+
+ len = bitmap_scnlistprintf(buf, end - buf, ptr, bits);
+
+ return buf + len;
+ }
}
spec.flags |= SMALL;
if (spec.field_width == -1) {
@@ -1261,8 +1288,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
break;

case FORMAT_TYPE_PTR:
- str = pointer(fmt+1, str, end, va_arg(args, void *),
- spec);
+ str = pointer(fmt+1, str, end, args, spec);
while (isalnum(*fmt))
fmt++;
break;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


peterz at infradead

May 9, 2012, 6:29 AM

Post #2 of 24 (266 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 2012-05-09 at 15:27 +0200, Peter Zijlstra wrote:
> +char *pointer(const char *fmt, char *buf, char *end, va_list args,
> struct printf_spec spec)

I'd better make that a va_list *, otherwise the iteration gets lost.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at kernel

May 9, 2012, 6:36 AM

Post #3 of 24 (262 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

* Peter Zijlstra <peterz [at] infradead> wrote:

> @@ -857,15 +860,19 @@ int kptr_restrict __read_mostly;
> * correctness of the format string and va_list arguments.
> * - 'K' For a kernel pointer that should be hidden from unprivileged users
> * - 'NF' For a netdev_features_t
> + * - 'b' For a bitmap, consumes 2 args, second is int

hm, won't the second arg confuse gcc's printf format checker?

> + case 'b':
> + {
> + int bits, len;
> +
> + switch (fmt[1]) {
> + case 'c':
> + bits = nr_cpumask_bits;
> + break;
> + case 'n':
> + bits = MAX_NUMNODES;
> + break;
> + default:
> + bits = va_arg(args, int);
> + break;
> + }
> +
> + len = bitmap_scnlistprintf(buf, end - buf, ptr, bits);

also, if the second argument is not provided, could va_arg()
return noise or even NULL?

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


peterz at infradead

May 9, 2012, 6:44 AM

Post #4 of 24 (263 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 2012-05-09 at 15:36 +0200, Ingo Molnar wrote:
> * Peter Zijlstra <peterz [at] infradead> wrote:
>
> > @@ -857,15 +860,19 @@ int kptr_restrict __read_mostly;
> > * correctness of the format string and va_list arguments.
> > * - 'K' For a kernel pointer that should be hidden from unprivileged users
> > * - 'NF' For a netdev_features_t
> > + * - 'b' For a bitmap, consumes 2 args, second is int
>
> hm, won't the second arg confuse gcc's printf format checker?


Ah, yes, I suppose I could abuse something like %*pb. Let me try that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


peterz at infradead

May 9, 2012, 6:59 AM

Post #5 of 24 (260 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 2012-05-09 at 15:44 +0200, Peter Zijlstra wrote:
> On Wed, 2012-05-09 at 15:36 +0200, Ingo Molnar wrote:
> > * Peter Zijlstra <peterz [at] infradead> wrote:
> >
> > > @@ -857,15 +860,19 @@ int kptr_restrict __read_mostly;
> > > * correctness of the format string and va_list arguments.
> > > * - 'K' For a kernel pointer that should be hidden from unprivileged users
> > > * - 'NF' For a netdev_features_t
> > > + * - 'b' For a bitmap, consumes 2 args, second is int
> >
> > hm, won't the second arg confuse gcc's printf format checker?
>
>
> Ah, yes, I suppose I could abuse something like %*pb. Let me try that.

I guess I should use %.*pb and keep the field_width in case someone
manages to actually make bitmap_scnlistprintf() conform to it. The
precision is unused anyway.

The current implementation limits both (field_width and precision) to
s16, which will limit us to printing 32Kb bitmaps. If need arises we
could increase printf_spec size I guess.

---
lib/vsprintf.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index abbabec..6eb30a9 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -25,6 +25,9 @@
#include <linux/kallsyms.h>
#include <linux/uaccess.h>
#include <linux/ioport.h>
+#include <linux/bitmap.h>
+#include <linux/cpumask.h>
+#include <linux/numa.h>
#include <net/addrconf.h>

#include <asm/page.h> /* for PAGE_SIZE */
@@ -857,6 +860,9 @@ int kptr_restrict __read_mostly;
* correctness of the format string and va_list arguments.
* - 'K' For a kernel pointer that should be hidden from unprivileged users
* - 'NF' For a netdev_features_t
+ * - 'b' For a bitmap; %.*pb is required and the precision is used as bitmap length
+ * - 'bc' For a cpumask
+ * - 'bn' For a nodemask
*
* Note: The difference between 'S' and 'F' is that on ia64 and ppc64
* function pointers are really function descriptors, which contain a
@@ -941,6 +947,26 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
return netdev_feature_string(buf, end, ptr, spec);
}
break;
+ case 'b':
+ {
+ int bits, len;
+
+ switch (fmt[1]) {
+ case 'c':
+ bits = nr_cpumask_bits;
+ break;
+ case 'n':
+ bits = MAX_NUMNODES;
+ break;
+ default:
+ bits = spec->precision;
+ break;
+ }
+
+ len = bitmap_scnlistprintf(buf, end - buf, ptr, bits);
+
+ return buf + len;
+ }
}
spec.flags |= SMALL;
if (spec.field_width == -1) {
@@ -1175,6 +1201,9 @@ int format_decode(const char *fmt, struct printf_spec *spec)
* %pI6c print an IPv6 address as specified by RFC 5952
* %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
* case.
+ * %.*pb print a bitmap list using the precision as bitmap length
+ * %pbc print a cpumask bitmap
+ * %pbn print a nodemask bitmap
* %n is ignored
*
* The return value is the number of characters which would

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at kernel

May 9, 2012, 7:15 AM

Post #6 of 24 (264 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

* Peter Zijlstra <peterz [at] infradead> wrote:

> On Wed, 2012-05-09 at 15:44 +0200, Peter Zijlstra wrote:
> > On Wed, 2012-05-09 at 15:36 +0200, Ingo Molnar wrote:
> > > * Peter Zijlstra <peterz [at] infradead> wrote:
> > >
> > > > @@ -857,15 +860,19 @@ int kptr_restrict __read_mostly;
> > > > * correctness of the format string and va_list arguments.
> > > > * - 'K' For a kernel pointer that should be hidden from unprivileged users
> > > > * - 'NF' For a netdev_features_t
> > > > + * - 'b' For a bitmap, consumes 2 args, second is int
> > >
> > > hm, won't the second arg confuse gcc's printf format checker?
> >
> >
> > Ah, yes, I suppose I could abuse something like %*pb. Let me try that.
>
> I guess I should use %.*pb and keep the field_width in case someone
> manages to actually make bitmap_scnlistprintf() conform to it. The
> precision is unused anyway.

That's a cute trick, and it's intuitive as well.

> + case 'b':
> + {
> + int bits, len;
> +
> + switch (fmt[1]) {
> + case 'c':
> + bits = nr_cpumask_bits;
> + break;
> + case 'n':
> + bits = MAX_NUMNODES;
> + break;
> + default:
> + bits = spec->precision;
> + break;

So, if someone specifies an incomplete "%pb" format - fmt[1]
will be 0 and we take precision as the length - presumably also
0. We stick that 0 into:

> + len = bitmap_scnlistprintf(buf, end - buf, ptr, bits);

Will that work?

Provided it all tests out fine for you it looks good to me.

I'd probably write the switch statement as:

switch (fmt[1]) {
case 'c': bits = nr_cpumask_bits; break;
case 'n': bits = MAX_NUMNODES; break;
default: bits = spec->precision; break;
}

... but that's a small detail and a matter of taste in any case.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


joe at perches

May 9, 2012, 7:19 AM

Post #7 of 24 (263 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 2012-05-09 at 15:59 +0200, Peter Zijlstra wrote:
> The current implementation limits both (field_width and precision) to
> s16, which will limit us to printing 32Kb bitmaps. If need arises we
> could increase printf_spec size I guess.

Not sure that's a problem really.
A single printk is limited to 1024 bytes.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -941,6 +947,26 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> return netdev_feature_string(buf, end, ptr, spec);
> }
> break;
> + case 'b':
> + {
> + int bits, len;

trivia: a tab indent could be avoided by using
case b: {


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


peterz at infradead

May 9, 2012, 7:24 AM

Post #8 of 24 (264 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 2012-05-09 at 16:15 +0200, Ingo Molnar wrote:
> So, if someone specifies an incomplete "%pb" format - fmt[1]
> will be 0 and we take precision as the length - presumably also
> 0.

yes, printf_spec is initialized to 0s.

> We stick that 0 into:
>
> > + len = bitmap_scnlistprintf(buf, end - buf, ptr, bits);
>
> Will that work?

Yes, bitmap_scnlistprintf() will only set buf[0] = 0 (which will be
overwritten by a possible next printf token) and return 0 length not
advancing the ptr.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


peterz at infradead

May 9, 2012, 8:32 AM

Post #9 of 24 (261 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 2012-05-09 at 16:15 +0200, Ingo Molnar wrote:
> > I guess I should use %.*pb and keep the field_width in case someone
> > manages to actually make bitmap_scnlistprintf() conform to it. The
> > precision is unused anyway.
>
> That's a cute trick, and it's intuitive as well.

kernel/sched/core.c:5570:3: warning: precision used with ‘%p’ gnu_printf format [-Wformat]

/me curses a bit.. anybody?


The below compiles and works (with the above caveat) and has all the
style muck people 'wanted'.

---
lib/vsprintf.c | 46 ++++++++++++++++++++++++++++++++--------------
1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index abbabec..6bd9a66 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -25,6 +25,9 @@
#include <linux/kallsyms.h>
#include <linux/uaccess.h>
#include <linux/ioport.h>
+#include <linux/bitmap.h>
+#include <linux/cpumask.h>
+#include <linux/numa.h>
#include <net/addrconf.h>

#include <asm/page.h> /* for PAGE_SIZE */
@@ -857,6 +860,9 @@ int kptr_restrict __read_mostly;
* correctness of the format string and va_list arguments.
* - 'K' For a kernel pointer that should be hidden from unprivileged users
* - 'NF' For a netdev_features_t
+ * - 'b' For a bitmap; %.*pb is required and the precision is used as bitmap length
+ * - 'bc' For a cpumask
+ * - 'bn' For a nodemask
*
* Note: The difference between 'S' and 'F' is that on ia64 and ppc64
* function pointers are really function descriptors, which contain a
@@ -902,24 +908,21 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
* 6: 000102...0f
*/
switch (fmt[1]) {
- case '6':
- return ip6_addr_string(buf, end, ptr, spec, fmt);
- case '4':
- return ip4_addr_string(buf, end, ptr, spec, fmt);
+ case '6': return ip6_addr_string(buf, end, ptr, spec, fmt);
+ case '4': return ip4_addr_string(buf, end, ptr, spec, fmt);
}
break;
case 'U':
return uuid_string(buf, end, ptr, spec, fmt);
- case 'V':
- {
- va_list va;
-
- va_copy(va, *((struct va_format *)ptr)->va);
- buf += vsnprintf(buf, end > buf ? end - buf : 0,
- ((struct va_format *)ptr)->fmt, va);
- va_end(va);
- return buf;
- }
+ case 'V': {
+ va_list va;
+
+ va_copy(va, *((struct va_format *)ptr)->va);
+ buf += vsnprintf(buf, end > buf ? end - buf : 0,
+ ((struct va_format *)ptr)->fmt, va);
+ va_end(va);
+ return buf;
+ }
case 'K':
/*
* %pK cannot be used in IRQ context because its test
@@ -941,6 +944,18 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
return netdev_feature_string(buf, end, ptr, spec);
}
break;
+ case 'b': {
+ int bits;
+
+ switch (fmt[1]) {
+ case 'c': bits = nr_cpumask_bits; break;
+ case 'n': bits = MAX_NUMNODES; break;
+ default: bits = spec.precision; break;
+ }
+
+ return buf + bitmap_scnlistprintf(buf, end - buf, ptr, bits);
+ }
+ default: break;
}
spec.flags |= SMALL;
if (spec.field_width == -1) {
@@ -1175,6 +1190,9 @@ int format_decode(const char *fmt, struct printf_spec *spec)
* %pI6c print an IPv6 address as specified by RFC 5952
* %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
* case.
+ * %.*pb print a bitmap list using the precision as bitmap length
+ * %pbc print a cpumask bitmap
+ * %pbn print a nodemask bitmap
* %n is ignored
*
* The return value is the number of characters which would

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at kernel

May 9, 2012, 8:34 AM

Post #10 of 24 (263 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

* Joe Perches <joe [at] perches> wrote:

> On Wed, 2012-05-09 at 15:59 +0200, Peter Zijlstra wrote:
> > The current implementation limits both (field_width and precision) to
> > s16, which will limit us to printing 32Kb bitmaps. If need arises we
> > could increase printf_spec size I guess.
>
> Not sure that's a problem really.
> A single printk is limited to 1024 bytes.
>
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> > @@ -941,6 +947,26 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> > return netdev_feature_string(buf, end, ptr, spec);
> > }
> > break;
> > + case 'b':
> > + {
> > + int bits, len;
>
> trivia: a tab indent could be avoided by using
> case b: {

Nit: 'b'.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at kernel

May 9, 2012, 8:41 AM

Post #11 of 24 (273 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

* Peter Zijlstra <peterz [at] infradead> wrote:

> On Wed, 2012-05-09 at 16:15 +0200, Ingo Molnar wrote:
> > > I guess I should use %.*pb and keep the field_width in case someone
> > > manages to actually make bitmap_scnlistprintf() conform to it. The
> > > precision is unused anyway.
> >
> > That's a cute trick, and it's intuitive as well.
>
> kernel/sched/core.c:5570:3: warning: precision used with ‘%p’ gnu_printf format [-Wformat]
>
> /me curses a bit.. anybody?

Yeah, that's a floating point precision thing (or for strings,
right-align) - could you use %30pb, etc?

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


peterz at infradead

May 9, 2012, 9:06 AM

Post #12 of 24 (261 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 2012-05-09 at 17:41 +0200, Ingo Molnar wrote:
> * Peter Zijlstra <peterz [at] infradead> wrote:
>
> > On Wed, 2012-05-09 at 16:15 +0200, Ingo Molnar wrote:
> > > > I guess I should use %.*pb and keep the field_width in case someone
> > > > manages to actually make bitmap_scnlistprintf() conform to it. The
> > > > precision is unused anyway.
> > >
> > > That's a cute trick, and it's intuitive as well.
> >
> > kernel/sched/core.c:5570:3: warning: precision used with ‘%p’ gnu_printf format [-Wformat]
> >
> > /me curses a bit.. anybody?
>
> Yeah, that's a floating point precision thing (or for strings,
> right-align) - could you use %30pb, etc?

%*pb you mean? Yeah that works, field width is allowed for %p. It does
mean we cannot ever make bitmaps respect field width though.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


joe at perches

May 9, 2012, 9:39 AM

Post #13 of 24 (261 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 2012-05-09 at 18:06 +0200, Peter Zijlstra wrote:
> On Wed, 2012-05-09 at 17:41 +0200, Ingo Molnar wrote:
> > * Peter Zijlstra <peterz [at] infradead> wrote:
> >
> > > On Wed, 2012-05-09 at 16:15 +0200, Ingo Molnar wrote:
> > > > > I guess I should use %.*pb and keep the field_width in case someone
> > > > > manages to actually make bitmap_scnlistprintf() conform to it. The
> > > > > precision is unused anyway.
> > > >
> > > > That's a cute trick, and it's intuitive as well.
> > >
> > > kernel/sched/core.c:5570:3: warning: precision used with ‘%p’ gnu_printf format [-Wformat]
> > >
> > > /me curses a bit.. anybody?
> >
> > Yeah, that's a floating point precision thing (or for strings,
> > right-align) - could you use %30pb, etc?
>
> %*pb you mean? Yeah that works, field width is allowed for %p. It does
> mean we cannot ever make bitmaps respect field width though.

You could also fill a new struct with info
about the bitmap and pass a pointer to that.

struct print_bitmap_info {
int foo;
int precision;
int width;
void *bitmap;
};
...
struct print_bitmap_info bi;
bi.foo = bar;
...
bi.bitmap = &bitmap;
...
printk("%pb", &bi);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


torvalds at linux-foundation

May 9, 2012, 10:15 AM

Post #14 of 24 (260 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, May 9, 2012 at 6:27 AM, Peter Zijlstra <peterz [at] infradead> wrote:
> + * - 'b' For a bitmap, consumes 2 args, second is int
> + * - 'bc' For a cpumask
> + * - 'bn' For a nodemask

Hard NAK.

No way. The "consumes 2 args" is fundamentally idiotic, since it
forces compiler warnings. The whole idea of %pXX was that you can give
it any pointer, because all that the compiler cares about is the "%p"
part, so random pointers to stuff won't break.

Your patch breaks the whole point of the extension.

The "bc" and "bn" would work, except for the fact that I doubt they
are printed out enough to matter.

A "%.*pb" is the only interface that can work for a "sized" bitmap
(with obviously fixed-length ones being possible with a "%.32bp" like
thing)

But the whole va_args games you play are not acceptable. %p *will*
continue to take a void *, and nothing else.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at kernel

May 9, 2012, 10:22 AM

Post #15 of 24 (270 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

* Joe Perches <joe [at] perches> wrote:

> On Wed, 2012-05-09 at 18:06 +0200, Peter Zijlstra wrote:
> > On Wed, 2012-05-09 at 17:41 +0200, Ingo Molnar wrote:
> > > * Peter Zijlstra <peterz [at] infradead> wrote:
> > >
> > > > On Wed, 2012-05-09 at 16:15 +0200, Ingo Molnar wrote:
> > > > > > I guess I should use %.*pb and keep the field_width in case someone
> > > > > > manages to actually make bitmap_scnlistprintf() conform to it. The
> > > > > > precision is unused anyway.
> > > > >
> > > > > That's a cute trick, and it's intuitive as well.
> > > >
> > > > kernel/sched/core.c:5570:3: warning: precision used with ‘%p’ gnu_printf format [-Wformat]
> > > >
> > > > /me curses a bit.. anybody?
> > >
> > > Yeah, that's a floating point precision thing (or for strings,
> > > right-align) - could you use %30pb, etc?
> >
> > %*pb you mean? Yeah that works, field width is allowed for %p. It does
> > mean we cannot ever make bitmaps respect field width though.
>
> You could also fill a new struct with info
> about the bitmap and pass a pointer to that.
>
> struct print_bitmap_info {
> int foo;
> int precision;
> int width;
> void *bitmap;
> };
> ...
> struct print_bitmap_info bi;
> bi.foo = bar;
> ...
> bi.bitmap = &bitmap;
> ...
> printk("%pb", &bi);

That defeats the whole purpose of avoiding extra complexity and
preparation - just passing in the bitmap pointer is the most
natural, most intuitive type to use.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


peterz at infradead

May 9, 2012, 10:22 AM

Post #16 of 24 (265 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 2012-05-09 at 10:15 -0700, Linus Torvalds wrote:
>
> The "bc" and "bn" would work, except for the fact that I doubt they
> are printed out enough to matter.

There's a number %pbc candidates in the scheduler that would be nice to
not have to do manually.

> A "%.*pb" is the only interface that can work for a "sized" bitmap
> (with obviously fixed-length ones being possible with a "%.32bp" like
> thing)

Right, sadly that also generates a warning :/

> But the whole va_args games you play are not acceptable. %p *will*
> continue to take a void *, and nothing else.

The va_args() games are dead.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at kernel

May 9, 2012, 10:24 AM

Post #17 of 24 (261 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

* Peter Zijlstra <peterz [at] infradead> wrote:

> On Wed, 2012-05-09 at 17:41 +0200, Ingo Molnar wrote:
> > * Peter Zijlstra <peterz [at] infradead> wrote:
> >
> > > On Wed, 2012-05-09 at 16:15 +0200, Ingo Molnar wrote:
> > > > > I guess I should use %.*pb and keep the field_width in case someone
> > > > > manages to actually make bitmap_scnlistprintf() conform to it. The
> > > > > precision is unused anyway.
> > > >
> > > > That's a cute trick, and it's intuitive as well.
> > >
> > > kernel/sched/core.c:5570:3: warning: precision used with ‘%p’ gnu_printf format [-Wformat]
> > >
> > > /me curses a bit.. anybody?
> >
> > Yeah, that's a floating point precision thing (or for strings,
> > right-align) - could you use %30pb, etc?
>
> %*pb you mean? Yeah that works, field width is allowed for %p.
> It does mean we cannot ever make bitmaps respect field width
> though.

I don't think that's a significant limitation, because it's
fixed width anyway. So if this works then this would be a pretty
good and simple to use solution.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


peterz at infradead

May 9, 2012, 10:25 AM

Post #18 of 24 (260 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 2012-05-09 at 19:24 +0200, Ingo Molnar wrote:
> I don't think that's a significant limitation, because it's
> fixed width anyway. So if this works then this would be a pretty
> good and simple to use solution.

Not quite following, there's nothing fixed width about
bitmap_scnlistprintf() output.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at kernel

May 9, 2012, 10:26 AM

Post #19 of 24 (264 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

* Peter Zijlstra <peterz [at] infradead> wrote:

> > A "%.*pb" is the only interface that can work for a "sized"
> > bitmap (with obviously fixed-length ones being possible with
> > a "%.32bp" like thing)
>
> Right, sadly that also generates a warning :/

Why not use what I suggested, %*pb? It also doubles as a width
field in essence, as bitmap output is fundamentally fixed width.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


peterz at infradead

May 9, 2012, 10:30 AM

Post #20 of 24 (262 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 2012-05-09 at 19:26 +0200, Ingo Molnar wrote:
> * Peter Zijlstra <peterz [at] infradead> wrote:
>
> > > A "%.*pb" is the only interface that can work for a "sized"
> > > bitmap (with obviously fixed-length ones being possible with
> > > a "%.32bp" like thing)
> >
> > Right, sadly that also generates a warning :/
>
> Why not use what I suggested, %*pb? It also doubles as a width
> field in essence, as bitmap output is fundamentally fixed width.

The bitmap list output uses RLE and generates things like:

6-11,18-23

Its the stuff used for all the sched_domain output etc..



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at kernel

May 9, 2012, 10:31 AM

Post #21 of 24 (262 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

* Peter Zijlstra <peterz [at] infradead> wrote:

> On Wed, 2012-05-09 at 19:24 +0200, Ingo Molnar wrote:
>
> > I don't think that's a significant limitation, because it's
> > fixed width anyway. So if this works then this would be a
> > pretty good and simple to use solution.
>
> Not quite following, there's nothing fixed width about
> bitmap_scnlistprintf() output.

There's a maximum field length possible, and we should/could use
that as padding.

Say for 8 bits, the worst-case is something like:

1-2,4-5,7-8

right? The max is roughly len*3/2.

I see a problem there: for something larger like 256 it would
lead to quite some padding ...

Well, we could just skip doing width for now - the ability to
easily print bitmaps is very valuable in itself.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


akpm at linux-foundation

May 9, 2012, 12:07 PM

Post #22 of 24 (274 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 09 May 2012 15:27:23 +0200
Peter Zijlstra <peterz [at] infradead> wrote:

> + * - 'b' For a bitmap, consumes 2 args, second is int

There's a pretty widespread convention that %b is used to print in
binary: printf("%b", 10) -> 1010. Regrettably it isn't part of
standard C, but it's often supported anyway. Perl's printf does this
as well. Perl uses %B also.

If we later want to add binary printing to kernel print(), we'd presumably
use %pb, to match this precedent.

So perhaps it would be better to avoid consuming `b' for bitmaps?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


peterz at infradead

May 9, 2012, 1:58 PM

Post #23 of 24 (245 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

On Wed, 2012-05-09 at 12:07 -0700, Andrew Morton wrote:
> On Wed, 09 May 2012 15:27:23 +0200
> Peter Zijlstra <peterz [at] infradead> wrote:
>
> > + * - 'b' For a bitmap, consumes 2 args, second is int
>
> There's a pretty widespread convention that %b is used to print in
> binary: printf("%b", 10) -> 1010. Regrettably it isn't part of
> standard C, but it's often supported anyway. Perl's printf does this
> as well. Perl uses %B also.
>
> If we later want to add binary printing to kernel print(), we'd presumably
> use %pb, to match this precedent.

I'd expect something like %ub, %p would take a pointer to a value.

Too bad both %b and %B generate a warning, it would be rather trivial to
add base 2 number stuff.

> So perhaps it would be better to avoid consuming `b' for bitmaps?

With %.*pb not actually working I could use %pC to mean a cpumask and
skip the generic bitmap stuff.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mingo at kernel

May 10, 2012, 12:45 AM

Post #24 of 24 (241 views)
Permalink
Re: [RFC][PATCH] printk: Add %pb to print bitmaps [In reply to]

* Peter Zijlstra <peterz [at] infradead> wrote:

> > So perhaps it would be better to avoid consuming `b' for
> > bitmaps?
>
> With %.*pb not actually working I could use %pC to mean a
> cpumask and skip the generic bitmap stuff.

I'd suggest to try that, all the objections came from trying to
support variable size bitmaps. Lets just solve the scheduler
bitmask problem and let others worry about the generic case?

You are trying to be too nice ;-)

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.