Re: [Xen-devel] [PATCH v2] xen: arm: improve handling of system with non-contiguous RAM regions

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-12-04 at 17:55 +0000, George Dunlap wrote:
> On 12/04/2013 05:29 PM, Ian Campbell wrote:
> > On Wed, 2013-12-04 at 17:02 +0000, George Dunlap wrote:
> >> On Mon, Dec 2, 2013 at 2:39 PM, Ian Campbell <ian.campbell@xxxxxxxxxx> 
> >> wrote:
> >>> arm32 currently only makes use of memory which is contiguous with the 
> >>> first
> >>> bank. On the Midway platform this means that we only use 4GB of the 8GB
> >>> available.
> >>>
> >>> Change things to make use of non-contiguous memory regions with the
> >>> restriction that we require that at least half of the total span of the 
> >>> RAM
> >>> addresses contain RAM. The frametable is currently not sparse and so this
> >>> restriction avoids problems with allocating enormous amounts of memory 
> >>> for the
> >>> frametable to cover holes in the address space and exhausting the actual 
> >>> RAM.
> >>>
> >>> 50% is arguably too restrictive. 4GB of RAM requires 32MB of frametable on
> >>> arm32 and 56M on arm64, so we could probably cope with a lower ratio of 
> >>> actual
> >>> RAM. However half is nice and conservative.
> >>>
> >>> arm64 currently uses all banks without regard for the size of the 
> >>> frametable,
> >>> which I have observed causing problems on models. Implement that same
> >>> restriction as arm32 there.
> >>>
> >>> Long term we should look at moving to a pfn compression based scheme 
> >>> similar
> >>> to x86, which removes the holes from the frametable.
> >>>
> >>> There were some bogus/outdated comments scattered around this code which I
> >>> have removed.
> >>>
> >>> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >>> Tested-by: Julien Grall <julien.grall@xxxxxxxxxx>
> >>> Acked-by: Julien Grall <julien.grall@xxxxxxxxxx>
> >>> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> >>> ---
> >>> v2: Rebase over "avoid truncation in mfn to paddr conversions"
> >>>
> >>> Freeze:
> >>>
> >>> The benefit of this series is that we can use the full 8GB of RAM on the
> >>> midway systems, rather than being limited to just the first 4GB (less I/O
> >>> holes). I expect it to be common that server class 32-bit arm systems will
> >>> have a hole in memory (between a bank <4GB and one above).
> >>>
> >>> The risk is that we regress on some other supported platform but AFAIK the
> >>> vexpress and sunxi only have a single memory bank and the arndale has
> >>> contiguous memory regions.
> >> If there were a bug in this patch, what would be the likely impact?
> >> Would the host not boot?  Would it only be able to access a part of
> >> its memory?
> > These are the two most likely outcomes of a bug. I can't say which one
> > is more likely for sure.
> >
> >> Or would it just allocate too much memory for a frametable?
> > This is unlikely since the restriction placed on the frametable size
> > relative to RAM size is deliberately pretty harsh to mitigate the risk
> > here.
> >
> >> The complexity of the patch looks middle to middle-high, just from the
> >> number of lines -- would you agree with that, or is the resulting code
> >> actually fairly simple and straightforward?
> > There's quite a few big comments ;-). (1/4 of the additions are
> > comments)
> >
> > The need to reindent the arm32 "/* Add non-xenheap memory */" loop
> > inside a new outer loop also adds quite a bit to the overall look of
> > complexity, plus I've created lots of temporary variables to try and
> > make the core logic as clear as possible.
> >
> > The real meat is the 1/2 dozen lines after the "If the new bank is
> > contiguous" comment in the arm32 case and the two lines after "We allow
> > non-contiguous regions " in the arm64 case. I think those pretty
> > unambiguously implement what is described in the commit log / comments.
> 
> It sounds like what you're saying is that the patch is more mid-low 
> complexity?

Yes.

> >> Does the fact that vexpress, sunxi, and arndale have single bank or
> >> contiguous memory regions mean that they will skip the complicated
> >> sections of code?  Or will they go through the complicated code and
> >> possibly trip over some bugs in spite of the fact that their layout is
> >> fairly simple?
> > There is some difference between the current code and the "simple fall
> > straight through" case after this patch but I don't think it is major.
> >
> > FWIW Julien has tested on Arndale (a multiple contiguous bank
> > configuration) and it is OK.
> 
> Right.  I'm inclined to classify "Can't access half of the ram" as a 
> kind of bug.  It's a bit less serious than "can't boot", but then again, 
> even "can't boot" is not a bad one to have as bugs go -- you know right 
> away that there's a problem, so it should be fairly easy to find a fix.

Agreed.

> So the benefit is fixing the "can't access half the ram" bug, at the 
> risk of introducing an easily discoverable and fixable "can't boot" bug 
> (or an equivalent "can't access all the ram" bug); and that risk is 
> fairly low, if your assessment of the complexity of the patch is accurate.
> 
> Unless someone has an objection, I think it should probably go in, then.
> 
> I guess that's a, "Release-ack-if-no-one-has-objected-in-a-day-or-so". :-)

Thanks, will-apply-tomorrow-unless-someone-objects,
Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel