Re: [PATCH v2.1] Consolidate valloc/pvalloc code. large.

[Will Newton <will dot newton at linaro dot org>]

On 11 November 2013 11:07, OndÅej BÃlka <neleai@seznam.cz> wrote:
> On Mon, Nov 11, 2013 at 09:27:13AM +0000, Will Newton wrote:
>> On 8 November 2013 21:02, OndÅej BÃlka <neleai@seznam.cz> wrote:
>> > On Fri, Nov 08, 2013 at 10:14:15AM +0000, Will Newton wrote:
>> >> On 7 November 2013 21:14, OndÅej BÃlka <neleai@seznam.cz> wrote:
>> >> > Hi,
>> >> >
>> >> > As valloc/pvalloc are legacy functionality they should occupy minimal
>> >> > amount of code. Here we redirect valloc/pvalloc calls to memalign calls.
>> >> >
>> >> > This allows us simplify _int_memalign as several checks are already done
>> >> > is __libc_memalign.
>> >> >
>> >> > We could go further with refactoring as hook call is almost duplicated
>> >> > except that we need to define new function and pass caller address as
>> >> > argument.
>> >> >
>> >> > Comments?
>> >>
>> > I done a refactoring by introducing _mid_memalign which will call hook
>> > with caller passed as argument.
>> >
>> >> I am in favour of this refactor in principle. However you also need to
>> >> look at hooks.c as _int_memalign is called there too. You can test
>> >> that code by setting MALLOC_CHECK_=3 and running the tests.
>> >>
>> > I added same check for hook as in mid_memalign, rest of checks there are
>> > also duplicate.
>> >
>> > diff --git a/malloc/hooks.c b/malloc/hooks.c
>> > index 1dbe93f..12727ab 100644
>> > --- a/malloc/hooks.c
>> > +++ b/malloc/hooks.c
>> > @@ -376,6 +376,14 @@ memalign_check(size_t alignment, size_t bytes, const void *caller)
>> >        return 0;
>> >      }
>> >
>> > +  /* Make sure alignment is power of 2 (in case MINSIZE is not).  */
>> > +  if ((alignment & (alignment - 1)) != 0) {
>>
>> I guess this could use powerof2 for consistency.
>>
> I will run a coccinelle to find where this occurs.
>
>> >  static void* internal_function mem2mem_check(void *p, size_t sz);
>> > @@ -3002,15 +3002,23 @@ libc_hidden_def (__libc_realloc)
>> >  void*
>> >  __libc_memalign(size_t alignment, size_t bytes)
>> >  {
>> > +  void *address = RETURN_ADDRESS (0);
>> > +  return _mid_memalign (pagesz, bytes, address);
>> > +}
>>
>> This does not compile, pagesz is not defined, it should be alignment.
>>
> Thanks, I fixed it when I ran test but forgoten also fix mail.
>
>> >    if (alignment <= MALLOC_ALIGNMENT) return __libc_malloc(bytes);
>> >
>> >    /* Otherwise, ensure that it is at least a minimum chunk size */
>> > @@ -3031,6 +3039,15 @@ __libc_memalign(size_t alignment, size_t bytes)
>> >        return 0;
>> >      }
>> >
>> > +
>> > +  /* Make sure alignment is power of 2 (in case MINSIZE is not).  */
>>
>> The MINSIZE comment confuses me. AFAICT MINSIZE is not the issue (and
>> MINSIZE can statically be determined to be a power of 2).
>>
> Removed.
>
> Here is v2.1
>
>         * malloc/hooks.c (memalign_check): Add alignment rounding.
>         * malloc/malloc.c (_mid_memalign): New function.
>         (__libc_valloc, __libc_pvalloc, __libc_memalign, __posix_memalign):
>         Implement by calling _mid_memalign.
>         * manual/probes.texi (Memory Allocation Probes): Remove
>         memory_valloc_retry and memory_pvalloc_retry.
>
> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index 1dbe93f..89fbd3c 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -376,6 +376,13 @@ memalign_check(size_t alignment, size_t bytes, const void *caller)
>        return 0;
>      }
>
> +  /* Make sure alignment is power of 2.  */
> +  if (!powerof2(alignment)) {
> +    size_t a = MALLOC_ALIGNMENT * 2;
> +    while (a < alignment) a <<= 1;
> +    alignment = a;
> +  }
> +

Sorry for the delay in reviewing. I am wondering if this is the
correct behaviour. memalign is documented as requiring a power of two
alignment and I wonder if rounding up the alignment makes sense from
the caller's perspective. posix_memalign already checks this and
returns EINVAL and pvalloc/valloc guarantee power of two alignments by
design.

-- 
Will Newton
Toolchain Working Group, Linaro