* [patch v1] malloc: fix set_max_fast "impossibly small" value
@ 2019-10-30 22:11 DJ Delorie
2019-10-31 2:35 ` Carlos O'Donell
0 siblings, 1 reply; 5+ messages in thread
From: DJ Delorie @ 2019-10-30 22:11 UTC (permalink / raw)
To: libc-alpha
From 83035919da9208a7e6666bdde60e110e2e301553 Mon Sep 17 00:00:00 2001
From: DJ Delorie <dj@redhat.com>
Date: Wed, 30 Oct 2019 18:03:14 -0400
Subject: Base max_fast on alignment, not width, of bins
set_max_fast set the "impossibly small" value based on,
eventually, MALLOC_ALIGNMENT. The comparisons for the smallest
chunk used, eventuall, MIN_CHUNK_SIZE. Note that i386
is the only platform where these are the same, so a smallest
chunk *would* be put in a no-fastbins fastbin.
This change calculates the "impossibly small" value
based on MIN_CHUNK_SIZE instead, so that we can know it will
always be impossibly small.
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 5d3e82a8f6..70cc35a473 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -1621,7 +1621,7 @@ static INTERNAL_SIZE_T global_max_fast;
#define set_max_fast(s) \
global_max_fast = (((s) == 0) \
- ? SMALLBIN_WIDTH : ((s + SIZE_SZ) & ~MALLOC_ALIGN_MASK))
+ ? MIN_CHUNK_SIZE / 2 : ((s + SIZE_SZ) & ~MALLOC_ALIGN_MASK))
static inline INTERNAL_SIZE_T
get_max_fast (void)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch v1] malloc: fix set_max_fast "impossibly small" value
2019-10-30 22:11 [patch v1] malloc: fix set_max_fast "impossibly small" value DJ Delorie
@ 2019-10-31 2:35 ` Carlos O'Donell
2019-10-31 3:09 ` DJ Delorie
2019-10-31 16:57 ` Joseph Myers
0 siblings, 2 replies; 5+ messages in thread
From: Carlos O'Donell @ 2019-10-31 2:35 UTC (permalink / raw)
To: DJ Delorie, libc-alpha
On 10/30/19 6:11 PM, DJ Delorie wrote:
>
> From 83035919da9208a7e6666bdde60e110e2e301553 Mon Sep 17 00:00:00 2001
> From: DJ Delorie <dj@redhat.com>
> Date: Wed, 30 Oct 2019 18:03:14 -0400
> Subject: Base max_fast on alignment, not width, of bins
Does this fix bug 24903? If it does please add "(Bug 24903)" to the
first line of your git commit.
I assume this message is your git format-patch HEAD~1 output.
OK for master with the following fixed:
- Add "(Bug 24903)" to the git commit first line.
- Apply both commit message changes.
> set_max_fast set the "impossibly small" value based on,
s/set/sets/g
> eventually, MALLOC_ALIGNMENT. The comparisons for the smallest
> chunk used, eventuall, MIN_CHUNK_SIZE. Note that i386
s/, eventuall/is, eventually/g
> is the only platform where these are the same, so a smallest
> chunk *would* be put in a no-fastbins fastbin.
>
> This change calculates the "impossibly small" value
> based on MIN_CHUNK_SIZE instead, so that we can know it will
> always be impossibly small.
>
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 5d3e82a8f6..70cc35a473 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1621,7 +1621,7 @@ static INTERNAL_SIZE_T global_max_fast;
>
> #define set_max_fast(s) \
> global_max_fast = (((s) == 0) \
> - ? SMALLBIN_WIDTH : ((s + SIZE_SZ) & ~MALLOC_ALIGN_MASK))
> + ? MIN_CHUNK_SIZE / 2 : ((s + SIZE_SZ) & ~MALLOC_ALIGN_MASK))
OK.
My analysis:
SMALLBIN_WIDTH is MALLOC_ALIGNMENT:
1403 #define SMALLBIN_WIDTH MALLOC_ALIGNMENT
Which on i386 is overriden here:
sysdeps/i386/malloc-alignment.h:
22 #define MALLOC_ALIGNMENT 16
This is because we have to account for 16-byte aligned vector types.
This means SMALLBIN_WIDTH is 16, which for 32-bit targets is an aligned
and perfectly viable chunk size.
1178 /* The smallest possible chunk */
1179 #define MIN_CHUNK_SIZE (offsetof(struct malloc_chunk, fd_nextsize))
This is because MIN_CHUNK_SIZE is:
1048 struct malloc_chunk {
1049
1050 INTERNAL_SIZE_T mchunk_prev_size; /* Size of previous chunk (if free). */
1051 INTERNAL_SIZE_T mchunk_size; /* Size in bytes, including overhead. */
1052
1053 struct malloc_chunk* fd; /* double links -- used only if free. */
1054 struct malloc_chunk* bk;
1055
1056 /* Only used for large blocks: pointer to next larger size. */
1057 struct malloc_chunk* fd_nextsize; /* double links -- used only if free. */
1058 struct malloc_chunk* bk_nextsize;
1059 };
The offsetof returns 16-bytes.
On x86_64 we have SMALLBIN_WIDTH of 16, but the MIN_CHUNK_SIZE is 32, so it works.
It's the increase in MALLOC_ALIGNMENT to 16 for i686 which breaks this.
Your solution of MIN_CHUNK_SIZE/2 works.
On i686 it yields 8, a size that is too small.
On x86_64 it yields 16, a size that is too small.
Thus it looks like the new calculation in set_max_fast yields an impossibly small
value that we can use that no result of request2size() will return.
>
> static inline INTERNAL_SIZE_T
> get_max_fast (void)
>
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch v1] malloc: fix set_max_fast "impossibly small" value
2019-10-31 2:35 ` Carlos O'Donell
@ 2019-10-31 3:09 ` DJ Delorie
2019-10-31 16:57 ` Joseph Myers
1 sibling, 0 replies; 5+ messages in thread
From: DJ Delorie @ 2019-10-31 3:09 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: libc-alpha
"Carlos O'Donell" <carlos@redhat.com> writes:
> Does this fix bug 24903? If it does please add "(Bug 24903)" to the
> first line of your git commit.
Yes and done.
> I assume this message is your git format-patch HEAD~1 output.
Yes, that way everyone can see exactly what the commit is :-)
> OK for master with the following fixed:
> - Add "(Bug 24903)" to the git commit first line.
> - Apply both commit message changes.
Done and pushed!
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch v1] malloc: fix set_max_fast "impossibly small" value
2019-10-31 2:35 ` Carlos O'Donell
2019-10-31 3:09 ` DJ Delorie
@ 2019-10-31 16:57 ` Joseph Myers
2019-11-04 15:07 ` Carlos O'Donell
1 sibling, 1 reply; 5+ messages in thread
From: Joseph Myers @ 2019-10-31 16:57 UTC (permalink / raw)
To: Carlos O'Donell; +Cc: DJ Delorie, libc-alpha
On Wed, 30 Oct 2019, Carlos O'Donell wrote:
> Which on i386 is overriden here:
> sysdeps/i386/malloc-alignment.h:
> 22 #define MALLOC_ALIGNMENT 16
>
> This is because we have to account for 16-byte aligned vector types.
Rather, to account for 16-byte-aligned _Float128 and _Decimal128. Vector
types are expected to require aligned_alloc, but any type that's a basic
type as defined in ISO C has a fundamental alignment and so malloc must
allocate memory suitably aligned for it.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch v1] malloc: fix set_max_fast "impossibly small" value
2019-10-31 16:57 ` Joseph Myers
@ 2019-11-04 15:07 ` Carlos O'Donell
0 siblings, 0 replies; 5+ messages in thread
From: Carlos O'Donell @ 2019-11-04 15:07 UTC (permalink / raw)
To: Joseph Myers; +Cc: DJ Delorie, libc-alpha
On 10/31/19 12:56 PM, Joseph Myers wrote:
> On Wed, 30 Oct 2019, Carlos O'Donell wrote:
>
>> Which on i386 is overriden here:
>> sysdeps/i386/malloc-alignment.h:
>> 22 #define MALLOC_ALIGNMENT 16
>>
>> This is because we have to account for 16-byte aligned vector types.
>
> Rather, to account for 16-byte-aligned _Float128 and _Decimal128. Vector
> types are expected to require aligned_alloc, but any type that's a basic
> type as defined in ISO C has a fundamental alignment and so malloc must
> allocate memory suitably aligned for it.
Thanks for the clarification.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-11-04 15:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-30 22:11 [patch v1] malloc: fix set_max_fast "impossibly small" value DJ Delorie
2019-10-31 2:35 ` Carlos O'Donell
2019-10-31 3:09 ` DJ Delorie
2019-10-31 16:57 ` Joseph Myers
2019-11-04 15:07 ` Carlos O'Donell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).