Re: [PATCH][BZ #11087] Use atomic operations to track memory

[OndÅej BÃlka <neleai at seznam dot cz>]

On Thu, Oct 17, 2013 at 02:33:03PM -0400, Carlos O'Donell wrote:
> On 10/17/2013 07:41 AM, OndÅej BÃlka wrote:
> > Hi,
> > 
> > I fixed this mostly because ulrich was wrong here in several ways. 
> > 
> > Calling added locking to update statistics as too expensive is nonsense
> > as this is needed only after mmap and mmap + associated minor faults
> > are much more costy.
> > 
> > Also there is no locking needed, atomic add will do job well.
> > 
> > This bug affects also malloc_stats.
> > 
> > Comments?
> > 
> > 	* malloc/malloc.c: Accurately track mmaped memory.
> 
> While this patch fixes the consistency problems with the variables
> in question it doesn't fix concurrent stores of slightly different
> values to the same variables.
>
I asked that in patch for 12486 how this should be handled.
 
> > diff --git a/malloc/malloc.c b/malloc/malloc.c
> > index 2938234..cdbd6f3 100644
> > --- a/malloc/malloc.c
> > +++ b/malloc/malloc.c
> > @@ -2334,10 +2334,11 @@ static void* sysmalloc(INTERNAL_SIZE_T nb, mstate av)
> >  
> >  	/* update statistics */
> >  
> > -	if (++mp_.n_mmaps > mp_.max_n_mmaps)
> > +	__sync_fetch_and_add (&mp_.n_mmaps, 1);
> > +	if (mp_.n_mmaps > mp_.max_n_mmaps)
> >  	  mp_.max_n_mmaps = mp_.n_mmaps;
> 
> Don't two threads race to update mp_.max_n_mmaps with potentially
> different values?
> 
What about replacing this by

atomic_max (&mp_.n_mmaps, mp_.max_n_mmaps);