From owner-freebsd-usb@FreeBSD.ORG Tue Aug 23 12:09:19 2011 Return-Path: Delivered-To: freebsd-usb@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B13FC106566C; Tue, 23 Aug 2011 12:09:19 +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 44E0D8FC0C; Tue, 23 Aug 2011 12:09:17 +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 PAA25743; Tue, 23 Aug 2011 15:09:15 +0300 (EEST) (envelope-from avg@FreeBSD.org) Message-ID: <4E53986B.5000804@FreeBSD.org> Date: Tue, 23 Aug 2011 15:09:15 +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: freebsd-arch@FreeBSD.org X-Enigmail-Version: 1.2pre Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Mailman-Approved-At: Tue, 23 Aug 2011 12:29:29 +0000 Cc: Subject: skipping locks, mutex_owned, usb X-BeenThere: freebsd-usb@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Aug 2011 12:09:19 -0000 Yes, the subject sounds quite hairy, so please let me try to explain it. First, let's consider one concrete function: static int ukbd_poll(keyboard_t *kbd, int on) { struct ukbd_softc *sc = kbd->kb_data; if (!mtx_owned(&Giant)) { /* XXX cludge */ int retval; mtx_lock(&Giant); retval = ukbd_poll(kbd, on); mtx_unlock(&Giant); return (retval); } 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 */ } return (0); } This "XXX cludge" [sic] pattern is scattered around a few functions in the ukbd code and perhaps other usb code: func() { if (!mtx_owned(&Giant)) { mtx_lock(&Giant); func(); mtx_unlock(&Giant); return; } // etc ... } I have a few question directly and indirectly related to this pattern. I. [Why] do we need this pattern? Can the code be re-written in a smarter (if not to say proper) way? II. ukbd_poll() in particular can be called from the kdb context (kdb_trap -> db_trap -> db_command_loop -> etc). What would happen if the Giant is held by a thread on some other CPU (which would be hard-stopped by kdp_trap)? Won't we get a lockup here? So shouldn't this code actually check for kdb_active? III. With my stop_scheduler_on_panic patch ukbd_poll() produces infinite chains of 'infinite' recursion because mtx_owned() always returns false. This is because I skip all lock/unlock/etc operations in the post-panic context. I think that it's a good philosophical question: what operations like mtx_owned(), mtx_recursed(), mtx_trylock() 'should' return when we actually act as if no locks exist at all? My first knee-jerk reaction was to change mtx_owned() to return true in this "lock-less" context, but _hypothetically_ there could exist some symmetric code that does something like: func() { if (mtx_owned(&Giant)) { mtx_unlock(&Giant); func(); mtx_lock(&Giant); return; } // etc ... What do you think about this problem? Should we re-write ukbd_poll() (and possibly usb code) or should mutex_owned() be adjusted? That question III actually brings another thought: perhaps the whole of idea of skipping locks in a certain context is not a correct direction? Perhaps, instead we should identify all the code that could be legitimately executed in the after-panic and/or kdb contexts and make that could explicitly aware of its execution context. That is, instead of trying to make _lock, _unlock, _owned, _trylock, etc do the right thing auto-magically, we should try to make the calling code check panicstr and kdb_active and do the right thing on that level (which would include not invoking those lock-related operations or other inappropriate operations). Thank you very much in advance for your insights and help! -- Andriy Gapon