From owner-cvs-all@FreeBSD.ORG Wed May 31 14:58:59 2006 Return-Path: X-Original-To: cvs-all@freebsd.org Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 75CD216B1B3; Wed, 31 May 2006 14:58:59 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from pooker.samsco.org (pooker.samsco.org [168.103.85.57]) by mx1.FreeBSD.org (Postfix) with ESMTP id 95DA243D58; Wed, 31 May 2006 14:58:52 +0000 (GMT) (envelope-from scottl@samsco.org) Received: from [10.10.3.185] ([69.15.205.254]) (authenticated bits=0) by pooker.samsco.org (8.13.4/8.13.4) with ESMTP id k4VEwhoW098789; Wed, 31 May 2006 08:58:49 -0600 (MDT) (envelope-from scottl@samsco.org) Message-ID: <447DAF19.3050901@samsco.org> Date: Wed, 31 May 2006 08:58:33 -0600 From: Scott Long User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.12) Gecko/20060206 X-Accept-Language: en-us, en MIME-Version: 1.0 To: John Baldwin References: <200605252306.k4PN6cCS081708@repoman.freebsd.org> <44766F75.9060100@samsco.org> <200605311050.12156.jhb@freebsd.org> In-Reply-To: <200605311050.12156.jhb@freebsd.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=0.0 required=3.8 tests=none autolearn=failed version=3.1.1 X-Spam-Checker-Version: SpamAssassin 3.1.1 (2006-03-10) on pooker.samsco.org Cc: cvs-src@freebsd.org, src-committers@freebsd.org, Warner Losh , cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/dev/syscons/apm apm_saver.c src/sys/i386/bios apm.c apm.h X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 May 2006 14:59:05 -0000 John Baldwin wrote: > On Thursday 25 May 2006 23:01, Scott Long wrote: > >>Warner Losh wrote: >> >> >>>imp 2006-05-25 23:06:38 UTC >>> >>> FreeBSD src repository >>> >>> Modified files: >>> sys/dev/syscons/apm apm_saver.c >>> sys/i386/bios apm.c apm.h >>> Log: >>> APM was calling the suspend process from a timeout. This meant that >>> other timeouts could not happen while suspending, including timeouts >>> for things like msleep. This caused the system to hang on suspend >>> when the cbb was enabled, since its suspend path powered down the >>> socket which used a timeout to wait for it to be done. >>> >>> APM now creates a thread when it is enabled, and deletes the thread >>> when it is disabled. This thread takes the place of the timeout by >>> doing its polling every ~.9s. When the thread is disabled, it will >>> wakeup early, otherwise it times out and polls the varius things the >>> old timeout polled (APM events, suspend delays, etc). >>> >>> This makes my Sony VAIO 505TS suspend/resume correctly when APM is >>> enabled (ACPI is black listed on my 505TS). >>> >>> This will likely fix other problems with the suspend path where >>> drivers would sleep with msleep and/or do other timeouts. Maybe >>> there's some special case code that would use DELAY while suspending >>> and msleep otherwise that can be revisited and removed. >>> >>> This was also tested by glebius@, who pointed out that in the patch I >>> sent him, I'd forgotten apm_saver.c >>> >>> MFC After: 3 weeks >> >>In the past, I've been against mandating that callouts/timeouts/generic >>taskqueues should not be allowed to sleep. However, after looking over >>the history of this problem as well as others, it seems that it's just >>too easy for driver authors to make bad assumptions and wind up with a >>priority inversion/deadlock like this. It would be relatively trivial >>to mark these contexts as being non-sleepable and have the msleep code >>enforce it, like is done with ithreads. What do you think? Anyways, >>thanks for looking at this and fixing it. > > > We already do for timeouts if INVARIANTS is on: > > softclock() > { > ... > THREAD_NO_SLEEPING(); > c_func(c_arg); > THREAD_SLEEPING_OK(); > ... > } > > That has been in place since 6.0 IIRC. > I thought that it was only enabled for DIAGNOSTIC. Scott