|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3 of 3] KEXEC: Allocate crash structures in low memory
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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |