Re: [Xen-devel] [RFC] KEXEC: Clean up logic to choose a range for the crash kernel

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/11/11 15:36, Jan Beulich wrote:
>>>> On 25.11.11 at 16:12, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>> Before this patch, using "classic" crashdump= syntax (<size>@<start>)
>> causes an exact size and location to be reserved in the e820 map
>> before Xen and modules are relocated.  However, using "new" syntax
>> (<start>-[<end>]:<size>) encounteres a myriad of problems, most
>> notibly not considering the range when allocating its region.
> I'm sure I tested this (with some basic values), so the "myriad of
> problems" make me curious as to what they are.

The comment was a bit old from before I twigged what some of the code
was doing.  The two key problems were only considering the size, and a
bug in set_kexec_crash_area_size() which will only consider a range as
being valid if its end is greater than system_ram.  This bug prevents
specifying a range as "128M anywhere in 1G-2G" actually working on a box
with more than 2G of ram.

> What does "range" refer to here, and what is "its region"?

I will need to clean up my use of the term range and region, although it
doesn't help that there is ambiguity in the source code as well. 
Loosely, I have been using "range" to mean a possible range as given on
the command line, and "region" to be the actual area in memory where the
kdump kernel is going to be put.

> You also seem to overlook that even the old syntax allowed for only
> specifying a size.

No.  In the case of using the old syntax, both
kexec_crash_area.{start,size} are set, which caused the earlier call to
kexec_reserve_region() in __setup_xen() to attempt to reserve an exact
region at an exact address.  Using the newer syntax,
kexec_crash_area.size did not get set in the parsing function, and got
considered in set_kexec_crash_area_size(), which only sets the size. 
The subsiquent call to kexec_reverse_area() on the next line fails
silently because kexec_crash_area.start is 0.  Later, when considering
the modules, kexec_crash_area.start gets set to e-size, at which point
the call to kexec_reserve_area() actually does attempt to reserve a
region, based on the limits of an arbitrary e820 entry, not on the
limits provided on the command line.

>> This patch tries to unify both codepaths, and fix the broken logic
>> when using "newer" syntax.
>>
>> Changes addressed:
>>
>> 1) Remove use of kexec_crash_area from parse_crashkernel().
>>     Use ranges[0] in preference, as there is no support for multiple
>>     ranges using classic syntax.  This prevents some wierd logic in
>>     __setup_xen() depending on whether kexec_crash_area.size is set
>>     or not.
> What weird about that?

As described above.

>> 2) Change set_kexec_crash_area_size() to choose_kexec_range()
>>     The new method of choosing a range provides an entire rather than
> And entire what?

Oops.  An entire "kexec_range" entry giving a start, end and size value,
rather than the current case which takes either an exact size/start pair
from the 'classic' case, or a size anywhere in memory for the 'newer' case.

>>     just setting kexec_crash_area.size.  Also, fix the very broken
>>     logic which will only consider a range if its .end is greater
>>     than system_ram.
> Where's that happening? Are you perhaps mis-interpreting the
> purpose of these ranges?

I might possibly be misinterpreting these ranges as the only
documentation is the code, but I believe I have got it correct based on
the upstream Linux code.  What are your interpretations of the ranges?

>>  Additionally, prefer range with the largest
>>     size if we have a choice of valid ones, and prefer ranges with
>>     more flexibility in their limits.
>>
>> 3) Move kexec_reserve_area() from setup.c to kexec.c
>>     It makes more sense here, Also, change its functionality a little
>>     to always work from the kexec range provided by choose_kexec_range()
>>
>> 4) Remove the kexec reservation code when considering modules
>>     This code only had any effect for people using the "newer" syntax
>>     on the command line, as people using the "classic" syntax would
>>     reserve a memory region before considering modules.
> Are you sure? How do you prevent the kexec area from overlapping
> any of the modules (in particular the initrd, which can get passed in
> place to Dom0)?

Good point, which is why this is an RFC.  Note that the "classic"
syntax, which everyone refers to in documents on the subject, will never
consider any of the modules.  I am not really sure which is best, but I
would consider positioning the kdump kernel more important than the
location of initrd.  The user is likely to know more about why their
kdump kernel needs to be located where specified than Xen does.  Perhaps
after deciding where the kdump kernel should go, we should move any
overlapping modules?

> 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