[ping][PATCH v2] Don't fall back to mmap if the original arena is not corrupt

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

Ping!

On Mon, Aug 24, 2015 at 12:45:07PM +0530, Siddhesh Poyarekar wrote:
> On Wed, Aug 19, 2015 at 02:53:06PM -0400, Mike Frysinger wrote:
> > i'm not sure this fixes the stated bug.  say you have two arenas:
> > 	a -> b -> a
> > b is corrupt and a is avoided.
> > 	avoid_arena = a;
> > 	result = a;
> > 	begin = a;
> > 
> > so the first loop runs because result == avoid_arena, so we move result=b,
> > and then the second loop runs because arena_is_corrupt(b), so we move
> > result=a, and then we hit the if statement which sets looped=true, and then
> > we break out.  then we return NULL even though there is a non-corrupt region.
> > 
> > looks like this applies whenever the first region is avoided and the others
> > are corrupt.
> 
> Thanks for pointing that out, but it appears to me that this problem
> is only valid when none of the arenas are corrupt and no arenas are
> avoided.  One would not really care about performance loss when any of
> the arenas are corrupt because it will just be a matter of time before
> the program dies.  There is of course the case of:
> 
> a -> a where one could fall back to mmap, but that's OK for two reasons:
> 
> 1. It is unlikely because the code will always attempt to create
>    another arena before going down the retry path, so you'd at least
>    end up with a -> b -> a.  In the unlikely case that creating the
>    arena fails, we don't fall back to reusing anyway, and we just use
>    mmap
> 
> 2. Even if we ever end up with the a -> a case, it's OK to fall back
>    because we don't really care about retrying in the same map that we
>    failed, so I'd argue that the original code was wrong in returning
>    the same arena.
> 
> This also means that my original assertion about the "degenerate case
> being only one arena" is wrong; there is no such degenerate case.
> Also, the code around this is a bit murky because of some useless
> bits.  I'll try to clean them up in follow-up patches.
> 
> Is the explanation sound enough for me to commit this?  I can add
> comments in the code to explain this in detail if you think it'll
> help.
> 
> Siddhesh

Attachment: pgp8cvGTP1pPf.pgp
Description: PGP signature