Re: [Qemu-devel] Error handling in realize() methods

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * Markus Armbruster (address@hidden) 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 <address@hidden>
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 <address@hidden>
    Signed-off-by: Anthony Liguori <address@hidden>
    
    
    git-svn-id: svn://svn.savannah.nongnu.org/qemu/address@hidden 
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).