Re: multi-pool malloc wip diff

[Otto Moerbeek]

On Tue, Aug 23, 2016 at 09:57:03AM +0200, Otto Moerbeek wrote:

> On Mon, Aug 22, 2016 at 08:59:44PM -0400, Ted Unangst wrote:
> 
> > Otto Moerbeek wrote:
> > > 
> > > After a forkl of a threaded program __isthreaded is reset, but
> > > existing allocations are spread around the pools. But the new single
> > > threaded child only looks in the first pool. I have to think how to
> > > solve this.
> > 
> > Create a new flag, _malloc_threads. Init to 0.
> > 
> > Change all existing tests for __isthreaded in malloc.c to _malloc_threads.
> > 
> > Set _malloc_threads to 1 (or N) in malloc_init().
> > 
> > (Future direction may be to replace _MALLOC_MUTEXES with _malloc_threads and
> > allow it to vary somewhat based on cpu count.)
> > 
> > Trying to reverse the process of going to thread mode is more trouble than
> > it's work. I'd even argue there's no need to reset __isthreaded, but I don't
> > want to get bogged down with other consequences.
> 
> Indeed, thats the solution I came up with as well. chrome, ff, gimp
> and mysqld seem to be happy. Online now.
> 
>   -Otto

Hi,

diff has been commited just now (from the hackathon in Cambridge,
UK). Upcoming snaps will contain it.  Please make sure you update
packages as well when they arrive at mirrors, since both librthread and
libc versions have been bumped. Otherwise you just will be testing old
code.

Thanks to all the testers,

-Otto

Re: multi-pool malloc wip diff

[Otto Moerbeek]

On Mon, Aug 22, 2016 at 08:59:44PM -0400, Ted Unangst wrote:

> Otto Moerbeek wrote:
> > 
> > After a forkl of a threaded program __isthreaded is reset, but
> > existing allocations are spread around the pools. But the new single
> > threaded child only looks in the first pool. I have to think how to
> > solve this.
> 
> Create a new flag, _malloc_threads. Init to 0.
> 
> Change all existing tests for __isthreaded in malloc.c to _malloc_threads.
> 
> Set _malloc_threads to 1 (or N) in malloc_init().
> 
> (Future direction may be to replace _MALLOC_MUTEXES with _malloc_threads and
> allow it to vary somewhat based on cpu count.)
> 
> Trying to reverse the process of going to thread mode is more trouble than
> it's work. I'd even argue there's no need to reset __isthreaded, but I don't
> want to get bogged down with other consequences.

Indeed, thats the solution I came up with as well. chrome, ff, gimp
and mysqld seem to be happy. Online now.

-Otto

Re: multi-pool malloc wip diff

[Ted Unangst]

Otto Moerbeek wrote:
> 
> After a forkl of a threaded program __isthreaded is reset, but
> existing allocations are spread around the pools. But the new single
> threaded child only looks in the first pool. I have to think how to
> solve this.

Create a new flag, _malloc_threads. Init to 0.

Change all existing tests for __isthreaded in malloc.c to _malloc_threads.

Set _malloc_threads to 1 (or N) in malloc_init().

(Future direction may be to replace _MALLOC_MUTEXES with _malloc_threads and
allow it to vary somewhat based on cpu count.)

Trying to reverse the process of going to thread mode is more trouble than
it's work. I'd even argue there's no need to reset __isthreaded, but I don't
want to get bogged down with other consequences.

Re: multi-pool malloc wip diff

[Otto Moerbeek]

On Mon, Aug 22, 2016 at 08:37:30PM +0200, Otto Moerbeek wrote:

> On Mon, Aug 22, 2016 at 07:35:49PM +0200, Otto Moerbeek wrote:
> 
> > On Mon, Aug 22, 2016 at 07:07:21PM +0200, Gregor Best wrote:
> > 
> > > On Mon, Aug 22, 2016 at 05:03:34PM +0200, Otto Moerbeek wrote:
> > > > On Mon, Aug 22, 2016 at 05:00:12PM +0200, Otto Moerbeek wrote:
> > > > 
> > > > > Hmm, indeed, looking into it.
> > > > 
> > > > Fixed diff now online,
> > > > [...]
> > > 
> > > With that one, Firefox and Chromium work fine. There's a problem with
> > > Gimp though:
> > > 
> > > $ gimp
> > > gimp(51103) in free(): error: bogus pointer (double free?) 0x127da3e84400
> > > gimp(51103) in malloc(): error: recursive call
> > > GLib (gthread-posix.c): Unexpected error from C library during 'malloc':
> > > Resource deadlock avoided.  Aborting.
> > > 
> > > This doesn't happen when starting Gimp for the first time, so the
> > > process to reproduce it (on my machine at least) is:
> > > 
> > > - install gimp
> > > - move ~/.gimp-2.8 out of the way if it exists to make sure it's a clean
> > >   start
> > > - launch gimp
> > > - (gimp works fine now)
> > > - close gimp, start it again
> > > - the above error message appears during startup while starting the
> > >   extension "extension-script-fu" and gimp doesn't continue starting
> > > 
> > > How can I debug this further? At first glance, I don't really see a way
> > > for malloc to be called recursively :/
> > 
> > most likely an SIGABRT signal handler is involved.
> 
> indeed, attaching gdb shows that.
> 
> The problem is in a race wrt the setting of __isthreaded.
> 
> Changing the __isthreaded test in ofree() to TRUE fixed this for me.
> 
> More later, have to do some other things first.
> 
>   -Otto
> 

After a forkl of a threaded program __isthreaded is reset, but
existing allocations are spread around the pools. But the new single
threaded child only looks in the first pool. I have to think how to
solve this.

-Otto

Re: multi-pool malloc wip diff

[Otto Moerbeek]

On Mon, Aug 22, 2016 at 07:07:21PM +0200, Gregor Best wrote:

> On Mon, Aug 22, 2016 at 05:03:34PM +0200, Otto Moerbeek wrote:
> > On Mon, Aug 22, 2016 at 05:00:12PM +0200, Otto Moerbeek wrote:
> > 
> > > Hmm, indeed, looking into it.
> > 
> > Fixed diff now online,
> > [...]
> 
> With that one, Firefox and Chromium work fine. There's a problem with
> Gimp though:
> 
> $ gimp
> gimp(51103) in free(): error: bogus pointer (double free?) 0x127da3e84400
> gimp(51103) in malloc(): error: recursive call
> GLib (gthread-posix.c): Unexpected error from C library during 'malloc':
> Resource deadlock avoided.  Aborting.
> 
> This doesn't happen when starting Gimp for the first time, so the
> process to reproduce it (on my machine at least) is:
> 
> - install gimp
> - move ~/.gimp-2.8 out of the way if it exists to make sure it's a clean
>   start
> - launch gimp
> - (gimp works fine now)
> - close gimp, start it again
> - the above error message appears during startup while starting the
>   extension "extension-script-fu" and gimp doesn't continue starting
> 
> How can I debug this further? At first glance, I don't really see a way
> for malloc to be called recursively :/

most likely an SIGABRT signal handler is involved.

-Otto

Re: multi-pool malloc wip diff

[Gregor Best]

On Mon, Aug 22, 2016 at 05:03:34PM +0200, Otto Moerbeek wrote:
> On Mon, Aug 22, 2016 at 05:00:12PM +0200, Otto Moerbeek wrote:
> 
> > Hmm, indeed, looking into it.
> 
> Fixed diff now online,
> [...]

With that one, Firefox and Chromium work fine. There's a problem with
Gimp though:

$ gimp
gimp(51103) in free(): error: bogus pointer (double free?) 0x127da3e84400
gimp(51103) in malloc(): error: recursive call
GLib (gthread-posix.c): Unexpected error from C library during 'malloc':
Resource deadlock avoided.  Aborting.

This doesn't happen when starting Gimp for the first time, so the
process to reproduce it (on my machine at least) is:

- install gimp
- move ~/.gimp-2.8 out of the way if it exists to make sure it's a clean
  start
- launch gimp
- (gimp works fine now)
- close gimp, start it again
- the above error message appears during startup while starting the
  extension "extension-script-fu" and gimp doesn't continue starting

How can I debug this further? At first glance, I don't really see a way
for malloc to be called recursively :/

-- 
Gregor

Re: multi-pool malloc wip diff

[Otto Moerbeek]

On Mon, Aug 22, 2016 at 05:00:12PM +0200, Otto Moerbeek wrote:

> Hmm, indeed, looking into it.

Fixed diff now online,

-Otto

Re: multi-pool malloc wip diff

[Otto Moerbeek]

On Mon, Aug 22, 2016 at 04:15:16PM +0200, Gregor Best wrote:

> Hi,
> 
> On Mon, Aug 22, 2016 at 08:21:08AM +0200, Otto Moerbeek wrote:
> > [...]
> > New diff posted in http://www.drijf.net/openbsd/malloc.
> > 
> > The intention is that this is close to commitable. But while a
> > straight merge of most recent diff with the current src, a bug
> > might have crept in. So beware etc.
> > [...]
> 
> After applying this to a clean /usr/src/lib/libc, I get
> 
> cc -O2 -pipe -g -Wall -g -Werror -Wshadow 
> -Werror-implicit-function-declaration -Wsign-compare 
> -I/usr/src/lib/librthread -include namespace.h  
> -I/usr/src/lib/librthread/../libc/arch/amd64 
> -I/usr/src/lib/librthread/../libc/include   -c rthread.c -o rthread.o
> cc1: warnings being treated as errors
> rthread.c: In function '_rthread_init':
> rthread.c:212: warning: assignment from incompatible pointer type
> rthread.c:213: warning: assignment from incompatible pointer type
> 
> when building librthread. libc builds fine but stuff like firefox and
> chrome die with
> 
>   chrome(97224) in unknown error: free() called before allocation
>   Abort trap (core dumped)
> 
> and stack traces that look like this:
> 
> (gdb) bt
> #0  0x0c27475c9aaa in thrkill () at :2
> #1  0x0c27475e6589 in *_libc_abort () at 
> /usr/src/lib/libc/stdlib/abort.c:52
> #2  0x0c27475b8239 in wrterror (d=0x0, msg=0xc274773b2d8 "free() called 
> before allocation", p=0x0)
> at /usr/src/lib/libc/stdlib/malloc.c:307
> #3  0x0c27475b9746 in free (ptr=Variable "ptr" is not available.) at 
> /usr/src/lib/libc/stdlib/malloc.c:1390
> #4  0x0c2760446c70 in pthread_attr_destroy (attrp=0x7f7c3850) at 
> /usr/src/lib/librthread/rthread_attr.c:64
> [... other stuff omitted from here on ...]
> 
> Is the diff (I chose malloc-20160822.dif) missing a librthread change?

Hmm, indeed, looking into it.

-Otto

> 
> -- 
>   Gregor
> --
> 
> For those who like this sort of thing, this is the sort of thing they like.
>   -- Abraham Lincoln

Re: multi-pool malloc wip diff

[Gregor Best]

Hi,

On Mon, Aug 22, 2016 at 08:21:08AM +0200, Otto Moerbeek wrote:
> [...]
> New diff posted in http://www.drijf.net/openbsd/malloc.
> 
> The intention is that this is close to commitable. But while a
> straight merge of most recent diff with the current src, a bug
> might have crept in. So beware etc.
> [...]

After applying this to a clean /usr/src/lib/libc, I get

cc -O2 -pipe -g -Wall -g -Werror -Wshadow -Werror-implicit-function-declaration 
-Wsign-compare -I/usr/src/lib/librthread -include namespace.h  
-I/usr/src/lib/librthread/../libc/arch/amd64 
-I/usr/src/lib/librthread/../libc/include   -c rthread.c -o rthread.o
cc1: warnings being treated as errors
rthread.c: In function '_rthread_init':
rthread.c:212: warning: assignment from incompatible pointer type
rthread.c:213: warning: assignment from incompatible pointer type

when building librthread. libc builds fine but stuff like firefox and
chrome die with

chrome(97224) in unknown error: free() called before allocation
Abort trap (core dumped)

and stack traces that look like this:

(gdb) bt
#0  0x0c27475c9aaa in thrkill () at :2
#1  0x0c27475e6589 in *_libc_abort () at /usr/src/lib/libc/stdlib/abort.c:52
#2  0x0c27475b8239 in wrterror (d=0x0, msg=0xc274773b2d8 "free() called 
before allocation", p=0x0)
at /usr/src/lib/libc/stdlib/malloc.c:307
#3  0x0c27475b9746 in free (ptr=Variable "ptr" is not available.) at 
/usr/src/lib/libc/stdlib/malloc.c:1390
#4  0x0c2760446c70 in pthread_attr_destroy (attrp=0x7f7c3850) at 
/usr/src/lib/librthread/rthread_attr.c:64
[... other stuff omitted from here on ...]

Is the diff (I chose malloc-20160822.dif) missing a librthread change?

-- 
Gregor
--

For those who like this sort of thing, this is the sort of thing they like.
-- Abraham Lincoln

Re: multi-pool malloc wip diff

[Otto Moerbeek]

On Wed, May 11, 2016 at 10:04:48AM +0200, Otto Moerbeek wrote:

> On Fri, Apr 29, 2016 at 08:42:00AM +0200, Otto Moerbeek wrote:
> 
> > Hi,
> > 
> > new diff in http://www.drijf.net/openbsd/malloc/
> > 
> > Should fix the issue Ted spotted and contains initial code to only set
> > up multiple pools if threaded. This one is only lightly tested by me,
> > but I wanted to post this before I'll be away for a semi-long weekend,
> > 
> > I don't think this is ready for wide testing, but of course I
> > encourage playing, testing, reviewing or printing out and using as
> > wallpaper ;-)
> > 
> > -Otto
> 
> And I just publsihed a new diff at http://www.drijf.net/openbsd/malloc
> 
> Changes:
> 
> - diff against current. Due to the TIB work quite some things changed.
> MOst importnat change is that I now have direct access to the thread
> id. Since this is a random value, I can use the lower bits directly
> to compute the "home" pool for a thread.
> 
> - Realized wrterror() is since some years unconditionally a dead(-end)
> function. I'm using this in the multi-thread related parts, but this
> could probably be used in other places as well.
> 
> Please be very sure you are running current packages, otherwise you
> are not testing what you think you are testing.
> 
>   -Otto

New diff posted in http://www.drijf.net/openbsd/malloc.

The intention is that this is close to commitable. But while a
straight merge of most recent diff with the current src, a bug
might have crept in. So beware etc.

-Otto

Re: multi-pool malloc wip diff

[Otto Moerbeek]

Hi,

new diff in http://www.drijf.net/openbsd/malloc/

Should fix the issue Ted spotted and contains initial code to only set
up multiple pools if threaded. This one is only lightly tested by me,
but I wanted to post this before I'll be away for a semi-long weekend,

I don't think this is ready for wide testing, but of course I
encourage playing, testing, reviewing or printing out and using as
wallpaper ;-)

-Otto

Re: multi-pool malloc wip diff

[Otto Moerbeek]

On Thu, Apr 28, 2016 at 01:07:30PM -0400, Ted Unangst wrote:

> Otto Moerbeek wrote:
> >  static void
> > -ofree(struct dir_info *pool, void *p)
> > +ofree(struct dir_info *argpool, void *p)
> >  {
> > +   struct dir_info *pool;
> > struct region_info *r;
> > size_t sz;
> > +   int i;
> >  
> > +   pool = argpool;
> > r = find(pool, p);
> > if (r == NULL) {
> > -   wrterror(pool, "bogus pointer (double free?)", p);
> > -   return;
> > +   for (i = 0; i < _MALLOC_MUTEXES; i++) {
> > +   if (i == pool->mutex)
> > +   continue;
> > +   pool->active--;
> > +   _MALLOC_UNLOCK(pool->mutex);
> > +   pool = mopts.malloc_pool[i];
> > +   _MALLOC_LOCK(pool->mutex);
> > +   pool->active++;
> > +   r = find(pool, p);
> > +   if (r != NULL)
> > +   break;
> > +   }   
> > +   if (r == NULL) {
> > +   wrterror(pool, "bogus pointer (double free?)", p);
> > +   goto done;
> > +   }
> 
> I'm having trouble understanding this loop. I think you are trying to avoid
> locking the initial pool again. but this only works if argpool is 0. if it's
> something else, then pool will change, and pool->mutex will never equal i.

Indeed. That isn't right. Have to compare to argpool->mutex probably.

Thanks for spoting that, same error applies to realloc,

-Otto

Re: multi-pool malloc wip diff

[Ted Unangst]

Otto Moerbeek wrote:
>  static void
> -ofree(struct dir_info *pool, void *p)
> +ofree(struct dir_info *argpool, void *p)
>  {
> + struct dir_info *pool;
>   struct region_info *r;
>   size_t sz;
> + int i;
>  
> + pool = argpool;
>   r = find(pool, p);
>   if (r == NULL) {
> - wrterror(pool, "bogus pointer (double free?)", p);
> - return;
> + for (i = 0; i < _MALLOC_MUTEXES; i++) {
> + if (i == pool->mutex)
> + continue;
> + pool->active--;
> + _MALLOC_UNLOCK(pool->mutex);
> + pool = mopts.malloc_pool[i];
> + _MALLOC_LOCK(pool->mutex);
> + pool->active++;
> + r = find(pool, p);
> + if (r != NULL)
> + break;
> + }   
> + if (r == NULL) {
> + wrterror(pool, "bogus pointer (double free?)", p);
> + goto done;
> + }

I'm having trouble understanding this loop. I think you are trying to avoid
locking the initial pool again. but this only works if argpool is 0. if it's
something else, then pool will change, and pool->mutex will never equal i.

Re: multi-pool malloc wip diff

[Otto Moerbeek]

On Wed, Mar 30, 2016 at 06:30:53PM +0200, Norman Golisz wrote:

> On Mon Mar 28 2016 11:27, Otto Moerbeek wrote:
> > Second diff. Only one person (Stefan Kempf, thanks!) gave feedback...
> 
> Sorry, running with this patch since a week, but missed to give
> feedback.
> 
> As others already reported, no regressions here on amd64 also.

Thanks to all the testers.

I have some ideas on how to improve things, but for at least one piece
I'm dependent on some other rthreads work that hasn' materialized yet.

On the other hand I have cleanup diffs I'd like to commit that confict
with the latest muliple-pool diff. 

So I will commiting those soon, and after that I will be working on a
new multi-pool diff.

-Otto

Re: multi-pool malloc wip diff

[Norman Golisz]

On Mon Mar 28 2016 11:27, Otto Moerbeek wrote:
> Second diff. Only one person (Stefan Kempf, thanks!) gave feedback...

Sorry, running with this patch since a week, but missed to give
feedback.

As others already reported, no regressions here on amd64 also.

Re: multi-pool malloc wip diff

[Juan Francisco Cantero Hurtado]

On Mon, Mar 28, 2016 at 11:27:32AM +0200, Otto Moerbeek wrote:
> On Wed, Mar 23, 2016 at 08:00:19AM +0100, Otto Moerbeek wrote:
> 
> > Hi,
> > 
> > first diff that seems to work. Tested on amd64 and compile tested on
> > sparc64. 
> > 
> > It is alo available at http://www.drijf.net/openbsd/malloc
> > 
> > Form the README:
> > 
> > The diff should be applied while in /usr/src/lib, it will patch
> > both librthreads as as well as libc.
> > 
> > THIS IS WORK IN PROGRESS. It contains multiple things that should
> > be improved. To name a few things:
> > 
> > - Curently fixed at 4 pools with a fixed thread -> pool mapping.
> > - All pools are always initialized, even for single threaded programs, where
> >   only one pool is used.
> > - Especially realloc gets quite a bit uglier.
> > - I'm pondering storing the thread -> pool mapping in the thread
> >   struct instead of computing it each time from the tcb address.
> > 
> > -Otto
> > 
> 
> Second diff. Only one person (Stefan Kempf, thanks!) gave feedback...
> 
> A race condition was fixed in the init code. But there remain race
> problems in the init code. I will be working on that the coming time.
> 
> Please be aware that to make this code ready for commit, I need
> feedback/tests/reviews. There's no way this code will end up in the tree 
> without those.

I don't see regressions on amd64.

-- 
Juan Francisco Cantero Hurtado http://juanfra.info

Re: multi-pool malloc wip diff

[Stuart Henderson]

On 2016/03/28 11:27, Otto Moerbeek wrote:
> Second diff. Only one person (Stefan Kempf, thanks!) gave feedback...

I've done i386 port bulk builds with both iterations of this, no
problems seen yet.

Re: multi-pool malloc wip diff

[Mike Larkin]

On Mon, Mar 28, 2016 at 11:27:32AM +0200, Otto Moerbeek wrote:
> On Wed, Mar 23, 2016 at 08:00:19AM +0100, Otto Moerbeek wrote:
> 
> > Hi,
> > 
> > first diff that seems to work. Tested on amd64 and compile tested on
> > sparc64. 
> > 
> > It is alo available at http://www.drijf.net/openbsd/malloc
> > 
> > Form the README:
> > 
> > The diff should be applied while in /usr/src/lib, it will patch
> > both librthreads as as well as libc.
> > 
> > THIS IS WORK IN PROGRESS. It contains multiple things that should
> > be improved. To name a few things:
> > 
> > - Curently fixed at 4 pools with a fixed thread -> pool mapping.
> > - All pools are always initialized, even for single threaded programs, where
> >   only one pool is used.
> > - Especially realloc gets quite a bit uglier.
> > - I'm pondering storing the thread -> pool mapping in the thread
> >   struct instead of computing it each time from the tcb address.
> > 
> > -Otto
> > 
> 
> Second diff. Only one person (Stefan Kempf, thanks!) gave feedback...
> 
> A race condition was fixed in the init code. But there remain race
> problems in the init code. I will be working on that the coming time.
> 
> Please be aware that to make this code ready for commit, I need
> feedback/tests/reviews. There's no way this code will end up in the tree 
> without those.
> 
>   -Otto
> 

Been running this in a VM since this weekend, no issues seen. (amd64).

-ml

> 
> Index: libc/include/thread_private.h
> ===
> RCS file: /cvs/src/lib/libc/include/thread_private.h,v
> retrieving revision 1.26
> diff -u -p -r1.26 thread_private.h
> --- libc/include/thread_private.h 7 Apr 2015 01:27:07 -   1.26
> +++ libc/include/thread_private.h 28 Mar 2016 08:22:31 -
> @@ -17,6 +17,8 @@
>   */
>  extern int __isthreaded;
>  
> +#define _MALLOC_MUTEXES 4
> +
>  /*
>   * Weak symbols are used in libc so that the thread library can
>   * efficiently wrap libc functions.
> @@ -136,16 +138,16 @@ extern void *__THREAD_NAME(serv_mutex);
>  /*
>   * malloc lock/unlock prototypes and definitions
>   */
> -void _thread_malloc_lock(void);
> -void _thread_malloc_unlock(void);
> +void _thread_malloc_lock(int);
> +void _thread_malloc_unlock(int);
>  
> -#define _MALLOC_LOCK()   do {
> \
> +#define _MALLOC_LOCK(n)  do {
> \
>   if (__isthreaded)   \
> - _thread_malloc_lock();  \
> + _thread_malloc_lock(n); \
>   } while (0)
> -#define _MALLOC_UNLOCK() do {\
> +#define _MALLOC_UNLOCK(n)do {\
>   if (__isthreaded)   \
> - _thread_malloc_unlock();\
> + _thread_malloc_unlock(n);\
>   } while (0)
>  
>  void _thread_atexit_lock(void);
> Index: libc/stdlib/malloc.c
> ===
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.185
> diff -u -p -r1.185 malloc.c
> --- libc/stdlib/malloc.c  17 Mar 2016 17:55:33 -  1.185
> +++ libc/stdlib/malloc.c  28 Mar 2016 08:22:31 -
> @@ -1,6 +1,6 @@
>  /*   $OpenBSD: malloc.c,v 1.185 2016/03/17 17:55:33 mmcc Exp $   */
>  /*
> - * Copyright (c) 2008, 2010, 2011 Otto Moerbeek 
> + * Copyright (c) 2008, 2010, 2011, 2016 Otto Moerbeek 
>   * Copyright (c) 2012 Matthew Dempsky 
>   * Copyright (c) 2008 Damien Miller 
>   * Copyright (c) 2000 Poul-Henning Kamp 
> @@ -43,6 +43,7 @@
>  #endif
>  
>  #include "thread_private.h"
> +#include 
>  
>  #if defined(__sparc__) && !defined(__sparcv9__)
>  #define MALLOC_PAGESHIFT (13U)
> @@ -95,10 +96,10 @@
>  
>  #define _MALLOC_LEAVE(d) do { if (__isthreaded) { \
>   (d)->active--; \
> - _MALLOC_UNLOCK(); } \
> + _MALLOC_UNLOCK(d->mutex); } \
>  } while (0)
>  #define _MALLOC_ENTER(d) do { if (__isthreaded) { \
> - _MALLOC_LOCK(); \
> + _MALLOC_LOCK(d->mutex); \
>   (d)->active++; } \
>  } while (0)
>  
> @@ -129,6 +130,7 @@ struct dir_info {
>   void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
>   size_t rbytesused;  /* random bytes used */
>   char *func; /* current function */
> + int mutex;
>   u_char rbytes[32];  /* random bytes */
>   u_short chunk_start;
>  #ifdef MALLOC_STATS
> @@ -178,7 +180,7 @@ struct chunk_info {
>  };
>  
>  struct malloc_readonly {
> - struct dir_info *malloc_pool;   /* Main bookkeeping information */
> + struct dir_info 

Re: multi-pool malloc wip diff

[Daniel Micay]

> - Curently fixed at 4 pools with a fixed thread -> pool mapping.
> - All pools are always initialized, even for single threaded programs,
> where
>   only one pool is used.
> - Especially realloc gets quite a bit uglier.
> - I'm pondering storing the thread -> pool mapping in the thread
>   struct instead of computing it each time from the tcb address.

If you wanted to get rid of the static mapping of threads to pools, it
would make sense to replace the per-pool region tables with a global, 2-
tier table. The first tier would be a table of mutexes paired with the
current style region tables, and it would pick the right one using a
secondary hash. It could be much larger than the number of pools to
avoid it being a bottleneck. This would eliminate the free / realloc
loops.

I don't think it would make sense right now, since it would add an extra
pair of lock / unlock calls to each malloc / free call in the common
case where allocations are freed by the allocating thread. If the thread
-> pool assignment was dynamic, it would be faster due to not needing
those loops. So if you wanted to do something like picking a random pool
each time, this would make a lot of sense. It would be increasingly
better with a larger number of pools.

Re: multi-pool malloc wip diff

[Henrik Friedrichsen]

Just a subjective opinion (if that counts as feedback):

This in combination with the scheduler patch by mpi@ seems to greatly
improve the browsing experience with Chrome, esp. when opening/closing
tabs, which I suppose involves a lot of memory management calls?

Other than that I haven't noticed any regressions, yet.

Re: multi-pool malloc wip diff

[Otto Moerbeek]

On Mon, Mar 28, 2016 at 11:43:22AM +0200, Otto Moerbeek wrote:

> No specifics, just watch out for malloc related bugs. It's
> both important that non-threaded programs and non-threaded programs
> show no new bugs.

And don't forget to test non-threaded programs ;-)

Re: multi-pool malloc wip diff

[Otto Moerbeek]

On Mon, Mar 28, 2016 at 03:40:21AM -0600, Abel Abraham Camarillo Ojeda wrote:

> On Mon, Mar 28, 2016 at 3:27 AM, Otto Moerbeek  wrote:
> > On Wed, Mar 23, 2016 at 08:00:19AM +0100, Otto Moerbeek wrote:
> >
> >> Hi,
> >>
> >> first diff that seems to work. Tested on amd64 and compile tested on
> >> sparc64.
> >>
> >> It is alo available at http://www.drijf.net/openbsd/malloc
> >>
> >> Form the README:
> >>
> >> The diff should be applied while in /usr/src/lib, it will patch
> >> both librthreads as as well as libc.
> >>
> >> THIS IS WORK IN PROGRESS. It contains multiple things that should
> >> be improved. To name a few things:
> >>
> >> - Curently fixed at 4 pools with a fixed thread -> pool mapping.
> >> - All pools are always initialized, even for single threaded programs, 
> >> where
> >>   only one pool is used.
> >> - Especially realloc gets quite a bit uglier.
> >> - I'm pondering storing the thread -> pool mapping in the thread
> >>   struct instead of computing it each time from the tcb address.
> >>
> >>   -Otto
> >>
> >
> > Second diff. Only one person (Stefan Kempf, thanks!) gave feedback...
> >
> > A race condition was fixed in the init code. But there remain race
> > problems in the init code. I will be working on that the coming time.
> >
> > Please be aware that to make this code ready for commit, I need
> > feedback/tests/reviews. There's no way this code will end up in the tree
> > without those.
> >
> 
> Hi Otto,
> 
> anything specific to test or general desktop-browser-use testing is enough?
> 
> thanks

No specifics, just watch out for malloc related bugs. It's
both important that non-threaded programs and non-threaded programs
show no new bugs.


-Otto

> 
> 
> > -Otto
> >
> >
> > Index: libc/include/thread_private.h
> > ===
> > RCS file: /cvs/src/lib/libc/include/thread_private.h,v
> > retrieving revision 1.26
> > diff -u -p -r1.26 thread_private.h
> > --- libc/include/thread_private.h   7 Apr 2015 01:27:07 -   1.26
> > +++ libc/include/thread_private.h   28 Mar 2016 08:22:31 -
> > @@ -17,6 +17,8 @@
> >   */
> >  extern int __isthreaded;
> >
> > +#define _MALLOC_MUTEXES 4
> > +
> >  /*
> >   * Weak symbols are used in libc so that the thread library can
> >   * efficiently wrap libc functions.
> > @@ -136,16 +138,16 @@ extern void *__THREAD_NAME(serv_mutex);
> >  /*
> >   * malloc lock/unlock prototypes and definitions
> >   */
> > -void   _thread_malloc_lock(void);
> > -void   _thread_malloc_unlock(void);
> > +void   _thread_malloc_lock(int);
> > +void   _thread_malloc_unlock(int);
> >
> > -#define _MALLOC_LOCK() do {\
> > +#define _MALLOC_LOCK(n)do {
> > \
> > if (__isthreaded)   \
> > -   _thread_malloc_lock();  \
> > +   _thread_malloc_lock(n); \
> > } while (0)
> > -#define _MALLOC_UNLOCK()   do {\
> > +#define _MALLOC_UNLOCK(n)  do {\
> > if (__isthreaded)   \
> > -   _thread_malloc_unlock();\
> > +   _thread_malloc_unlock(n);\
> > } while (0)
> >
> >  void   _thread_atexit_lock(void);
> > Index: libc/stdlib/malloc.c
> > ===
> > RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> > retrieving revision 1.185
> > diff -u -p -r1.185 malloc.c
> > --- libc/stdlib/malloc.c17 Mar 2016 17:55:33 -  1.185
> > +++ libc/stdlib/malloc.c28 Mar 2016 08:22:31 -
> > @@ -1,6 +1,6 @@
> >  /* $OpenBSD: malloc.c,v 1.185 2016/03/17 17:55:33 mmcc Exp $   */
> >  /*
> > - * Copyright (c) 2008, 2010, 2011 Otto Moerbeek 
> > + * Copyright (c) 2008, 2010, 2011, 2016 Otto Moerbeek 
> >   * Copyright (c) 2012 Matthew Dempsky 
> >   * Copyright (c) 2008 Damien Miller 
> >   * Copyright (c) 2000 Poul-Henning Kamp 
> > @@ -43,6 +43,7 @@
> >  #endif
> >
> >  #include "thread_private.h"
> > +#include 
> >
> >  #if defined(__sparc__) && !defined(__sparcv9__)
> >  #define MALLOC_PAGESHIFT   (13U)
> > @@ -95,10 +96,10 @@
> >
> >  #define _MALLOC_LEAVE(d) do { if (__isthreaded) { \
> > (d)->active--; \
> > -   _MALLOC_UNLOCK(); } \
> > +   _MALLOC_UNLOCK(d->mutex); } \
> >  } while (0)
> >  #define _MALLOC_ENTER(d) do { if (__isthreaded) { \
> > -   _MALLOC_LOCK(); \
> > +   _MALLOC_LOCK(d->mutex); \
> > (d)->active++; } \
> >  } while (0)
> >
> > @@ -129,6 +130,7 @@ 

Re: multi-pool malloc wip diff

[Abel Abraham Camarillo Ojeda]

On Mon, Mar 28, 2016 at 3:27 AM, Otto Moerbeek  wrote:
> On Wed, Mar 23, 2016 at 08:00:19AM +0100, Otto Moerbeek wrote:
>
>> Hi,
>>
>> first diff that seems to work. Tested on amd64 and compile tested on
>> sparc64.
>>
>> It is alo available at http://www.drijf.net/openbsd/malloc
>>
>> Form the README:
>>
>> The diff should be applied while in /usr/src/lib, it will patch
>> both librthreads as as well as libc.
>>
>> THIS IS WORK IN PROGRESS. It contains multiple things that should
>> be improved. To name a few things:
>>
>> - Curently fixed at 4 pools with a fixed thread -> pool mapping.
>> - All pools are always initialized, even for single threaded programs, where
>>   only one pool is used.
>> - Especially realloc gets quite a bit uglier.
>> - I'm pondering storing the thread -> pool mapping in the thread
>>   struct instead of computing it each time from the tcb address.
>>
>>   -Otto
>>
>
> Second diff. Only one person (Stefan Kempf, thanks!) gave feedback...
>
> A race condition was fixed in the init code. But there remain race
> problems in the init code. I will be working on that the coming time.
>
> Please be aware that to make this code ready for commit, I need
> feedback/tests/reviews. There's no way this code will end up in the tree
> without those.
>

Hi Otto,

anything specific to test or general desktop-browser-use testing is enough?

thanks


> -Otto
>
>
> Index: libc/include/thread_private.h
> ===
> RCS file: /cvs/src/lib/libc/include/thread_private.h,v
> retrieving revision 1.26
> diff -u -p -r1.26 thread_private.h
> --- libc/include/thread_private.h   7 Apr 2015 01:27:07 -   1.26
> +++ libc/include/thread_private.h   28 Mar 2016 08:22:31 -
> @@ -17,6 +17,8 @@
>   */
>  extern int __isthreaded;
>
> +#define _MALLOC_MUTEXES 4
> +
>  /*
>   * Weak symbols are used in libc so that the thread library can
>   * efficiently wrap libc functions.
> @@ -136,16 +138,16 @@ extern void *__THREAD_NAME(serv_mutex);
>  /*
>   * malloc lock/unlock prototypes and definitions
>   */
> -void   _thread_malloc_lock(void);
> -void   _thread_malloc_unlock(void);
> +void   _thread_malloc_lock(int);
> +void   _thread_malloc_unlock(int);
>
> -#define _MALLOC_LOCK() do {\
> +#define _MALLOC_LOCK(n)do {  
>   \
> if (__isthreaded)   \
> -   _thread_malloc_lock();  \
> +   _thread_malloc_lock(n); \
> } while (0)
> -#define _MALLOC_UNLOCK()   do {\
> +#define _MALLOC_UNLOCK(n)  do {\
> if (__isthreaded)   \
> -   _thread_malloc_unlock();\
> +   _thread_malloc_unlock(n);\
> } while (0)
>
>  void   _thread_atexit_lock(void);
> Index: libc/stdlib/malloc.c
> ===
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.185
> diff -u -p -r1.185 malloc.c
> --- libc/stdlib/malloc.c17 Mar 2016 17:55:33 -  1.185
> +++ libc/stdlib/malloc.c28 Mar 2016 08:22:31 -
> @@ -1,6 +1,6 @@
>  /* $OpenBSD: malloc.c,v 1.185 2016/03/17 17:55:33 mmcc Exp $   */
>  /*
> - * Copyright (c) 2008, 2010, 2011 Otto Moerbeek 
> + * Copyright (c) 2008, 2010, 2011, 2016 Otto Moerbeek 
>   * Copyright (c) 2012 Matthew Dempsky 
>   * Copyright (c) 2008 Damien Miller 
>   * Copyright (c) 2000 Poul-Henning Kamp 
> @@ -43,6 +43,7 @@
>  #endif
>
>  #include "thread_private.h"
> +#include 
>
>  #if defined(__sparc__) && !defined(__sparcv9__)
>  #define MALLOC_PAGESHIFT   (13U)
> @@ -95,10 +96,10 @@
>
>  #define _MALLOC_LEAVE(d) do { if (__isthreaded) { \
> (d)->active--; \
> -   _MALLOC_UNLOCK(); } \
> +   _MALLOC_UNLOCK(d->mutex); } \
>  } while (0)
>  #define _MALLOC_ENTER(d) do { if (__isthreaded) { \
> -   _MALLOC_LOCK(); \
> +   _MALLOC_LOCK(d->mutex); \
> (d)->active++; } \
>  } while (0)
>
> @@ -129,6 +130,7 @@ struct dir_info {
> void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
> size_t rbytesused;  /* random bytes used */
> char *func; /* current function */
> +   int mutex;
> u_char rbytes[32];  /* random bytes */
> u_short chunk_start;
>  #ifdef MALLOC_STATS
> @@ -178,7 +180,7 @@ struct chunk_info {
>  };
>
>  struct malloc_readonly {
> -   struct dir_info *malloc_pool;   /* Main bookkeeping information 

Re: multi-pool malloc wip diff

[Otto Moerbeek]

On Wed, Mar 23, 2016 at 08:00:19AM +0100, Otto Moerbeek wrote:

> Hi,
> 
> first diff that seems to work. Tested on amd64 and compile tested on
> sparc64. 
> 
> It is alo available at http://www.drijf.net/openbsd/malloc
> 
> Form the README:
> 
> The diff should be applied while in /usr/src/lib, it will patch
> both librthreads as as well as libc.
> 
> THIS IS WORK IN PROGRESS. It contains multiple things that should
> be improved. To name a few things:
> 
> - Curently fixed at 4 pools with a fixed thread -> pool mapping.
> - All pools are always initialized, even for single threaded programs, where
>   only one pool is used.
> - Especially realloc gets quite a bit uglier.
> - I'm pondering storing the thread -> pool mapping in the thread
>   struct instead of computing it each time from the tcb address.
> 
>   -Otto
> 

Second diff. Only one person (Stefan Kempf, thanks!) gave feedback...

A race condition was fixed in the init code. But there remain race
problems in the init code. I will be working on that the coming time.

Please be aware that to make this code ready for commit, I need
feedback/tests/reviews. There's no way this code will end up in the tree 
without those.

-Otto


Index: libc/include/thread_private.h
===
RCS file: /cvs/src/lib/libc/include/thread_private.h,v
retrieving revision 1.26
diff -u -p -r1.26 thread_private.h
--- libc/include/thread_private.h   7 Apr 2015 01:27:07 -   1.26
+++ libc/include/thread_private.h   28 Mar 2016 08:22:31 -
@@ -17,6 +17,8 @@
  */
 extern int __isthreaded;
 
+#define _MALLOC_MUTEXES 4
+
 /*
  * Weak symbols are used in libc so that the thread library can
  * efficiently wrap libc functions.
@@ -136,16 +138,16 @@ extern void *__THREAD_NAME(serv_mutex);
 /*
  * malloc lock/unlock prototypes and definitions
  */
-void   _thread_malloc_lock(void);
-void   _thread_malloc_unlock(void);
+void   _thread_malloc_lock(int);
+void   _thread_malloc_unlock(int);
 
-#define _MALLOC_LOCK() do {\
+#define _MALLOC_LOCK(n)do {
\
if (__isthreaded)   \
-   _thread_malloc_lock();  \
+   _thread_malloc_lock(n); \
} while (0)
-#define _MALLOC_UNLOCK()   do {\
+#define _MALLOC_UNLOCK(n)  do {\
if (__isthreaded)   \
-   _thread_malloc_unlock();\
+   _thread_malloc_unlock(n);\
} while (0)
 
 void   _thread_atexit_lock(void);
Index: libc/stdlib/malloc.c
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.185
diff -u -p -r1.185 malloc.c
--- libc/stdlib/malloc.c17 Mar 2016 17:55:33 -  1.185
+++ libc/stdlib/malloc.c28 Mar 2016 08:22:31 -
@@ -1,6 +1,6 @@
 /* $OpenBSD: malloc.c,v 1.185 2016/03/17 17:55:33 mmcc Exp $   */
 /*
- * Copyright (c) 2008, 2010, 2011 Otto Moerbeek 
+ * Copyright (c) 2008, 2010, 2011, 2016 Otto Moerbeek 
  * Copyright (c) 2012 Matthew Dempsky 
  * Copyright (c) 2008 Damien Miller 
  * Copyright (c) 2000 Poul-Henning Kamp 
@@ -43,6 +43,7 @@
 #endif
 
 #include "thread_private.h"
+#include 
 
 #if defined(__sparc__) && !defined(__sparcv9__)
 #define MALLOC_PAGESHIFT   (13U)
@@ -95,10 +96,10 @@
 
 #define _MALLOC_LEAVE(d) do { if (__isthreaded) { \
(d)->active--; \
-   _MALLOC_UNLOCK(); } \
+   _MALLOC_UNLOCK(d->mutex); } \
 } while (0)
 #define _MALLOC_ENTER(d) do { if (__isthreaded) { \
-   _MALLOC_LOCK(); \
+   _MALLOC_LOCK(d->mutex); \
(d)->active++; } \
 } while (0)
 
@@ -129,6 +130,7 @@ struct dir_info {
void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
size_t rbytesused;  /* random bytes used */
char *func; /* current function */
+   int mutex;
u_char rbytes[32];  /* random bytes */
u_short chunk_start;
 #ifdef MALLOC_STATS
@@ -178,7 +180,7 @@ struct chunk_info {
 };
 
 struct malloc_readonly {
-   struct dir_info *malloc_pool;   /* Main bookkeeping information */
+   struct dir_info *malloc_pool[_MALLOC_MUTEXES];  /* Main bookkeeping 
information */
int malloc_freenow; /* Free quickly - disable chunk rnd */
int malloc_freeunmap;   /* mprotect free pages PROT_NONE? */
int malloc_hint;/* call madvice on free pages?  */
@@ -202,14 +204,13 @@ static union {

multi-pool malloc wip diff

[Otto Moerbeek]

Hi,

first diff that seems to work. Tested on amd64 and compile tested on
sparc64. 

It is alo available at http://www.drijf.net/openbsd/malloc

Form the README:

The diff should be applied while in /usr/src/lib, it will patch
both librthreads as as well as libc.

THIS IS WORK IN PROGRESS. It contains multiple things that should
be improved. To name a few things:

- Curently fixed at 4 pools with a fixed thread -> pool mapping.
- All pools are always initialized, even for single threaded programs, where
  only one pool is used.
- Especially realloc gets quite a bit uglier.
- I'm pondering storing the thread -> pool mapping in the thread
  struct instead of computing it each time from the tcb address.

-Otto

Index: libc/include/thread_private.h
===
RCS file: /cvs/src/lib/libc/include/thread_private.h,v
retrieving revision 1.26
diff -u -p -r1.26 thread_private.h
--- libc/include/thread_private.h   7 Apr 2015 01:27:07 -   1.26
+++ libc/include/thread_private.h   21 Mar 2016 16:34:52 -
@@ -17,6 +17,8 @@
  */
 extern int __isthreaded;
 
+#define _MALLOC_MUTEXES 4
+
 /*
  * Weak symbols are used in libc so that the thread library can
  * efficiently wrap libc functions.
@@ -136,16 +138,16 @@ extern void *__THREAD_NAME(serv_mutex);
 /*
  * malloc lock/unlock prototypes and definitions
  */
-void   _thread_malloc_lock(void);
-void   _thread_malloc_unlock(void);
+void   _thread_malloc_lock(int);
+void   _thread_malloc_unlock(int);
 
-#define _MALLOC_LOCK() do {\
+#define _MALLOC_LOCK(n)do {
\
if (__isthreaded)   \
-   _thread_malloc_lock();  \
+   _thread_malloc_lock(n); \
} while (0)
-#define _MALLOC_UNLOCK()   do {\
+#define _MALLOC_UNLOCK(n)  do {\
if (__isthreaded)   \
-   _thread_malloc_unlock();\
+   _thread_malloc_unlock(n);\
} while (0)
 
 void   _thread_atexit_lock(void);
Index: libc/stdlib/malloc.c
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.185
diff -u -p -r1.185 malloc.c
--- libc/stdlib/malloc.c17 Mar 2016 17:55:33 -  1.185
+++ libc/stdlib/malloc.c21 Mar 2016 16:34:52 -
@@ -1,6 +1,6 @@
 /* $OpenBSD: malloc.c,v 1.185 2016/03/17 17:55:33 mmcc Exp $   */
 /*
- * Copyright (c) 2008, 2010, 2011 Otto Moerbeek 
+ * Copyright (c) 2008, 2010, 2011, 2016 Otto Moerbeek 
  * Copyright (c) 2012 Matthew Dempsky 
  * Copyright (c) 2008 Damien Miller 
  * Copyright (c) 2000 Poul-Henning Kamp 
@@ -43,6 +43,7 @@
 #endif
 
 #include "thread_private.h"
+#include 
 
 #if defined(__sparc__) && !defined(__sparcv9__)
 #define MALLOC_PAGESHIFT   (13U)
@@ -95,10 +96,10 @@
 
 #define _MALLOC_LEAVE(d) do { if (__isthreaded) { \
(d)->active--; \
-   _MALLOC_UNLOCK(); } \
+   _MALLOC_UNLOCK(d->mutex); } \
 } while (0)
 #define _MALLOC_ENTER(d) do { if (__isthreaded) { \
-   _MALLOC_LOCK(); \
+   _MALLOC_LOCK(d->mutex); \
(d)->active++; } \
 } while (0)
 
@@ -129,6 +130,7 @@ struct dir_info {
void *delayed_chunks[MALLOC_DELAYED_CHUNK_MASK + 1];
size_t rbytesused;  /* random bytes used */
char *func; /* current function */
+   int mutex;
u_char rbytes[32];  /* random bytes */
u_short chunk_start;
 #ifdef MALLOC_STATS
@@ -178,7 +180,7 @@ struct chunk_info {
 };
 
 struct malloc_readonly {
-   struct dir_info *malloc_pool;   /* Main bookkeeping information */
+   struct dir_info *malloc_pool[_MALLOC_MUTEXES];  /* Main bookkeeping 
information */
int malloc_freenow; /* Free quickly - disable chunk rnd */
int malloc_freeunmap;   /* mprotect free pages PROT_NONE? */
int malloc_hint;/* call madvice on free pages?  */
@@ -202,14 +204,13 @@ static union {
u_char _pad[MALLOC_PAGESIZE];
 } malloc_readonly __attribute__((aligned(MALLOC_PAGESIZE)));
 #define mopts  malloc_readonly.mopts
-#define getpool() mopts.malloc_pool
 
 char   *malloc_options;/* compile-time options */
 
 static u_char getrbyte(struct dir_info *d);
 
 #ifdef MALLOC_STATS
-void malloc_dump(int);
+void malloc_dump(int, struct dir_info *);
 PROTO_NORMAL(malloc_dump);
 static void malloc_exit(void);
 #define CALLER __builtin_return_address(0)
@@