[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC] KEXEC: Clean up logic to choose a range for the crash kernel
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |