Date: Fri, 21 Nov 2014 15:01:23 +0200 From: Andriy Gapon <avg@FreeBSD.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Jung-uk Kim <jkim@FreeBSD.org>, "freebsd-arch@freebsd.org" <freebsd-arch@FreeBSD.org> Subject: Re: suspending threads before devices Message-ID: <546F37A3.8090506@FreeBSD.org> In-Reply-To: <20141120142806.GJ17068@kib.kiev.ua> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?546F37A3.8090506>