From owner-freebsd-arch@FreeBSD.ORG Fri Nov 21 13:02:29 2014 Return-Path: Delivered-To: freebsd-arch@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 CAD8B6B5; Fri, 21 Nov 2014 13:02:29 +0000 (UTC) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id A56F9D56; Fri, 21 Nov 2014 13:02:28 +0000 (UTC) Received: from porto.starpoint.kiev.ua (porto-e.starpoint.kiev.ua [212.40.38.100]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id PAA04809; Fri, 21 Nov 2014 15:04:13 +0200 (EET) (envelope-from avg@FreeBSD.org) Received: from localhost ([127.0.0.1]) by porto.starpoint.kiev.ua with esmtp (Exim 4.34 (FreeBSD)) id 1Xrnqu-0007T6-0v; Fri, 21 Nov 2014 15:02:20 +0200 Message-ID: <546F37A3.8090506@FreeBSD.org> Date: Fri, 21 Nov 2014 15:01:23 +0200 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Konstantin Belousov Subject: Re: suspending threads before devices References: <201203202037.q2KKbNfK037014@svn.freebsd.org> <201411181721.56505.jhb@freebsd.org> <87FFDA99-ADDC-4F56-A3E8-56CCAA544542@bsdimp.com> <1580793.3ynJAbfVom@ralph.baldwin.cx> <20141120142806.GJ17068@kib.kiev.ua> In-Reply-To: <20141120142806.GJ17068@kib.kiev.ua> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: Jung-uk Kim , "freebsd-arch@freebsd.org" X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 Nov 2014 13:02:29 -0000 On 20/11/2014 16:28, Konstantin Belousov wrote: > Below is the prototyped patch for global process suspension. There is > debugging sysctl debug.total_stop, which demonstrates the KPI, also I > used it for light testing. It successfully survived suspend/resume of > usermode threads in multiuser with mt processes running. I applied this patch and hooked the code to ACPI suspend and I must that this greatly improved chances of suspend + resume working even when initiated during heavy graphic activity on my machine with radeon hardware. So far it hasn't failed a single time through a dozen tests. It even worked kern.vt.suspendswitch=0 which never worked for me before. In addition to your changes I also have have https://reviews.freebsd.org/D1004 for initiating VT switch at a more appropriate moment when kern.vt.suspendswitch=1. But that was not enough, I also had to split power_suspend into power_suspend and power_suspend_early. power_suspend_early is called before the userland is frozen, so that things like aforementioned VT switching could be done. power_suspend is called after the userland is frozen and it is more appropriate for things like powering down disks[*]. The code that I am running is here: https://github.com/avg-I/freebsd/compare/wip/kib-userland-freeze Speaking about the code I probably would like the names to be polished up a little bit. E.g. maybe proc_stop_total -> proc_stop_all or even stop_all_procs. 'Total' is slightly confusing in this context. SINGLE_TOTAL sounds very confusing and non-descriptive to me, but I can not think up a better name at the moment. Also, maybe met_stopped -> seen_stopped. Likewise, did_stop -> stopped_some? Finally, some calls to thread_unsuspend_one() look quite ugly because of line wrapping. Maybe thread_single() code could be restructured / reformatted a little bit to avoid that. Finally, it seems that the code assumes that the calling process (curproc when proc_stop_total is called) is single-threaded. I think that that is a fine assumption, but maybe it needs to be spelled out explicitly and asserted. I should add that the code logic is good, so I only have 'color of bikeshed' comments on its style. So, I like your change a lot! Thanks! [*] And that needs to be done via an event handler only because of the lack of proper integration between the bus code and CAM. -- Andriy Gapon