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

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Fri, Oct 18, 2013 at 10:15:35AM +0200, OndÅej BÃlka wrote:
> Here is version that uses atomic* functions and tracks accurately
> maximum. OK to commit?
> 
> 	[BZ #11087]
> 	* malloc/malloc.c: Accurately track mmaped memory.

Wrong ChangeLog entry.  You need an entry for each function you touch.

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 1a18c3f..b814062 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -2325,12 +2325,11 @@ static void* sysmalloc(INTERNAL_SIZE_T nb, mstate av)
>  
>  	/* update statistics */
>  
> -	if (++mp_.n_mmaps > mp_.max_n_mmaps)
> -	  mp_.max_n_mmaps = mp_.n_mmaps;
> +	int old = atomic_exchange_and_add (&mp_.n_mmaps, 1);
> +	atomic_max (&mp_.max_n_mmaps, old + 1);
>  
> -	sum = mp_.mmapped_mem += size;
> -	if (sum > (unsigned long)(mp_.max_mmapped_mem))
> -	  mp_.max_mmapped_mem = sum;
> +	sum = atomic_exchange_and_add(&mp_.mmapped_mem, size) + size;
> +	atomic_max (&mp_.max_mmapped_mem, sum);
>  

Move the declaration of SUM down to where it's being used first.

I'll admit that this is a nit, but you could also use a consistent
style in the above two changes.  Use either:

  f = atomic_exchange_and_add (&g1, s);
  atomic_max (&g2, f + c)

or

  f = atomic_exchange_and_add (&g1, s) + c;
  atomic_max (&g2, f)

>  	check_chunk(av, p);
>  
> @@ -2780,8 +2779,8 @@ munmap_chunk(mchunkptr p)
>        return;
>      }
>  
> -  mp_.n_mmaps--;
> -  mp_.mmapped_mem -= total_size;
> +  atomic_decrement (&mp_.n_mmaps);
> +  atomic_add (&mp_.mmapped_mem, -total_size);
>  
>    /* If munmap failed the process virtual memory address space is in a
>       bad shape.  Just leave the block hanging around, the process will
> @@ -2798,6 +2797,7 @@ mremap_chunk(mchunkptr p, size_t new_size)
>    size_t page_mask = GLRO(dl_pagesize) - 1;
>    INTERNAL_SIZE_T offset = p->prev_size;
>    INTERNAL_SIZE_T size = chunksize(p);
> +  INTERNAL_SIZE_T old;

Delay this declaration to when it is going to be used...

>    char *cp;
>  
>    assert (chunk_is_mmapped(p));
> @@ -2822,10 +2822,8 @@ mremap_chunk(mchunkptr p, size_t new_size)
>    assert((p->prev_size == offset));
>    set_head(p, (new_size - offset)|IS_MMAPPED);
>  
> -  mp_.mmapped_mem -= size + offset;
> -  mp_.mmapped_mem += new_size;
> -  if ((unsigned long)mp_.mmapped_mem > (unsigned long)mp_.max_mmapped_mem)
> -    mp_.max_mmapped_mem = mp_.mmapped_mem;
> +  old = atomic_exchange_and_add (&mp_.mmapped_mem, new_size - size - offset);

... which is here.

> +  atomic_max (&mp_.max_mmapped_mem, old + new_size - size - offset);
>    return p;
>  }
>  

Siddhesh