From owner-freebsd-arch@FreeBSD.ORG Thu Aug 25 11:34:50 2011 Return-Path: Delivered-To: freebsd-arch@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A248F106566C; Thu, 25 Aug 2011 11:34:50 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id C0BBD8FC0A; Thu, 25 Aug 2011 11:34:49 +0000 (UTC) Received: from odyssey.starpoint.kiev.ua (alpha-e.starpoint.kiev.ua [212.40.38.101]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id OAA02587; Thu, 25 Aug 2011 14:34:45 +0300 (EEST) (envelope-from avg@FreeBSD.org) Message-ID: <4E563354.5020106@FreeBSD.org> Date: Thu, 25 Aug 2011 14:34:44 +0300 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:5.0) Gecko/20110705 Thunderbird/5.0 MIME-Version: 1.0 To: Hans Petter Selasky , John Baldwin References: <4E53986B.5000804@FreeBSD.org> <201108230911.09021.jhb@freebsd.org> <201108251235.15853.hselasky@c2i.net> In-Reply-To: <201108251235.15853.hselasky@c2i.net> X-Enigmail-Version: 1.2pre Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: freebsd-arch@FreeBSD.org Subject: Re: skipping locks, mutex_owned, usb X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Aug 2011 11:34:50 -0000 on 25/08/2011 13:35 Hans Petter Selasky said the following: > On Tuesday 23 August 2011 15:11:08 John Baldwin wrote: >>> I. [Why] do we need this pattern? >>> Can the code be re-written in a smarter (if not to say proper) way? >> > > Hi, > >> Giant is recursive, it should just be always acquired. Also, this >> recursive call pattern is very non-obvious. A far more straightforward >> approach would be to just have: > > Unless Witness is changed, that won't work. It will lead to LOR warnings I > think. > > Imagine that the root function locks Giant, then another lock is locked and > then ukbd_poll() is called. Won't the second lock of Giant cause a LOR > warning? I think no. At least that's how I interpret the following snippet in witness_checkorder(): /* * Check to see if we are recursing on a lock we already own. If * so, make sure that we don't mismatch exclusive and shared lock * acquires. */ lock1 = find_instance(lock_list, lock); if (lock1 != NULL) { if ((lock1->li_flags & LI_EXCLUSIVE) != 0 && (flags & LOP_EXCLUSIVE) == 0) { printf("shared lock of (%s) %s @ %s:%d\n", class->lc_name, lock->lo_name, file, line); printf("while exclusively locked from %s:%d\n", lock1->li_file, lock1->li_line); panic("share->excl"); } if ((lock1->li_flags & LI_EXCLUSIVE) == 0 && (flags & LOP_EXCLUSIVE) != 0) { printf("exclusive lock of (%s) %s @ %s:%d\n", class->lc_name, lock->lo_name, file, line); printf("while share locked from %s:%d\n", lock1->li_file, lock1->li_line); panic("excl->share"); } return; } Because of the return statement we do not seem to be doing any additional order checking in the case of recursion. >> static int >> ukbd_poll(keyboard_t *kbd, int on) >> { >> struct ukbd_softc *sc = kbd->kb_data; >> >> mtx_lock(&Giant); >> if (on) { >> sc->sc_flags |= UKBD_FLAG_POLLING; >> sc->sc_poll_thread = curthread; >> } else { >> sc->sc_flags &= ~UKBD_FLAG_POLLING; >> ukbd_start_timer(sc); /* start timer */ >> } >> mtx_unlock(&Giant); >> return (0); >> } >> > >> Most code should not be abusing mtx_owned() in this fashion. For Giant you >> should just follow a simple pattern like above that lets it recurse. For >> your own locks you generally should use things like mtx_assert() to >> require all callers of a given routine to hold the lock rather than >> recursively acquiring the lock. Very few legitimate cases of mtx_owned() >> exist IMO. It is debatable if we should even have a mtx_owned() routine >> since we have mtx_assert(). > > How would you solve the LOR case? > > --HPS -- Andriy Gapon