From: Alexander Monakov <amonakov@ispras.ru>
To: musl@lists.openwall.com
Subject: Re: [PATCH 2/2] ldso, malloc: implement reclaim_gaps via __malloc_donate
Date: Tue, 17 Apr 2018 08:30:40 +0300 (MSK) [thread overview]
Message-ID: <alpine.LNX.2.20.13.1804170733330.29823@monopod.intra.ispras.ru> (raw)
In-Reply-To: <20180416223918.GA3094@brightrain.aerifal.cx>
> > +void __malloc_donate(char *start, char *end)
> > +{
> > + ssize_t align_start_up = (SIZE_ALIGN - 1) & -(uintptr_t)start;
> > + ssize_t align_end_down = (SIZE_ALIGN - 1) & (uintptr_t)end;
> > + ssize_t chunk_size = end - start - (OVERHEAD + align_start_up + align_end_down);
> > + if (chunk_size < OVERHEAD + SIZE_ALIGN) return;
> > + start += align_start_up;
> > + end -= align_end_down;
> > +
> > + struct chunk *c = MEM_TO_CHUNK(start + OVERHEAD), *n = MEM_TO_CHUNK(end);
> > + c->psize = n->csize = C_INUSE;
> > + c->csize = n->psize = C_INUSE | chunk_size;
> > + bin_chunk(c);
> > +}
>
> I think this version of the size logic is harder to read than the old
> one, and inconsistent with how malloc does accounting internally. In
> the notation used everywhere else, "chunk size" always includes
> OVERHEAD plus the usable space; it's the distance between the chunk
> header and the next chunk header.
Same here. In 'ssize_t chunk_size = ...', OVERHEAD is subtracted once to
account for the sentinel/footer; to compute usable size, OVERHEAD would
need to be subtracted twice.
> I also don't like use of signed arithmetic with sizes. I thought all
> the values were nonnegative anyway, so I wasn't sure why you
> introduced signed types, but apparently chunk_size can be negative and
> the comparison against OVERHEAD+SIZE_ALIGN relies on the RHS being
> signed (and on the unsigned result of the expression initializing
> chunk_size getting coerced to signed) to work.
Well, 'end - start' is a signed expression to begin with, so I doubt a
coercion is taking place there.
Is there a problem with assuming OVERHEAD+SIZE_ALIGN is signed?
> I think the above code may also be wrong. start is aligned mod
> SIZE_ALIGN, so start+OVERHEAD is misaligned, and therefore not a valid
> argument to MEM_TO_CHUNK. Continuing further along this line of
> reasoning, aligning start up mod SIZE_ALIGN like you're doing is not
> sufficient. There must also be space for a header below the aligned
> point.
Yes, that's an oversight, but easily corrected: we need to adjust 'start'
so it's congruent to -OVERHEAD (rather than 0) modulo SIZE_ALIGN:
void __malloc_donate(char *start, char *end)
{
ssize_t align_start_up = (SIZE_ALIGN - 1) & (-(uintptr_t)start - OVERHEAD);
ssize_t align_end_down = (SIZE_ALIGN - 1) & (uintptr_t)end;
ssize_t chunk_size = end - start - (OVERHEAD + align_start_up + align_end_down);
if (chunk_size < OVERHEAD + SIZE_ALIGN) return;
start += align_start_up;
end -= align_end_down;
struct chunk *c = MEM_TO_CHUNK(start + OVERHEAD), *n = MEM_TO_CHUNK(end);
c->psize = n->csize = C_INUSE;
c->csize = n->psize = C_INUSE | chunk_size;
bin_chunk(c);
}
The above addresses the alignment issue, and I've responded to other
concerns. Do you need a new patch with this?
Alexander
next prev parent reply other threads:[~2018-04-17 5:30 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-16 17:54 [PATCH 1/2] malloc: fix an over-allocation bug Alexander Monakov
2018-04-16 17:54 ` [PATCH 2/2] ldso, malloc: implement reclaim_gaps via __malloc_donate Alexander Monakov
2018-04-16 22:39 ` Rich Felker
2018-04-17 5:30 ` Alexander Monakov [this message]
2018-04-17 15:57 ` Rich Felker
2018-04-17 17:06 ` Rich Felker
2018-04-17 19:01 ` Rich Felker
2018-04-17 20:52 ` Alexander Monakov
2018-04-17 22:21 ` Rich Felker
2018-04-16 22:40 ` [PATCH 1/2] malloc: fix an over-allocation bug Rich Felker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LNX.2.20.13.1804170733330.29823@monopod.intra.ispras.ru \
--to=amonakov@ispras.ru \
--cc=musl@lists.openwall.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
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 NNTP newsgroup(s).