[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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?


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.

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". :-)

 -George

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.