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

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

On 19 November 2013 17:38, OndÅej BÃlka <neleai@seznam.cz> wrote:
> On Tue, Nov 19, 2013 at 01:02:27PM +0000, Will Newton wrote:
>> 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.
>>
> This is correct as we just moved this upstream from int_memalign so
> behaviour won't change. A question is if this could be dropped.

Ok, well the patch looks good to me. I guess at some point that code
should be investigated and probably replaced with an error return.

-- 
Will Newton
Toolchain Working Group, Linaro