Date: Mon, 29 Sep 2014 11:02:20 -0500 From: Bryan Venteicher <bryanv@daemoninthecloset.org> To: John Baldwin <jhb@freebsd.org> Cc: "freebsd-hackers@freebsd.org" <freebsd-hackers@freebsd.org>, jeff@freebsd.org Subject: Re: Change uma_mtx to rwlock Message-ID: <CAMo0n6SF5KUefTigP=QxEaCKeeMf6Mav_S2pEeqob3xmmNn=1w@mail.gmail.com> In-Reply-To: <1458140.gGPpU3NGiG@ralph.baldwin.cx> References: <CAMo0n6Q=P5H3%2BCqr8KjFRVLvWHZfJYnROVe4xF3DmKw95D%2B5zQ@mail.gmail.com> <1458140.gGPpU3NGiG@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Sep 29, 2014 at 10:27 AM, John Baldwin <jhb@freebsd.org> wrote:
> On Saturday, September 27, 2014 07:59:47 PM Bryan Venteicher wrote:
> > Hi,
> >
> > I'd appreciate some comments attached patch that changes the uma_mtx to a
> > rwlock.
> >
> > At $JOB, we have machines with ~400GB RAM, with much of that being
> > allocated through UMA zones. We've observed that timeouts were sometimes
> > unexpectedly delayed by a half second or more. We tracked one of the
> > reasons for this down to when the page daemon was running, calling
> > uma_reclaim() -> zone_foreach(). zone_foreach() holds the uma_mtx while
> > zone_drain()'ing each zone. If uma_timeout() fires, it will block on the
> > uma_mtx when it tries to zone_timeout() each zone.
>
> The only nit I see is in zone_drain_wait(). It would be nice to not need
> the
> hack of checking for a read or write lock and just require the one it
> actually
> needs depending on the callers.
>
However, checking the code in HEAD, this appears to just be broken.
> Specifically, zone_drain_wait() is called in two places:
>
> void
> zone_drain(uma_zone_t zone)
> {
>
> zone_drain_wait(zone, M_NOWAIT);
> }
>
> ...
>
>
> static void
> zone_dtor(void *arg, int size, void *udata)
> {
> ...
> mtx_lock(&uma_mtx);
> LIST_REMOVE(zone, uz_link);
> mtx_unlock(&uma_mtx);
> /*
> * XXX there are some races here where
> * the zone can be drained but zone lock
> * released and then refilled before we
> * remove it... we dont care for now
> */
> zone_drain_wait(zone, M_WAITOK);
> ...
> }
>
> Neither one calls it with the uma_mtx locked! This appears to have been
> broken since that function was introduced in r187681.
>
>
Indeed. I had noticed and mentioned that when I sent this patch to jeff@ a
few months ago:
When zone_dtor() calls zone_drain_wait(), should it hold the uma_{mtx,
rwlock}? Can the zone
not be in the DRAINING state at this point? Similarly, does the while
draining loop in
zone_drain_wait() then take the uma_mtx and the zone lock out of
order after the msleep().
But I was just trying to clear out my queue a bit, and hadn't looked at
the HEAD UMA in awhile, so I was going to double check that later.
I think it might be best to first remove the unlock/lock of uma_mtx from
> zone_drain_wait() (so it can be MFC'd). That then simplifies that one
> part of
> your patch (which I think is otherwise fine).
>
>
I'll try to get a review started in Phabric soon.
> --
> John Baldwin
> _______________________________________________
> freebsd-hackers@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAMo0n6SF5KUefTigP=QxEaCKeeMf6Mav_S2pEeqob3xmmNn=1w>