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

["Carlos O'Donell" <carlos at redhat dot com>]

On 10/17/2013 03:15 PM, OndÅej BÃlka wrote:
> 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.

Sorry, it's hard for a reviewer keep all the comments in order.
I apologize if I'm asking you to restate the same questions again.
  
>>> 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);
 
That works. I didn't know we had such a macro :-)

Cheers,
Carlos.