svn commit: r338802 - head/sys/vm

[Shawn Webb]

On Wed, Sep 19, 2018 at 06:12:29PM +0200, Mateusz Guzik wrote:
> On Wed, Sep 19, 2018 at 6:08 PM, Shawn Webb <shawn.webb at hardenedbsd.org>
> wrote:
> 
> > On Wed, Sep 19, 2018 at 04:02:34PM +0000, Mateusz Guzik wrote:
> > > Author: mjg
> > > Date: Wed Sep 19 16:02:33 2018
> > > New Revision: 338802
> > > URL: https://svnweb.freebsd.org/changeset/base/338802
> > >
> > > Log:
> > >   vm: check for empty kstack cache before locking
> > >
> > >   The current cache logic checks the total number of stacks in the
> > kernel,
> > >   which even on small boxes significantly exceeds the 128 limit (e.g. an
> > >   8-way box with zfs has almost 800 stacks allocated).
> > >
> > >   Stacks are cached earlier for each main thread.
> > >
> > >   As a result the code is rarely executed, but when it is then (on boxes
> > like
> > >   the above) it always fails. Since there are no provisions made for
> > NUMA and
> > >   release time is approaching, just do a quick check to avoid acquiring
> > the
> > >   lock.
> > >
> > >   Approved by:        re (kib)
> > >
> > > Modified:
> > >   head/sys/vm/vm_glue.c
> > >
> > > Modified: head/sys/vm/vm_glue.c
> > > ============================================================
> > ==================
> > > --- head/sys/vm/vm_glue.c     Wed Sep 19 15:39:16 2018        (r338801)
> > > +++ head/sys/vm/vm_glue.c     Wed Sep 19 16:02:33 2018        (r338802)
> > > @@ -327,7 +327,7 @@ vm_thread_new(struct thread *td, int pages)
> > >       else if (pages > KSTACK_MAX_PAGES)
> > >               pages = KSTACK_MAX_PAGES;
> > >
> > > -     if (pages == kstack_pages) {
> > > +     if (pages == kstack_pages && kstack_cache != NULL) {
> > >               mtx_lock(&kstack_cache_mtx);
> > >               if (kstack_cache != NULL) {
> > >                       ks_ce = kstack_cache;
> >
> > Since kstack_cache is guaranteed not to be NULL, can the second
> > conditional that checks for kstack_cache not being NULL be removed?
> >
> >
> The one with the lock held? By the time we get there someone else might
> have removed the last cached stack making the pointer NULL.
> 
> Checking a condition before locking and then checking again is a common
> optimization pattern for cases where the condition is likely false.

Ah, I understand now. Thanks for the clarification!

-- 
Shawn Webb
Cofounder and Security Engineer
HardenedBSD

Tor-ified Signal:    +1 443-546-8752
Tor+XMPP+OTR:        lattera at is.a.hacker.sx
GPG Key ID:          0x6A84658F52456EEE
GPG Key Fingerprint: 2ABA B6BD EF6A F486 BE89  3D9E 6A84 658F 5245 6EEE
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20180919/6458dcf4/attachment.sig>