Bug #84

malloc changes

Added by kevin.kane over 9 years ago. Updated over 8 years ago.

Status:ClosedStart date:
Priority:LowDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

OpenBSD has alot of useful changes to its malloc.c so I decided to
pull over a bunch of that.

fetch http://www.uberstyle.net/malloc.patch

patch includes:
guard pages and chunk randomization
free page protection
malloc stats(dump)
pointer guard
no longer uses brk/sbrk
updated man page

affects the following files:
lib/libc/stdlib/malloc.c
lib/libc/stdlib/malloc.3
lib/libc/gen/err.c

in err.c i had to weak symbol warn. if you dont do that after you
compile and install the patched libc it will fail make buildworld in
lex.

-Kevin

History

#1 Updated by dillon over 9 years ago

:OpenBSD has alot of useful changes to its malloc.c so I decided to
:pull over a bunch of that.
:
:fetch http://www.uberstyle.net/malloc.patch
:
:patch includes:
:guard pages and chunk randomization
:free page protection
:malloc stats(dump)
:pointer guard
:no longer uses brk/sbrk
:updated man page
:
:affects the following files:
:lib/libc/stdlib/malloc.c
:lib/libc/stdlib/malloc.3
:lib/libc/gen/err.c
:
:in err.c i had to weak symbol warn. if you dont do that after you
:compile and install the patched libc it will fail make buildworld in
:lex.
:
:-Kevin

That's a pretty significant patch, how long did it take you to
integrate the work?

The patch looks ok to my quick perusal. I would like to get some
testing by other users, in particular with GUI programs like mozilla.
If that works out ok we can commit it.

-Matt
Matthew Dillon
<>

#2 Updated by kevin.kane over 9 years ago

It didn't take all that much time, i did the bulk of it over a few
days. I spent far more time compiling, testing and trying to figure
out why it wouldn't make buildworld after it was in place than I did
putting it together.

I've been running the patch for ~2 weeks now minus the weak reference
in err.c because that was the last gotcha that I had been working out.
I don't run X on my dfly machines at all so I havent done any GUI
testing.

-Kevin

On 2/2/06, Matthew Dillon <> wrote:
>
> :OpenBSD has alot of useful changes to its malloc.c so I decided to
> :pull over a bunch of that.
> :
> :fetch http://www.uberstyle.net/malloc.patch
> :
> :patch includes:
> :guard pages and chunk randomization
> :free page protection
> :malloc stats(dump)
> :pointer guard
> :no longer uses brk/sbrk
> :updated man page
> :
> :affects the following files:
> :lib/libc/stdlib/malloc.c
> :lib/libc/stdlib/malloc.3
> :lib/libc/gen/err.c
> :
> :in err.c i had to weak symbol warn. if you dont do that after you
> :compile and install the patched libc it will fail make buildworld in
> :lex.
> :
> :-Kevin
>
> That's a pretty significant patch, how long did it take you to
> integrate the work?
>
> The patch looks ok to my quick perusal. I would like to get some
> testing by other users, in particular with GUI programs like mozilla.
> If that works out ok we can commit it.
>
> -Matt
> Matthew Dillon
> <>
>

#3 Updated by wa1ter over 9 years ago

Matthew Dillon wrote:
[...]
> The patch looks ok to my quick perusal. I would like to get some
> testing by other users, in particular with GUI programs like mozilla.
> If that works out ok we can commit it.

Specifically what problems should we look for. Panics, memory leaks,
what else?

#4 Updated by emikulic over 9 years ago

On Thu, Feb 02, 2006 at 01:43:43PM -0800, walt wrote:
> Matthew Dillon wrote:
> [...]
> > The patch looks ok to my quick perusal. I would like to get some
> > testing by other users, in particular with GUI programs like mozilla.
> > If that works out ok we can commit it.
>
> Specifically what problems should we look for. Panics, memory leaks,
> what else?

This is the userland malloc(), so it's highly unlikely this would cause
any kernel panics, and it shouldn't cause leaks in programs that don't
already have them.

IIRC, when OpenBSD did this, they found a lot of problems with programs
that wrote past the end of a malloc'd buffer because their mmap malloc
puts in ElectricFence-esque guard pages.

So I guess what you're looking for is segfaults resulting from writes
outside of the malloc'd blocks.

--Emil

#6 Updated by kevin.kane over 9 years ago

this stuff is all optional and off by default, the default behavior of
this patch should be roughly equivalent to the old malloc except that
it doesnt use brk/sbrk... when you turn on guard pages and free page
protection(options G and F) you WILL see segfaults. I know xmms has
issues for sure.

-Kevin

On 2/2/06, Emil Mikulic <> wrote:
> A better explanation:
> http://article.gmane.org/gmane.os.openbsd.misc/87489
>

#7 Updated by dillon over 9 years ago

:
:this stuff is all optional and off by default, the default behavior of
:this patch should be roughly equivalent to the old malloc except that
:it doesnt use brk/sbrk... when you turn on guard pages and free page
:protection(options G and F) you WILL see segfaults. I know xmms has
:issues for sure.
:
:-Kevin
:
:On 2/2/06, Emil Mikulic <> wrote:
:> A better explanation:
:> http://article.gmane.org/gmane.os.openbsd.misc/87489

The main thing I'm looking for is that mainstream programs continue
to operate with default settings and that there is no serious
(or any) degredation in performance, again with default settings.

The debugging options the new code brings in look pretty interesting.

-Matt
Matthew Dillon
<>

#8 Updated by eng over 9 years ago

FYI,

FreeBSD just imported a new malloc to replace the aged
phkmalloc.

There was much back and forth with the author and David Xu
so he might have an opinion as to whether the changes in the
FreeBSD malloc are worth looking into.

The new malloc is not derived from the phkmalloc so likely
these changes would not apply. Many of the features are
similar, except the new jemalloc was engineered with true
concurrent threaded applications in mind.

#9 Updated by dillon over 9 years ago

:
:FYI,
:
:FreeBSD just imported a new malloc to replace the aged
:phkmalloc.
:
:There was much back and forth with the author and David Xu
:so he might have an opinion as to whether the changes in the
:FreeBSD malloc are worth looking into.
:
:The new malloc is not derived from the phkmalloc so likely
:these changes would not apply. Many of the features are
:similar, except the new jemalloc was engineered with true
:concurrent threaded applications in mind.

I've been following that thread in the FreeBSD groups.
We will definitely *NOT* going to be importing FreeBSD's
new malloc.

My feeling is that if a threaded program allocates so much
data that a fast MP-safe malloc is required, then we should
simply port the slab allocator from the DragonFly kernel to
userland and use that.

For now its not on the table. The current proposed malloc
changes for DragonFly IS on the table and I do want to see
it committed, but I'd appreciate it if some GUI users could
test it out with e.g. mozilla, X, and so forth, first.

-Matt
Matthew Dillon
<>

#10 Updated by drhodus over 9 years ago

On 2/3/06, Matthew Dillon <> wrote:
>
> :
> :FYI,
> :
> :FreeBSD just imported a new malloc to replace the aged
> :phkmalloc.
> :
> :There was much back and forth with the author and David Xu
> :so he might have an opinion as to whether the changes in the
> :FreeBSD malloc are worth looking into.
> :
> :The new malloc is not derived from the phkmalloc so likely
> :these changes would not apply. Many of the features are
> :similar, except the new jemalloc was engineered with true
> :concurrent threaded applications in mind.
>
> I've been following that thread in the FreeBSD groups.
> We will definitely *NOT* going to be importing FreeBSD's
> new malloc.
>
> My feeling is that if a threaded program allocates so much
> data that a fast MP-safe malloc is required, then we should
> simply port the slab allocator from the DragonFly kernel to
> userland and use that.
>
> For now its not on the table. The current proposed malloc
> changes for DragonFly IS on the table and I do want to see
> it committed, but I'd appreciate it if some GUI users could
> test it out with e.g. mozilla, X, and so forth, first.
>
> -Matt

I haven't been able to get a pkgsrc bulk build to complete using the
malloc patch.

-DR

#11 Updated by kevin.kane over 9 years ago

Looking into this now, hopefully ill have a updated patch soonish.

-Kevin

On 2/3/06, David Rhodus <> wrote:
> On 2/3/06, Matthew Dillon <> wrote:
> >
> > :
> > :FYI,
> > :
> > :FreeBSD just imported a new malloc to replace the aged
> > :phkmalloc.
> > :
> > :There was much back and forth with the author and David Xu
> > :so he might have an opinion as to whether the changes in the
> > :FreeBSD malloc are worth looking into.
> > :
> > :The new malloc is not derived from the phkmalloc so likely
> > :these changes would not apply. Many of the features are
> > :similar, except the new jemalloc was engineered with true
> > :concurrent threaded applications in mind.
> >
> > I've been following that thread in the FreeBSD groups.
> > We will definitely *NOT* going to be importing FreeBSD's
> > new malloc.
> >
> > My feeling is that if a threaded program allocates so much
> > data that a fast MP-safe malloc is required, then we should
> > simply port the slab allocator from the DragonFly kernel to
> > userland and use that.
> >
> > For now its not on the table. The current proposed malloc
> > changes for DragonFly IS on the table and I do want to see
> > it committed, but I'd appreciate it if some GUI users could
> > test it out with e.g. mozilla, X, and so forth, first.
> >
> > -Matt
>
> I haven't been able to get a pkgsrc bulk build to complete using the
> malloc patch.
>
> -DR
>
>

#12 Updated by dillon over 9 years ago

:Looking into this now, hopefully ill have a updated patch soonish.
:
:-Kevin
:...
:>
:> I haven't been able to get a pkgsrc bulk build to complete using the
:> malloc patch.
:>
:> -DR

Lets try to get more info for Kevin there. What exactly failed, DR ?
could you throw the log onto a web site somewhere ?

-Matt
Matthew Dillon
<>

#13 Updated by wa1ter over 9 years ago

Matthew Dillon wrote:
> :Looking into this now, hopefully ill have a updated patch soonish.
> :
> :-Kevin
> :...
> :>
> :> I haven't been able to get a pkgsrc bulk build to complete using the
> :> malloc patch.
> :>
> :> -DR
>
> Lets try to get more info for Kevin there. What exactly failed, DR ?
> could you throw the log onto a web site somewhere ?

While trying to build the clisp package just now I saw this error
during the 'configure' stage:

checking for working mprotect... Bus error (core dumped)

I haven't actually looked at 'configure' yet to see what code
it was trying to run, but this could be related, maybe?

#14 Updated by kevin.kane over 9 years ago

I was able to build the clisp package fine with the patch.

Do you have any of the options enabled system wide using /etc/malloc.conf?

-Kevin

On 2/6/06, walt <> wrote:
> Matthew Dillon wrote:
> > :Looking into this now, hopefully ill have a updated patch soonish.
> > :
> > :-Kevin
> > :...
> > :>
> > :> I haven't been able to get a pkgsrc bulk build to complete using the
> > :> malloc patch.
> > :>
> > :> -DR
> >
> > Lets try to get more info for Kevin there. What exactly failed, DR ?
> > could you throw the log onto a web site somewhere ?
>
> While trying to build the clisp package just now I saw this error
> during the 'configure' stage:
>
> checking for working mprotect... Bus error (core dumped)
>
> I haven't actually looked at 'configure' yet to see what code
> it was trying to run, but this could be related, maybe?
>

#15 Updated by wa1ter over 9 years ago

Kevin L. Kane wrote:
> I was able to build the clisp package fine with the patch.
>
> Do you have any of the options enabled system wide using /etc/malloc.conf?

No, I have no /etc/malloc.conf. I should mention that the error
did *not* stop the clisp package from building. I noticed the
error just by chance because I was watching the screen when it
flashed by. I went back and used 'script' to capture the error
message during a rebuild so I could post it here.

#16 Updated by drhodus over 9 years ago

On 2/6/06, Matthew Dillon <> wrote:
>
> :Looking into this now, hopefully ill have a updated patch soonish.
> :
> :-Kevin
> :...
> :>
> :> I haven't been able to get a pkgsrc bulk build to complete using the
> :> malloc patch.
> :>
> :> -DR
>
> Lets try to get more info for Kevin there. What exactly failed, DR ?
> could you throw the log onto a web site somewhere ?
>
> -Matt

:
# ln -s GF /etc/malloc.conf
# cd /usr/pkgsrc/lang/perl5
# bmake

Buildlog : fetch http://machdep.com/x

-DR

#17 Updated by dillon over 9 years ago

::
:# ln -s GF /etc/malloc.conf
:# cd /usr/pkgsrc/lang/perl5
:# bmake
:
:Buildlog : fetch http://machdep.com/x
:
:-DR

What happens if you try to compile up that program manually? The one
printed out in the log file? The cc line being reported is not right,
but I'm not sure that is the cause of the problem. When -pthread is
used, -lc_r should not also be specified (since -pthread includes it
automatically).

-Matt
Matthew Dillon
<>

#18 Updated by kevin.kane over 9 years ago

I was playing around with this and if you don't turn on option G
system wide you don't have a problem(perl5 compiles and runs fine) I
compiled up the program mentioned in the log manually and it has
problems when G is enabled system wide. Turn G off, works fine. I
tried it without -lc_r option and it has the same behavior.

-Kevin

On 2/7/06, Matthew Dillon <> wrote:
>
> ::
> :# ln -s GF /etc/malloc.conf
> :# cd /usr/pkgsrc/lang/perl5
> :# bmake
> :
> :Buildlog : fetch http://machdep.com/x
> :
> :-DR
>
> What happens if you try to compile up that program manually? The one
> printed out in the log file? The cc line being reported is not right,
> but I'm not sure that is the cause of the problem. When -pthread is
> used, -lc_r should not also be specified (since -pthread includes it
> automatically).
>
> -Matt
> Matthew Dillon
> <>
>

#19 Updated by dillon over 9 years ago

:I was playing around with this and if you don't turn on option G
:system wide you don't have a problem(perl5 compiles and runs fine) I
:compiled up the program mentioned in the log manually and it has
:problems when G is enabled system wide. Turn G off, works fine. I
:tried it without -lc_r option and it has the same behavior.
:
:-Kevin

Ok, this is looking better. DR verified that the failure he reported
was with 'G'. DR is also running a desktop now with no problems.

The patch is looking really good at this point. Lets give it until
thursday and if there are no reports of problems we will commit it.

-Matt
Matthew Dillon
<>

#20 Updated by dillon over 9 years ago

I am doing some final buildworld testing on this patch and if it
works, which it almost certainly will, I will commit it today!

-Matt

#21 Updated by joerg over 9 years ago

On Sun, Feb 12, 2006 at 11:35:41AM -0800, Matthew Dillon wrote:
> I am doing some final buildworld testing on this patch and if it
> works, which it almost certainly will, I will commit it today!

I want to see at least a basic measurements first. This is very likely
to increase both the runtime and memory foot print of programs and the
argument "OpenBSD did it" is not a good justification for both.

Joerg

#22 Updated by dillon over 9 years ago

:I want to see at least a basic measurements first. This is very likely
:to increase both the runtime and memory foot print of programs and the
:argument "OpenBSD did it" is not a good justification for both.
:
:Joerg

Well, you are welcome to make such measurements but I don't see anything
in the patch that would increase memory footprints, since the old code
was sbrk()ing memory in multiples of PAGE_SIZE and the new code
effectively does the same thing, just with mmap() instead of sbrk().

There is some potential for a more fragmented VM map for processes
which mix malloc()'s and their own file mmap()'s, but that's about it.
Sans a process doing its own mmap()ing, the effect on the VM map will
be the same.

I will do a /usr/bin/time -l on a buildworld before and after the patch
to make sure that there are no severe differences. I doubt I'll see
any.

-Matt
Matthew Dillon
<>

#23 Updated by joerg over 9 years ago

On Sun, Feb 12, 2006 at 12:06:31PM -0800, Matthew Dillon wrote:
>
> :I want to see at least a basic measurements first. This is very likely
> :to increase both the runtime and memory foot print of programs and the
> :argument "OpenBSD did it" is not a good justification for both.
> :
> :Joerg
>
> Well, you are welcome to make such measurements but I don't see anything
> in the patch that would increase memory footprints, since the old code
> was sbrk()ing memory in multiples of PAGE_SIZE and the new code
> effectively does the same thing, just with mmap() instead of sbrk().

You mean like the guard pages?

Joerg

#24 Updated by dillon over 9 years ago

:You mean like the guard pages?
:
:Joerg

Those are not enabled by default.

-Matt
Matthew Dillon
<>

Also available in: Atom PDF