dyn_array: using %pF instead of print_fn_descriptor_symbol

PATCH In review Yinghai Lu 3 participants 11 messages Aug 29, 2008 – Sep 6, 2008

lore

Share & tools

lore

Yinghai Lu

author

2008-08-29 22:08 UTC #0 lore raw

PATCH 1/1 dyn_array: using %pF instead of print_fn_descriptor_symbol

Signed-off-by Yinghai Lu <[email protected]>

init/dyn_array.c +9 ~~-12~~

index c4cd902..e85b1d6 100644
@@ -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;

Andrew Morton

2008-08-29 22:21 UTC #1 lore raw

init/dyn_array.c void __init per_cpu_alloc_dyn_array(int cpu, char *ptr) · line 106

Quoted 2 lines · On Fri, 29 Aug 2008 15:07:49 -0700 Yinghai Lu <[email protected]> wrote:

init/dyn_array.c +2 -0

+		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).

Yinghai Lu

author

2008-08-29 22:33 UTC #2 lore raw

Quoted 14 lines · Andrew Morton

init/dyn_array.c void __init per_cpu_alloc_dyn_array(int cpu, char *ptr) · line 106
Quoted 2 lines · On Fri, 29 Aug 2008 15:07:49 -0700 Yinghai Lu <[email protected]> wrote:
init/dyn_array.c +2 -0
+             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.

Quoted 6 lines

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.

Andrew Morton

2008-08-29 22:45 UTC #3 lore raw

Quoted 56 lines · On Fri, 29 Aug 2008 15:32:58 -0700 "Yinghai Lu" <[email protected]> wrote:

Quoted 14 lines · Andrew Morton
init/dyn_array.c void __init per_cpu_alloc_dyn_array(int cpu, char *ptr) · line 106
Quoted 2 lines · On Fri, 29 Aug 2008 15:07:49 -0700 Yinghai Lu <[email protected]> wrote:
init/dyn_array.c +2 -0
+             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.
Quoted 6 lines
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);

Quoted 6 lines

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"?

Andrew Morton

2008-08-29 22:53 UTC #4 lore raw

In reply to Yinghai Lu

Quoted 3 lines

	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().

Quoted 1 line

#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.

Yinghai Lu

author

2008-08-29 23:34 UTC #5 lore raw

Quoted 3 lines · Andrew Morton

Quoted 3 lines

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

like to give exact error message.

Quoted 6 lines

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

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

Quoted 3 lines

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

should only have 80 char length warning...

Andrew Morton

2008-08-30 00:23 UTC #6 lore raw

Quoted 7 lines · On Fri, 29 Aug 2008 16:34:26 -0700 "Yinghai Lu" <[email protected]> wrote:

Quoted 3 lines · Andrew Morton
Quoted 3 lines
      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().

Quoted 7 lines

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

Quoted 9 lines

ah!
it should break sparc, m68k, and s390...
Quoted 3 lines
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.

Ingo Molnar

2008-09-06 17:10 UTC #7 lore raw

In reply to Andrew Morton

Quoted 5 lines · Andrew Morton

Quoted 3 lines
	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.

Quoted 3 lines

Quoted 1 line
#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.

Quoted 1 line

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

yeah. Yinghai, could you please fix them?

Ingo

Yinghai Lu

author

2008-09-06 17:16 UTC #8 lore raw

Quoted 13 lines · Ingo Molnar

Quoted 5 lines · Andrew Morton
Quoted 3 lines
    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.
Quoted 2 lines
Quoted 1 line
#ifdef CONFIF_GENERIC_HARDIRQS

Quoted 8 lines

Quoted 1 line
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.
Quoted 1 line
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.

Ingo Molnar

2008-09-06 17:18 UTC #9 lore raw

Quoted 19 lines · Yinghai Lu

Quoted 18 lines · Ingo Molnar
Quoted 5 lines · Andrew Morton
Quoted 3 lines
    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.
Quoted 3 lines
Quoted 1 line
#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.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
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

Yinghai Lu

author

2008-09-06 17:27 UTC #10 lore raw

Quoted 24 lines · Ingo Molnar

Quoted 21 lines · Yinghai Lu
Quoted 20 lines · Ingo Molnar
Quoted 5 lines · Andrew Morton
Quoted 3 lines
    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.
Quoted 3 lines
Quoted 1 line
#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.
Quoted 1 line
The code has a few coding-style glitches which checkpatch can detect.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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.