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 16:03, Andrew Cooper wrote:
> 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?

On second thoughts, how about this?

If a user provides an exact position and size for the kdump kernel,
place the kdump kernel there and move any overlapping modules.

If the user provides a range, attempt to fit the kdump kernel around
other modules, and in the case where we cant, move the offending
module(s) elsewhere.

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