From owner-freebsd-hackers@FreeBSD.ORG Mon Sep 29 16:02:46 2014 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id BE03BD8E for ; Mon, 29 Sep 2014 16:02:46 +0000 (UTC) Received: from mail-ie0-f175.google.com (mail-ie0-f175.google.com [209.85.223.175]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 89DE9E85 for ; Mon, 29 Sep 2014 16:02:46 +0000 (UTC) Received: by mail-ie0-f175.google.com with SMTP id y20so5245960ier.6 for ; Mon, 29 Sep 2014 09:02:40 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=yvjtphGDvZfVVOsmICWNIaPe3hXvF3kMutmC97jup6A=; b=dOgTcHDJVhWh+Gm459tDUhA93CixaMuISHaveaTcPilCQFZn1dL0vCtCAGrPg6yz3c sqHYdZKTYM0mdqbf0w1PrJ5HPB+vgJytzsFVxNDRRWwALkyohZ+RQhlB/MihDqTpiOGm HakS0B/cUckVVouE8UDFfITYzKfYjq+oCF+H6LD6ngcM2L9/FRXRfhksG8C3bP63P9Qf pGV6IHG8g6uCEx1ocp0d06VlW7sKXtK/gPMjInux4QjdSmNwjIuVjt9x7vpFdOKja6QZ qCdX834H1sCXQe+n/ucfaryyck9vjdbKT8A3+CThvPI3wz6H9c3Pf7hyE+Cn/K64El5f z5wA== X-Gm-Message-State: ALoCoQlho7pTdnd49PbMcKmG0cA5hrBvx108aToyl2cjvBtGTjwyVlSw+nTAtBfmmGrxdEw6euMU X-Received: by 10.50.109.228 with SMTP id hv4mr36301701igb.13.1412006560276; Mon, 29 Sep 2014 09:02:40 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.9.67 with HTTP; Mon, 29 Sep 2014 09:02:20 -0700 (PDT) X-Originating-IP: [216.240.30.23] In-Reply-To: <1458140.gGPpU3NGiG@ralph.baldwin.cx> References: <1458140.gGPpU3NGiG@ralph.baldwin.cx> From: Bryan Venteicher Date: Mon, 29 Sep 2014 11:02:20 -0500 Message-ID: Subject: Re: Change uma_mtx to rwlock To: John Baldwin Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.18-1 Cc: "freebsd-hackers@freebsd.org" , jeff@freebsd.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Sep 2014 16:02:46 -0000 On Mon, Sep 29, 2014 at 10:27 AM, John Baldwin 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 sometime= s > > 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 th= e > > 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. > > =E2=80=8BIndeed. 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().=E2=80=8B =E2=80=8BBut I was just trying to clear out my queue a bit, and hadn't look= ed 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). > > =E2=80=8BI'll try to get a review started in Phabric =E2=80=8Bsoon. > -- > 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= " >