From owner-cvs-src@FreeBSD.ORG Tue Apr 29 13:59:17 2003 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 65F4F37B405 for ; Tue, 29 Apr 2003 13:59:17 -0700 (PDT) Received: from mail.speakeasy.net (mail16.speakeasy.net [216.254.0.216]) by mx1.FreeBSD.org (Postfix) with ESMTP id E2F6B43F85 for ; Tue, 29 Apr 2003 13:59:14 -0700 (PDT) (envelope-from jhb@FreeBSD.org) Received: (qmail 20450 invoked from network); 29 Apr 2003 20:59:23 -0000 Received: from unknown (HELO server.baldwin.cx) ([216.27.160.63]) (envelope-sender )encrypted SMTP for ; 29 Apr 2003 20:59:23 -0000 Received: from laptop.baldwin.cx (gw1.twc.weather.com [216.133.140.1]) by server.baldwin.cx (8.12.8/8.12.8) with ESMTP id h3TKxAOv019524; Tue, 29 Apr 2003 16:59:11 -0400 (EDT) (envelope-from jhb@FreeBSD.org) Message-ID: X-Mailer: XFMail 1.5.4 on FreeBSD X-Priority: 3 (Normal) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit MIME-Version: 1.0 In-Reply-To: <16046.55840.945040.149302@grasshopper.cs.duke.edu> Date: Tue, 29 Apr 2003 16:59:16 -0400 (EDT) From: John Baldwin To: Andrew Gallatin cc: cvs-src@FreeBSD.org cc: src-committers@FreeBSD.org cc: "M. Warner Losh" cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/fxp if_fxp.c if_fxpvar.h X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Apr 2003 20:59:18 -0000 On 29-Apr-2003 Andrew Gallatin wrote: > > Where would you do this check for more pending handlers? In detach? > > What does detach do when it's true? The problem is that detach may > > It sleeps. > > > be too late. Take this sequence: > > > > > > CPU 0 CPU 1 > > -------- -------- > > foo_detach() ... > > FOO_LOCK() interrupt for foo > > ... foo_intr() > > ... FOO_LOCK() and blocks > > You forgot: > prevent_device_from_generating_interrupts(); > FOO_UNLOCK(); > > > bus_teardown_intr() This doesn't do any good though. When you FOO_UNLOCK, CPU 1 is _STILL_ going to resume execution of foo_intr(). Depending on if CPU 1 gets interrupted some more before it finishes foo_intr(), CPU 0 can finish all of detach before CPU 1 finishes foo_intr(). How do you handle that? [ Reading on it seems you advocate the sleep in detach to ensure you don't go beyond bus_teardown_intr() in detach(). ] > In foo_detach you disable interrupts on the board, and you replace its > handler with an orphan handler for the benefit of shared irqs. At the > point there can be exactly one interrupt pending which may be blocked > anywere, possibly on the foo lock. You then drop the foo lock. > > You then grab the required locks, and see if foo's ithread is running, > or runnable. If it is, you drop the locks, sleep for hz and repeat > the check. If its not, you do the guts of ithread remove handler. Your orphan handler doesn't buy you anything. Due to IH_DEAD you don't have to worry about that step, IH_DEAD already ensures that you only have the one race case we are talking about to deal with. Basically though, you are moving the sleep from inside bus_teardown_intr() out into each driver, increasing the maintenance load on the driver author. Also, you still haven't solved the problem that it may be _very_ bad to touch foo's registers in foo_intr() after you've gotten into foo_detach() since it could re-enable interrupts, etc. You might be able to work around that on a case-by-case basis by tweaking the order of shutdown, but that makes it harder to write drivers. Also, it's only a win if you avoid a test for the special case in foo_intr(). Hmmmm. You almost want a way to turn off the interrupt handler as the very first thing (maybe just mark it as dead if it is still running, but not block) while holding the driver lock, then turn off the ability to generate interrupts, then sit and wait for the interrupt handler to remove itself. Maybe: mtx_lock(&sc->sc_mtx); /* handler no longer executes */ bus_disable_intr(inthand_cookie, &sc->sc_mtx); /* dink with device */ mtx_unlock(&sc->sc_mtx); bus_teardown_intr(inthande_cookie); With bus_disable_intr() calling a function like this: void ithread_disable_handler(void *cookie, struct mtx *m) { struct intrhand *ih; struct ithread *it; ih = (struct intrhand *)cookie; it = ih->ih_it; mtx_lock(&it->it_lock); if (ithread_is_running) { it->it_need = 1; ih->ih_flags |= IH_DISABLING; ih->ih_mutex = m; mtx_unlock(&it->it_lock); msleep(ih, m, ...); } else { ih->ih_flags |= IH_DISABLED; mtx_unlock(&it->it_lock); } } and some changes to ithread_loop() along the lines of: CTR6(... /* existing */) if (ih->ih_flags & IH_DISABLED) continue; if (ih->ih_flags & IH_DISABLING) { struct mtx *m; m = ih->ih_mutex; ih->ih_flags |= IH_DISABLED; ih->ih_flags &= ~IH_DISABLING; mtx_unlock(&it->it_lock); mtx_lock(&m); wakeup(ih); mtx_unlock(&m); goto restart; } if (ih->ih_flags & IH_DEAD) { TAILQ_REMOVE(...); wakeup(ih); continue; } I would need to hold the ithread lock around the TAILQ_FOREACH() bit and drop it around the execution of the handlers, but I probably need that anyways. Of course, all this really does, is take your gone test and move it up in the stack. You are still checking a flag on every interrupt handler invocation. > > > We really need to think about efficiency. Our 5.x performance sucks. > > > Really sucks. We're being nickled and dimed to death by extra > > > instructions here, there, and everywhere. > > > > Unfortunately 5.x attempts to run with a thread-safe kernel, and that > > involves extra overhead to work around races that 4.x didn't even have > > to dream about. In theory the increased performance should come from > > increased parallelism at the cost of increased overhead. If FreeBSD > > No, in theory increased performance should come from increased > parallelism with no increased overhead. Any increased overhead is a > bug. Linux 2.4 runs a thread safe kernel with less overhead than we > have in 4.x. Its possible. How are we going to achieve increased paralellism w/o increased overhead? Discounting redesigning algo's which would have been a win in the non-parallel kernel as well. A mutex is far more expensive than an spl. You have to protect against more things. Of course overhead is going to go up. > As we get closer to a stable branchpoing, and continue to suck, I'm > starting to think we should start over. Well, the Project is free to choose that if it wishes. -- John Baldwin <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/