git: 85d028223bc2 - main - libthr malloc: support recursion on thr_malloc_umtx.

[Konstantin Belousov]

On Tue, Jan 12, 2021 at 11:16:25AM +0000, Alexander Richardson wrote:
> On Tue, 12 Jan 2021 at 10:46, Konstantin Belousov <kib at freebsd.org> wrote:
> >
> > The branch main has been updated by kib:
> >
> > URL: https://cgit.FreeBSD.org/src/commit/?id=85d028223bc2768651f4d44881644ceb5dc2a664
> >
> > commit 85d028223bc2768651f4d44881644ceb5dc2a664
> > Author:     Konstantin Belousov <kib at FreeBSD.org>
> > AuthorDate: 2021-01-12 09:02:37 +0000
> > Commit:     Konstantin Belousov <kib at FreeBSD.org>
> > CommitDate: 2021-01-12 10:45:44 +0000
> >
> >     libthr malloc: support recursion on thr_malloc_umtx.
> >
> >     One possible way the recursion can happen is during fork: suppose
> >     that fork is called from early code that did not triggered
> >     jemalloc(3) initialization yet. Then we lock thr_malloc lock, and
> >     call malloc_prefork() that might require initialization of jemalloc
> >     pthread_mutexes, calling into libthr malloc. It is safe to allow
> >     recursion for this occurence.
> >
> >     PR:     252579
> >     Reported by:    Vasily Postnicov <shamaz.mazum at gmail.com>
> >     MFC after:      1 week
> >     Sponsored by:   The FreeBSD Foundation
> > ---
> >  lib/libthr/thread/thr_malloc.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/libthr/thread/thr_malloc.c b/lib/libthr/thread/thr_malloc.c
> > index 2f1c81142942..80a81f9c6c27 100644
> > --- a/lib/libthr/thread/thr_malloc.c
> > +++ b/lib/libthr/thread/thr_malloc.c
> > @@ -41,6 +41,7 @@ int npagesizes;
> >  size_t *pagesizes;
> >  static size_t pagesizes_d[2];
> >  static struct umutex thr_malloc_umtx;
> > +static u_int thr_malloc_umtx_level;
> >
> >  void
> >  __thr_malloc_init(void)
> > @@ -60,11 +61,16 @@ __thr_malloc_init(void)
> >  static void
> >  thr_malloc_lock(struct pthread *curthread)
> >  {
> > +       uint32_t curtid;
> >
> >         if (curthread == NULL)
> >                 return;
> >         curthread->locklevel++;
> > -       _thr_umutex_lock(&thr_malloc_umtx, TID(curthread));
> > +       curtid = TID(curthread);
> > +       if ((uint32_t)thr_malloc_umtx.m_owner == curtid)
> > +               thr_malloc_umtx_level++;
> > +       else
> > +               _thr_umutex_lock(&thr_malloc_umtx, curtid);
> >  }
> >
> >  static void
> > @@ -73,7 +79,10 @@ thr_malloc_unlock(struct pthread *curthread)
> >
> >         if (curthread == NULL)
> >                 return;
> > -       _thr_umutex_unlock(&thr_malloc_umtx, TID(curthread));
> > +       if (thr_malloc_umtx_level > 0)
> > +               thr_malloc_umtx_level--;
> 
> Should this also assert that "thr_malloc_umtx.m_owner == curtid"?
I think a generic principle for userspace libraries is to not abort
the application.  Ideally we would return some error indicating memory
corruption, but libthr is not set up for this.

There are some asserts in libthr that are currently always compiled in.
I wanted to make libthr in stable branches to avoid any assertions, but
did not handled that.