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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.12.11 at 13:29, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>+static spinlock_t crash_notes_lock = SPIN_LOCK_UNLOCKED;

Please use DEFINE_SPINLOCK() here.

>+    register_keyhandler('C', &crashdump_trigger_keyhandler);
>+
>+    /* If no crash area, no need to allocate space for notes. */
>+    if ( 0 == kexec_crash_area.size )
>+        return 0;

Wouldn't it make sense to switch the order of these?

>+    crash_notes = xmalloc_bytes(nr_cpu_ids * sizeof(void*));

Please use xmalloc_array() here.

>+    if ( !crash_notes[nr] && 0 != kexec_init_cpu_notes(nr) )

The first check is pointless - the function will return zero if the
allocation was already done.

Further, you shouldn't take a lock around a call to xmalloc() or alike
unless absolutely necessary. It is pretty simple to avoid here - you
really only need to lock around the storing of the pointer and maybe
the setup_note() calls (but be careful with returning -ENOMEM - you
shouldn't if the allocation fails, but you then find - under the lock -
that a pointer was already set by another CPU).

Finally, one thing I failed to notice on the previous version - the
nr_bytes calculations are now being done twice. This should
probably be moved into a helper function, especially since you
said you intend to add stuff here subsequently.

Jan


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