Re: [PATCH 3 of 3] KEXEC: Allocate crash structures in low memory

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

On 22/12/11 18:09, David Vrabel wrote:
> On 22/12/11 17:36, Andrew Cooper wrote:
>> On 64bit Xen with 32bit dom0 and crashkernel, xmalloc'ing items such
>> as the CPU crash notes will go into the xenheap, which tends to be in
>> upper memory.  This causes problems on machines with more than 64GB
>> (or 4GB if no PAE support) of ram as the crashkernel physically cant
>> access the crash notes.
> I've never been entirely convinced that this was the correct approach.
> It seems that using a 64-bit crash kernel would be an easier solution.

In an ideal world yes, but reality is that XS will stay with a 32bit
kernel for a while yet.

>> @@ -320,7 +405,10 @@ static int kexec_init_cpu_notes(const un
>>          return ret;
>>  
>>      nr_bytes = sizeof_cpu_notes(cpu);
>> -    note = xmalloc_bytes(nr_bytes);
>> +
>> +    /* If we dont care about the position of allocation, malloc. */
>> +    if ( low_crashinfo_mode == LOW_CRASHINFO_NONE )
>> +        note = xmalloc_bytes(nr_bytes);
> Suggest refactoring this (and the other similar places) so that the
> check for the regular/crash head occurs in one wrapper alloc function.

Can't refactor this as its other half is specifically inside the lock
while this is outside.

>>      /* Protect the write into crash_notes[] with a spinlock, as this function
>>       * is on a hotplug path and a hypercall path. */
>> @@ -338,6 +426,11 @@ static int kexec_init_cpu_notes(const un
>>      }
>>      else
>>      {
>> +        /* If we care about memory possition, alloc from the crash heap,
>> +         * also protected by the crash_notes_lock. */
>> +        if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
>> +            note = alloc_from_crash_heap(nr_bytes);
>> +
>>          crash_notes[cpu].start = note;
>>          crash_notes[cpu].size = nr_bytes;
>>          spin_unlock(&crash_notes_lock);
>> @@ -397,6 +490,24 @@ static struct notifier_block cpu_nfb = {
>>      .notifier_call = cpu_callback
>>  };
>>  
>> +void __init kexec_early_calculations(void)
>> +{
>> +    /* If low_crashinfo_mode is still INVALID, neither "low_crashinfo" nor
>> +     * "crashinfo_maxaddr" have been specified on the command line, so
>> +     * explicitly set to NONE. */
>> +    if ( low_crashinfo_mode == LOW_CRASHINFO_INVALID )
>> +        low_crashinfo_mode = LOW_CRASHINFO_NONE;
> Why not initialize low_crash_info_mode to NONE?

So there is not a dependency between the order of low_crashinfo and
crashinfo_maxaddr, while still allowing each option to set a sensible
default for the other if only a single one is specified on the command line.

>> +
>> +    crashinfo_maxaddr_bits = 0;
>> +    if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
>> +    {
>> +        paddr_t tmp = crashinfo_maxaddr;
>> +
>> +        while ( tmp >>= 1 )
>> +            crashinfo_maxaddr_bits++;
>> +    }
>> +}
> Do these during the parsing of the command line?

Argument along the same lines as above w.r.t two command line
arguments.  crashinfo_maxaddr_bits is always used.  Therefore it must be
0 in the case where no limits are applied, meaning that setting it to a
default of lg(4GB) will break things.  Setting it in the parsing
function would prevent a sensible default being set if the user only
specified low_crashinfo without making a combinational explosion of
logic in the parsing function.

> These will allow you to remove this unhelpfully named function.
>
>> +
>>  static int __init kexec_init(void)
>>  {
>>      void *cpu = (void *)(unsigned long)smp_processor_id();
>> @@ -407,6 +518,29 @@ static int __init kexec_init(void)
>>  
>>      register_keyhandler('C', &crashdump_trigger_keyhandler);
>>  
>> +    if ( low_crashinfo_mode > LOW_CRASHINFO_NONE )
>> +    {
>> +        size_t crash_heap_size;
>> +
>> +        /* This calculation is safe even if the machine is booted in 
>> +         * uniprocessor mode. */
>> +        crash_heap_size = sizeof_cpu_notes(0) +
>> +            sizeof_cpu_notes(1) * (nr_cpu_ids - 1);
>> +        crash_heap_size = PAGE_ALIGN(crash_heap_size);
>> +
>> +        crash_heap_current = alloc_xenheap_pages(
>> +            get_order_from_bytes(crash_heap_size),
>> +            MEMF_bits(crashinfo_maxaddr_bits) );
>> +
>> +        if ( crash_heap_current )
>> +            crash_heap_end = crash_heap_current + crash_heap_size;
>> +        else
>> +            return -ENOMEM;
>> +    }
> Suggest moving this into a crash_heap_setup() function.

Questionable.  I would argue that it is trading an extra indirection in
the source code for a gain of only a few lines, which will be inlined
back to here by the compiler.

>> +
>> +    /* crash_notes may be allocated anywhere Xen can reach in memory.
>> +       Only the individual CPU crash notes themselves must be allocated
>> +       in lower memory if requested. */
>>      crash_notes = xmalloc_array(crash_note_range_t, nr_cpu_ids);
>>      if ( ! crash_notes )
>>          return -ENOMEM;
>> diff -r 3da37c68284f -r 23c31d59ffb1 xen/drivers/char/console.c
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -584,6 +584,7 @@ void __init console_init_postirq(void)
>>  {
>>      char *ring;
>>      unsigned int i, order;
>> +    u64 memflags;
>>  
>>      serial_init_postirq();
>>  
>> @@ -591,7 +592,8 @@ void __init console_init_postirq(void)
>>          opt_conring_size = num_present_cpus() << (9 + xenlog_lower_thresh);
>>  
>>      order = get_order_from_bytes(max(opt_conring_size, conring_size));
>> -    while ( (ring = alloc_xenheap_pages(order, 0)) == NULL )
>> +    memflags = low_crashinfo_mode > LOW_CRASHINFO_NONE ? MEMF_bits(crashinfo_maxaddr_bits) : 0;
> If you set crashinfo_maxaddr_bits to 64 if low_crashinfo_mode isn't used
> you wouldn't need this test.

I am not familiar enough with the intricacies of alloc_xenheap_pages to
know whether that is safe.  cc'ing Tim for his expertise.

> David

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