Re: [Xen-devel] [RFC] KEXEC: allocate crash note buffers at boot time v2

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.11.11 at 18:24, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>-static DEFINE_PER_CPU_READ_MOSTLY(void *, crash_notes);
>+static void * crash_notes[NR_CPUS];

If at all possible, can you please allocate this dynamically using
nr_cpu_ids for the size?

>+static int kexec_init_cpu_notes(const int cpu)

If the function's argument was made 'unsigned int' (as it already is in
its caller), then ...

>+    BUG_ON( cpu < 0 || cpu >= NR_CPUS );

... only the second check would be needed. Furthermore, it's
pointless to use signed types for CPU numbers (and it's
inefficient on various architectures), despite there being numerous
(bad) examples throughout the tree.

>+    if ( 0 == cpu )
>+        nr_bytes = nr_bytes +
>+            sizeof_note("Xen", sizeof(crash_xen_info_t));

Minor nit: Why not use += here (presumably allowing the statement to
fit on one line)?

>+    if ( ! note )
>+        /* Ideally, this would be -ENOMEM.  However, there are more problems
>+         * assocated with trying to deal with -ENOMEM sensibly than just
>+         * pretending that the crash note area doesn't exist. */
>+        return 0;

The only current caller ignores the return value, so why the bogus
return value?

>+    if ( nr < 0 || nr >= nr_cpu_ids || !cpu_online(nr) || !crash_notes[nr] )
>...
>-    if ( per_cpu(crash_notes, nr) == NULL )
>-    {
>...
>-    }

I would suggest to retry allocation here. Even if this isn't suitable for
a 32-bit kdump kernel on large systems, there#s no reason to penalize
fully 64-bit environment.

And here it would also become meaningful that kexec_init_cpu_notes()
actually returns a meaningful error upon failure.

Also, I can't see the reason for the patch to move around
do_crashdump_trigger() and crashdump_trigger_keyhandler.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel