This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2.1] Consolidate valloc/pvalloc code. large.
- From: OndÅej BÃlka <neleai at seznam dot cz>
- To: Will Newton <will dot newton at linaro dot org>
- Cc: libc-alpha <libc-alpha at sourceware dot org>
- Date: Tue, 19 Nov 2013 18:38:17 +0100
- Subject: Re: [PATCH v2.1] Consolidate valloc/pvalloc code. large.
- Authentication-results: sourceware.org; auth=none
- References: <20131107204959 dot GA21537 at domone> <20131107211406 dot GA22108 at domone> <CANu=Dmh86w91DjZGNa-jKN97W7478ciypCCUygo7LjBnz2hUug at mail dot gmail dot com> <20131108210228 dot GA4867 at domone> <CANu=DmhsGS0PZdpkNjx5UZ--noHN5QH7_xLHzfx2o4vJCeD5GA at mail dot gmail dot com> <20131111110756 dot GA10558 at domone dot podge> <CANu=DmjaqqZQ=HKReXG00pxvUJJXjFU-XDkKrFby89btvSALxA at mail dot gmail dot com>
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.