[PATCH v2 1/5] malloc: Split _int_free() into 3 sub functions

[Florian Weimer]

* Wangyang Guo:

> Split _int_free() into 3 smaller functions for flexible combination:
> * _int_free_check -- sanity check for free
> * tcache_free -- free memory to tcache (quick path)
> * _int_free_chunk -- free memory chunk (slow path)
>
> Signed-off-by: Wangyang Guo <wangyang.guo@intel.com>

Is this patch submission covered by Intel's copyright assignment?  If it
is, please drop the Signed-off-by:.

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index bcb6e5b83c..b2373b2212 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1086,7 +1086,9 @@ typedef struct malloc_chunk* mchunkptr;
>  /* Internal routines.  */
>  
>  static void*  _int_malloc(mstate, size_t);
> -static void     _int_free(mstate, mchunkptr, int);
> +static void _int_free (mstate, mchunkptr, int);
> +static void _int_free_check (mstate, mchunkptr, INTERNAL_SIZE_T);
> +static void _int_free_chunk (mstate, mchunkptr, INTERNAL_SIZE_T, int);
>  static void _int_free_merge_chunk (mstate, mchunkptr, INTERNAL_SIZE_T);
>  static INTERNAL_SIZE_T _int_free_create_chunk (mstate,
>  					       mchunkptr, INTERNAL_SIZE_T,
> @@ -3206,6 +3208,49 @@ tcache_next (tcache_entry *e)
>    return (tcache_entry *) REVEAL_PTR (e->next);
>  }
>  
> +static inline bool
> +tcache_free (mchunkptr p, INTERNAL_SIZE_T size)

Please add a function comment regarding preconditions.

The function probably needs to be wrapped in #if USE_TCACHE.
Or maybe we should just make tcache unconditional in a first cleanup
patch (basically run unifdef -m -UUSE_TCACHE malloc/{malloc,arena}.c).

> +{
> +  bool done = false;
> +  size_t tc_idx = csize2tidx (size);
> +  if (tcache != NULL && tc_idx < mp_.tcache_bins)
> +    {
> +      /* Check to see if it's already in the tcache.  */
> +      tcache_entry *e = (tcache_entry *) chunk2mem (p);
> +
> +      /* This test succeeds on double free.  However, we don't 100%
> +	 trust it (it also matches random payload data at a 1 in
> +	 2^<size_t> chance), so verify it's not an unlikely
> +	 coincidence before aborting.  */
> +      if (__glibc_unlikely (e->key == tcache_key))
> +	{

I wonder if it's beneficial to move the code in the body of the if
statement into a separate noinline function, so that it doesn't get
inlined.  It's not expected to execute very often.  Using __COLD is
probably too much (because it can run).

> +static void
> +_int_free_chunk (mstate av, mchunkptr p, INTERNAL_SIZE_T size, int have_lock)
> +{
> +  mfastbinptr *fb;             /* associated fastbin */


Maybe add a function comment here as well.

>      If eligible, place chunk on a fastbin so it can be found
> @@ -4657,6 +4662,23 @@ _int_free (mstate av, mchunkptr p, int have_lock)
>    }
>  }
>  
> +static void
> +_int_free (mstate av, mchunkptr p, int have_lock)
> +{

That function is exected before, so the missing comment is kind of
expected, but if you could add one, that would be stellar.

I checked the refactoring and it looks okay to me, except for the
USE_TCACHE issue.

Thanks,
Florian