"Dr. David Alan Gilbert" <dgilb...@redhat.com> writes:
> * Markus Armbruster (arm...@redhat.com) wrote:
>> In general, code running withing a realize() method should not exit() on
>> error. Instad, errors should be propagated through the realize()
>> method. Additionally, the realize() method should fail cleanly,
>> i.e. carefully undo its side effects such as wiring of interrupts,
>> mapping of memory, and so forth. Tedious work, but necessary to make
>> hot plug safe.
[...]
>> Next, let's consider the special case "out of memory".
>>
>> Our general approach is to treat it as immediately fatal. This makes
>> sense, because when a smallish allocation fails, the process is almost
>> certainly doomed anyway. Moreover, memory allocation is frequent, and
>> attempting to recover from failed memory allocation adds loads of
>> hard-to-test error paths. These are *dangerous* unless carefully tested
>> (and we don't).
>>
>> Certain important allocations we handle more gracefully. For instance,
>> we don't want to die when the user tries to hot-plug more memory than we
>> can allocate, or tries to open a QCOW2 image with a huge L1 table.
>>
>> Guest memory allocation used to have the "immediately fatal" policy
>> baked in at a fairly low level, but it's since been lifted into callers;
>> see commit c261d77..fc7a580 and fixups 4f96676..0bdaa3a. During review
>> of the latter, Peter Crosthwaite called out the &error_fatal in the
>> realize methods and their supporting code. I agreed with him back then
>> that the errors should really be propagated. But now I've changed my
>> mind: I think we should treat these memory allocation failures like
>> we've always treated them, namely report and exit(1). Except for
>> "large" allocations, where we have a higher probability of failure, and
>> a more realistic chance to recover safely.
>>
>> Can we agree that passing &error_fatal to memory_region_init_ram() &
>> friends is basically okay even in realize() methods and their supporting
>> code?
>
> I'd say it depends if they can be hotplugged; I think anything that we really
> want to hotplug onto real running machines (as opposed to where we're just
> hotplugging during setup) we should propagate errors properly.
>
> And tbh I don't buy the small allocation argument; I think we should handle
> them
> all; in my utopian world a guest wouldn't die unless there was no way out.
I guess in Utopia nobody ever makes stupid coding mistakes, the error
paths are all covered by a comprehensive test suite, and they make the
code prettier, too. Oh, and kids always eat their vegetables without
complaint.
However, we don't actually live in Utopia. In our world, error paths
clutter the code, are full of bugs, and the histogram of their execution
counts in testing (automated or not) has a frighteningly tall bar at
zero.
We're not going to make this problem less severe by making it bigger.
In fact, we consciously decided to hack off a big chunk with an axe:
commit 8a1d02aba9f986ca03d854184cd432ee98bcd179
Author: aliguori <aliguori@c046a42c-6fe2-441c-8c8c-71466251a162>
Date: Thu Feb 5 22:05:49 2009 +0000
Terminate emulation on memory allocation failure (Avi Kivity)
Memory allocation failures are a very rare condition on virtual-memory
hosts. They are also very difficult to handle correctly (especially in a
hardware emulation context). Because of this, it is better to gracefully
terminate emulation rather than executing untested or even unwritten
recovery
code paths.
This patch changes the qemu memory allocation routines to terminate
emulation
if an allocation failure is encountered.
Signed-off-by: Avi Kivity <a...@redhat.com>
Signed-off-by: Anthony Liguori <aligu...@us.ibm.com>
git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@6526
c046a42c-6fe2-441c-8c8c-71466251a162
Let me elaborate a bit on Avi's arguments:
* Memory allocations are very, very common. I count at least 2500,
Memory allocation failure is easily the most common *potential* error,
both statically and dynamically.
* Error recovery is always tedious and often hard. Especially when the
error happens in the middle of a complex operation that needs to be
fully rolled back to recover. A sensible approach is to acquire
resources early, when recovery is still relatively easy, but that's
often impractical for memory. This together with the ubiquitousness
of memory allocation makes memory allocation failure even harder to
handle than other kinds of errors.
* Not all allocations are equal. When an attempt to allocate a gigabyte
fails gracefully, there's a good chance that ordinary code can go on
allocating and freeing kilobytes as usual. But when an attempt to
allocate kilobytes fails, it's very likely that handling this failure
gracefully will only lead to another one, and another one, until some
buggy error handling puts the doomed process out of its misery.
Out of the ~2400 memory allocations that go through GLib, 59 can fail.
The others all terminate the process.
* How often do we see failures from these other 2300+? Bug reports from
users? As far as I can see, they're vanishingly rare.
* Reviewing and testing the error handling chains rooted at 59
allocations is hard enough, and I don't think we're doing particularly
well on the testing. What chance would we have with 2300+ more?
2300+ instances of a vanishingly rare error with generally complex
error handling and basically no test coverage: does that sound more
useful than 2300+ instances of a vanishingly rare error that kills the
process? If yes, how much of our limited resources is the difference
worth?
* You might argue that we don't have to handle all 2300+ instances, only
the ones reachable from hotplug. I think that's a pipe dream. Once
you permit "terminate on memory allocation failure" in general purpose
code, it hides behind innocent-looking function calls. Even if we
could find them all, we'd still have to add memory allocation failure
handling to lots of general purpose code just to make it usable for
hot plug. And then we'd get to keep finding them all forever.
I don't think handling all memory allocation failures gracefully
everywhere or even just within hotplug is practical this side of Utopia.
I believe all we could achieve trying is an illusion of graceful
handling that is sufficiently convincing to let us pat on our backs,
call the job done, and go back to working on stuff that matters to
actual users.
My current working assumption is that passing &error_fatal to
memory_region_init_ram() & friends is okay even in realize() methods and
their supporting code, except when the allocation can be large. Even
then, &error_fatal is better than buggy recovery code (which I can see
all over the place, but that's a separate topic).
