[PATCH] dyn_array: using %pF instead of print_fn_descriptor_symbol

[PATCH] dyn_array: using %pF instead of print_fn_descriptor_symbol

[Yinghai Lu]

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>


diff --git a/init/dyn_array.c b/init/dyn_array.c
index c4cd902..e85b1d6 100644
--- a/init/dyn_array.c
+++ b/init/dyn_array.c
@@ -17,10 +17,9 @@ void __init pre_alloc_dyn_array(void)
 	for (daa = __dyn_array_start ; daa < __dyn_array_end; daa++) {
 		struct dyn_array *da = *daa;
 
+		printk(KERN_DEBUG "dyn_array %pF size:%#lx nr:%d align:%#lx\n",
+			da->name, da->size, *da->nr, da->align);
 		size = da->size * (*da->nr);
-		print_fn_descriptor_symbol("dyn_array %s ", da->name);
-		printk(KERN_CONT "size:%#lx nr:%d align:%#lx\n",
-			da->size, *da->nr, da->align);
 		total_size += roundup(size, da->align);
 		if (da->align > max_align)
 			max_align = da->align;
@@ -42,11 +41,10 @@ void __init pre_alloc_dyn_array(void)
 		struct dyn_array *da = *daa;
 
 		size = da->size * (*da->nr);
-		print_fn_descriptor_symbol("dyn_array %s ", da->name);
-
 		phys = roundup(phys, da->align);
+		printk(KERN_DEBUG "dyn_array %pF ==> [%#lx - %#lx]\n",
+			da->name, phys, phys + size);
 		*da->name = phys_to_virt(phys);
-		printk(KERN_CONT " ==> [%#lx - %#lx]\n", phys, phys + size);
 
 		phys += size;
 
@@ -74,10 +72,9 @@ unsigned long __init per_cpu_dyn_array_size(unsigned long *align)
 	for (daa = __per_cpu_dyn_array_start ; daa < __per_cpu_dyn_array_end; daa++) {
 		struct dyn_array *da = *daa;
 
+		printk(KERN_DEBUG "per_cpu_dyn_array %pF size:%#lx nr:%d align:%#lx\n",
+			da->name, da->size, *da->nr, da->align);
 		size = da->size * (*da->nr);
-		print_fn_descriptor_symbol("per_cpu_dyn_array %s ", da->name);
-		printk(KERN_CONT "size:%#lx nr:%d align:%#lx\n",
-			da->size, *da->nr, da->align);
 		total_size += roundup(size, da->align);
 		if (da->align > max_align)
 			max_align = da->align;
@@ -104,15 +101,15 @@ void __init per_cpu_alloc_dyn_array(int cpu, char *ptr)
 		struct dyn_array *da = *daa;
 
 		size = da->size * (*da->nr);
-		print_fn_descriptor_symbol("per_cpu_dyn_array %s ", da->name);
-
 		phys = roundup(phys, da->align);
+		printk(KERN_DEBUG "per_cpu_dyn_array %pF ==> [%#lx - %#lx]\n",
+			da->name, phys, phys + size);
+
 		addr = (unsigned long)da->name;
 		addr += per_cpu_offset(cpu);
 		array = (void **)addr;
 		*array = phys_to_virt(phys);
 		*da->name = *array; /* so init_work could use it directly */
-		printk(KERN_CONT " ==> [%#lx - %#lx]\n", phys, phys + size);
 
 		phys += size;
 

Re: [PATCH] dyn_array: using %pF instead of print_fn_descriptor_symbol

[Andrew Morton]

On Fri, 29 Aug 2008 15:07:49 -0700
Yinghai Lu <yhlu.kernel@gmail.com> wrote:

> +		printk(KERN_DEBUG "per_cpu_dyn_array %pF ==> [%#lx - %#lx]\n",
> +			da->name, phys, phys + size);

This:

struct dyn_array {
	void **name;

is a bit confusing.  One normally expects a variable called "name" to
point at a character string.

What _does_ this thing point at?  There are no code comments which I
can find, it's unobvious from the source code, the type is the
information-free void** and the identifier is misleading.

I find that documenting the data structures is the best way of making
code understandable (and hence maintainable).

Re: [PATCH] dyn_array: using %pF instead of print_fn_descriptor_symbol

[Yinghai Lu]

On Fri, Aug 29, 2008 at 3:20 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 29 Aug 2008 15:07:49 -0700
> Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>
>> +             printk(KERN_DEBUG "per_cpu_dyn_array %pF ==> [%#lx - %#lx]\n",
>> +                     da->name, phys, phys + size);
>
> This:
>
> struct dyn_array {
>        void **name;
>
> is a bit confusing.  One normally expects a variable called "name" to
> point at a character string.
>
> What _does_ this thing point at?  There are no code comments which I
> can find, it's unobvious from the source code, the type is the
> information-free void** and the identifier is misleading.
>
> I find that documenting the data structures is the best way of making
> code understandable (and hence maintainable).

struct dyn_array {
        void **name;
        unsigned long size;
        unsigned int *nr;
        unsigned long align;
        void (*init_work)(void *);
};
extern struct dyn_array *__dyn_array_start[], *__dyn_array_end[];
extern struct dyn_array *__per_cpu_dyn_array_start[],
*__per_cpu_dyn_array_end[];

#define DEFINE_DYN_ARRAY_ADDR(nameX, addrX, sizeX, nrX, alignX, init_workX) \
                static struct dyn_array __dyn_array_##nameX __initdata = \
                {       .name = (void **)&(nameX),\
                        .size = sizeX,\
                        .nr   = &(nrX),\
                        .align = alignX,\
                        .init_work = init_workX,\
                }; \
                static struct dyn_array *__dyn_array_ptr_##nameX __used \
                __attribute__((__section__(".dyn_array.init"))) = \
                        &__dyn_array_##nameX

#define DEFINE_DYN_ARRAY(nameX, sizeX, nrX, alignX, init_workX) \
        DEFINE_DYN_ARRAY_ADDR(nameX, nameX, sizeX, nrX, alignX, init_workX)

and use is

struct irq_desc *sparse_irqs;
DEFINE_DYN_ARRAY(sparse_irqs, sizeof(struct irq_desc), nr_irq_desc,
PAGE_SIZE, init_work);


then sparse_irqs is pointer, and .name store the address of that pointer.

later use
*da->name = phys_to_virt(phys);
to take back the dyn address.

YH

Re: [PATCH] dyn_array: using %pF instead of print_fn_descriptor_symbol

[Andrew Morton]

On Fri, 29 Aug 2008 15:32:58 -0700
"Yinghai Lu" <yhlu.kernel@gmail.com> wrote:

> On Fri, Aug 29, 2008 at 3:20 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Fri, 29 Aug 2008 15:07:49 -0700
> > Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> >
> >> +             printk(KERN_DEBUG "per_cpu_dyn_array %pF ==> [%#lx - %#lx]\n",
> >> +                     da->name, phys, phys + size);
> >
> > This:
> >
> > struct dyn_array {
> >        void **name;
> >
> > is a bit confusing.  One normally expects a variable called "name" to
> > point at a character string.
> >
> > What _does_ this thing point at?  There are no code comments which I
> > can find, it's unobvious from the source code, the type is the
> > information-free void** and the identifier is misleading.
> >
> > I find that documenting the data structures is the best way of making
> > code understandable (and hence maintainable).
> 
> struct dyn_array {
>         void **name;
>         unsigned long size;
>         unsigned int *nr;
>         unsigned long align;
>         void (*init_work)(void *);
> };
> extern struct dyn_array *__dyn_array_start[], *__dyn_array_end[];
> extern struct dyn_array *__per_cpu_dyn_array_start[],
> *__per_cpu_dyn_array_end[];
> 
> #define DEFINE_DYN_ARRAY_ADDR(nameX, addrX, sizeX, nrX, alignX, init_workX) \
>                 static struct dyn_array __dyn_array_##nameX __initdata = \
>                 {       .name = (void **)&(nameX),\
>                         .size = sizeX,\
>                         .nr   = &(nrX),\
>                         .align = alignX,\
>                         .init_work = init_workX,\
>                 }; \
>                 static struct dyn_array *__dyn_array_ptr_##nameX __used \
>                 __attribute__((__section__(".dyn_array.init"))) = \
>                         &__dyn_array_##nameX
> 
> #define DEFINE_DYN_ARRAY(nameX, sizeX, nrX, alignX, init_workX) \
>         DEFINE_DYN_ARRAY_ADDR(nameX, nameX, sizeX, nrX, alignX, init_workX)
> 
> and use is
> 
> struct irq_desc *sparse_irqs;
> DEFINE_DYN_ARRAY(sparse_irqs, sizeof(struct irq_desc), nr_irq_desc,
> PAGE_SIZE, init_work);
> 
> 
> then sparse_irqs is pointer, and .name store the address of that pointer.
> 
> later use
> *da->name = phys_to_virt(phys);
> to take back the dyn address.
> 

Well yes, I have a copy of that too.

Why is it called "name"?

Re: [PATCH] dyn_array: using %pF instead of print_fn_descriptor_symbol

[Andrew Morton]

>	ptr = __alloc_bootmem_nopanic(total_size, max_align, 0);
>	if (!ptr)
>		panic("Can not alloc dyn_alloc\n");

Why duplicate the panic()?  Just call __alloc_bootmem().

> #ifdef CONFIF_GENERIC_HARDIRQS

That doesn't appear to have been very well tested?

The code has a few coding-style glitches which checkpatch can detect.

Re: [PATCH] dyn_array: using %pF instead of print_fn_descriptor_symbol

[Yinghai Lu]

On Fri, Aug 29, 2008 at 3:53 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>>       ptr = __alloc_bootmem_nopanic(total_size, max_align, 0);
>>       if (!ptr)
>>               panic("Can not alloc dyn_alloc\n");

like to give exact error message.

>
> Why duplicate the panic()?  Just call __alloc_bootmem().
>
>> #ifdef CONFIF_GENERIC_HARDIRQS
>
> That doesn't appear to have been very well tested?

ah!
it should break sparc, m68k, and s390...

>
> The code has a few coding-style glitches which checkpatch can detect.
>

should only have 80 char length warning...

YH

Re: [PATCH] dyn_array: using %pF instead of print_fn_descriptor_symbol

[Andrew Morton]

On Fri, 29 Aug 2008 16:34:26 -0700
"Yinghai Lu" <yhlu.kernel@gmail.com> wrote:

> On Fri, Aug 29, 2008 at 3:53 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >>       ptr = __alloc_bootmem_nopanic(total_size, max_align, 0);
> >>       if (!ptr)
> >>               panic("Can not alloc dyn_alloc\n");
> 
> like to give exact error message.

It's pointless.  panic() will do a dump_stack().

> >
> > Why duplicate the panic()?  Just call __alloc_bootmem().
> >
> >> #ifdef CONFIF_GENERIC_HARDIRQS
> >
> > That doesn't appear to have been very well tested?
> 
> ah!
> it should break sparc, m68k, and s390...
> 
> >
> > The code has a few coding-style glitches which checkpatch can detect.
> >
> 
> should only have 80 char length warning...
> 

sure.  The code looks rather miserable in an 80-col display.

there's also

WARNING: braces {} are not necessary for single statement blocks
#119: FILE: dyn_array.c:119:
+               if (da->init_work) {
+                       da->init_work(da);
+               }



and checkpatch should have detected the misplaced semicolon here:

+       for (daa = __per_cpu_dyn_array_start ; daa < __per_cpu_dyn_array_end; daa++) {

but didn't.

		

Re: [PATCH] dyn_array: using %pF instead of print_fn_descriptor_symbol

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> >	ptr = __alloc_bootmem_nopanic(total_size, max_align, 0);
> >	if (!ptr)
> >		panic("Can not alloc dyn_alloc\n");
> 
> Why duplicate the panic()?  Just call __alloc_bootmem().

agreed.

> > #ifdef CONFIF_GENERIC_HARDIRQS
> 
> That doesn't appear to have been very well tested?

on non-genirq systems? Most likely. If then most testing they get is 
from cross-build tools. Few if any actual users.

> The code has a few coding-style glitches which checkpatch can detect.

yeah. Yinghai, could you please fix them?

	Ingo

Re: [PATCH] dyn_array: using %pF instead of print_fn_descriptor_symbol

[Yinghai Lu]

On Sat, Sep 6, 2008 at 10:09 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Andrew Morton <akpm@linux-foundation.org> wrote:
>
>> >     ptr = __alloc_bootmem_nopanic(total_size, max_align, 0);
>> >     if (!ptr)
>> >             panic("Can not alloc dyn_alloc\n");
>>
>> Why duplicate the panic()?  Just call __alloc_bootmem().
>
> agreed.
>
>> > #ifdef CONFIF_GENERIC_HARDIRQS
>>
>> That doesn't appear to have been very well tested?
>
> on non-genirq systems? Most likely. If then most testing they get is
> from cross-build tools. Few if any actual users.
>
>> The code has a few coding-style glitches which checkpatch can detect.
>
> yeah. Yinghai, could you please fix them?

already in tip/master and -mm
except  first one.

YH

Re: [PATCH] dyn_array: using %pF instead of print_fn_descriptor_symbol

[Ingo Molnar]

* Yinghai Lu <yhlu.kernel@gmail.com> wrote:

> On Sat, Sep 6, 2008 at 10:09 AM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> >> >     ptr = __alloc_bootmem_nopanic(total_size, max_align, 0);
> >> >     if (!ptr)
> >> >             panic("Can not alloc dyn_alloc\n");
> >>
> >> Why duplicate the panic()?  Just call __alloc_bootmem().
> >
> > agreed.
> >
> >> > #ifdef CONFIF_GENERIC_HARDIRQS
> >>
> >> That doesn't appear to have been very well tested?
> >
> > on non-genirq systems? Most likely. If then most testing they get is
> > from cross-build tools. Few if any actual users.
> >
> >> The code has a few coding-style glitches which checkpatch can detect.
> >
> > yeah. Yinghai, could you please fix them?
> 
> already in tip/master and -mm
> except  first one.

that needs fixing too i think. We dont really want to sprinkle the code 
with various specific panics.

	Ingo

Re: [PATCH] dyn_array: using %pF instead of print_fn_descriptor_symbol

[Yinghai Lu]

On Sat, Sep 6, 2008 at 10:18 AM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Yinghai Lu <yhlu.kernel@gmail.com> wrote:
>
>> On Sat, Sep 6, 2008 at 10:09 AM, Ingo Molnar <mingo@elte.hu> wrote:
>> >
>> > * Andrew Morton <akpm@linux-foundation.org> wrote:
>> >
>> >> >     ptr = __alloc_bootmem_nopanic(total_size, max_align, 0);
>> >> >     if (!ptr)
>> >> >             panic("Can not alloc dyn_alloc\n");
>> >>
>> >> Why duplicate the panic()?  Just call __alloc_bootmem().
>> >
>> > agreed.
>> >
>> >> > #ifdef CONFIF_GENERIC_HARDIRQS
>> >>
>> >> That doesn't appear to have been very well tested?
>> >
>> > on non-genirq systems? Most likely. If then most testing they get is
>> > from cross-build tools. Few if any actual users.
>> >
>> >> The code has a few coding-style glitches which checkpatch can detect.
>> >
>> > yeah. Yinghai, could you please fix them?
>>
>> already in tip/master and -mm
>> except  first one.
>
> that needs fixing too i think. We dont really want to sprinkle the code
> with various specific panics.

ok, please check that in another mail.

YH