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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/12/11 09:08, Jan Beulich wrote:
>>>> 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?

Ok

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

Good point.  I will clean this up

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

Because the next few patches in my series adds a new crash notes at this
point.  I felt it was neater to leave in this form for a reduced diff
next patch, and gcc will optimize it to +=

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

Originally it passed the return value back up to the cpu hotplug
notifier, but I decided that causing an -ENOMEM at this point was a
little silly given that:
1) a lack of mem on boot will quickly kill the host in other ways, and
2) while there is no way useful way to ensure that the crashdump kernel
gets reloaded with new notes pointers, blocking on not being able to
allocate notes is silly.

Would you consider this enough of a problem to actually fail the
CPU_PREPARE_UP ?

On further thought from my musings yesterday, it would be fantastic if
we were able to point the kdump kernel at a PT_NOTE header without
discerned length, at which point Xen can rewrite its crash notes without
having to get /sbin/kexec to repackage its magic elf blog pointing to
new/different memory locations.  However, as this would involve changes
to kexec-tools, Linux (and Xen) in a not trivially backward compatible
fashion, the chances of it happening are slim.

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

At this point, none of the allocation makes it suitable for a 32bit
system.  For that, I need to start investigating something akin to
xalloc_below(), which is probably going to be todays task.  If we have
previously failed to allocate the notes buffer (and are somehow still
running), what if anything makes it likely that we will successfully
allocate memory this time?

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

This is probably collateral damage from having to reorder sizeof_note()
and setup_note() in preference to making a forward declaration of them. 
I will see about fixing.

> Jan
>

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


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