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

[OndÅej BÃlka <neleai at seznam dot cz>]

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.