This is the mail archive of the
mailing list for the glibc project.
Re: [PATCH][BZ #11087] Use atomic operations to track memory
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: OndÅej BÃlka <neleai at seznam dot cz>
- Cc: libc-alpha at sourceware dot org
- Date: Thu, 17 Oct 2013 15:20:52 -0400
- Subject: Re: [PATCH][BZ #11087] Use atomic operations to track memory
- Authentication-results: sourceware.org; auth=none
- References: <20131017114140 dot GA24230 at domone dot podge> <52602D5F dot 8010002 at redhat dot com> <20131017191552 dot GA5697 at domone dot podge>
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:
>>> 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.
>>> * 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 :-)