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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 01/12/11 12:56, Jan Beulich wrote:
>>>> On 01.12.11 at 13:29, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> +static spinlock_t crash_notes_lock = SPIN_LOCK_UNLOCKED;
> Please use DEFINE_SPINLOCK() here.

Ok

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

Possibly.  In the case where a crash kernel has not been loaded, it
would degrade to a reboot, so it is still of some use if the there is no
kexec area.  Having said that, there is an explicit reboot handler, so
making this one disappear is probably a good thing.

>> +    crash_notes = xmalloc_bytes(nr_cpu_ids * sizeof(void*));
> Please use xmalloc_array() here.

Yes - it was dim of me to forget that.

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

Good point - I missed that.

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

So what we should do is this:

xmalloc
take lock
check to see if the entry is been filled in the meantime.  if so, free
the malloc'd region
release lock
only return -ENOMEM if we fail the malloc and the crash_note is still
NULL when we take the lock

I think this ought to cover all possible cases ?

(In reality I think the xmalloc itself should be covered by the fact we
will fail the !cpu_online(nr) test before we consider trying to
reallocate the buffer, but that doesn't preclude future proofing the code)

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

I had noticed this and was going to let it slide for now, considering
what would be best to do about it.  Playing with void pointers and
calculating lengths with sizeof is always more dangerous than
calculating a size, malloc'ing it and filling in a range start and size.

Given that it is such a rare codepath, I am honestly not sure which is
the better tradeof - an extra function call in 2 places or doubling the
size of the crash_notes array by introducing a size as well as a start. 
Both seem very minor in the grand scheme of things.

> Jan
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com