system_nrt_wq, system suspend, and the freezer
Tejun Heo wrote: > > > My question to all of you: Should system_nrt_wq be made freezable, or > > > should I create a new workqueue that is both freezable and > > > non-reentrant? And if I do, which of the usages above should be > > > converted to the new workqueue? > > > > As far as keys are concerned, it's only for garbage collection purposes, so > > having it freezable shouldn't be a problem. > > If freezing is not strictly necessary, please avoid marking it as > freezable. ? The key_garbage_collector work item is marked neither freezable nor unfreezable that I can see. David
system_nrt_wq, system suspend, and the freezer
Alan Stern wrote: > My question to all of you: Should system_nrt_wq be made freezable, or > should I create a new workqueue that is both freezable and > non-reentrant? And if I do, which of the usages above should be > converted to the new workqueue? As far as keys are concerned, it's only for garbage collection purposes, so having it freezable shouldn't be a problem. David
system_nrt_wq, system suspend, and the freezer
On Thu, 16 Feb 2012, Tejun Heo wrote: > Hello, > > On Thu, Feb 16, 2012 at 11:37:33AM -0500, Alan Stern wrote: > > Um. I don't think I can audit all the calls in the kernel that submit > > block requests and determine which ones need to be allowed while a > > system sleep is in progress. > > ??? we need to do that anyway and the ones which should go through are > much smaller than the ones which shouldn't go through. Agreed. I'm just saying that it's too big a job for me to handle by myself. And all the marking has to be done before you plug the unmarked requests; otherwise you could break hibernation. > > Well, there are some dedicated threads that exist for no other purpose > > than to do I/O to devices and to handle hotplug/unplug events. I don't > > see any reason not allow such threads to be freezable. It's a quick, > > convenient method for getting them out of the way. > > Well, it's convenient to use incorrectly. If you look at most of > freezable kthreads, they're sadly broken. I mean, a lot of kthread > users don't even get kthread_should_stop() right. With freezable() > thrown into the mix, it seems hopeless. With wq, it's better as > freezing is handled by wq proper. Even then, I don't know. It just > seems to lead people to think "ooh, I marked it freezable so I don't > have to think about synchronization across PM events. Freezer will > magically solve this for me!". :( Certainly there are issues which need to be considered carefully. That doesn't mean it should never be used. Alan Stern
system_nrt_wq, system suspend, and the freezer
On Thu, 16 Feb 2012, Tejun Heo wrote:
> Hello,
>
> On Thu, Feb 16, 2012 at 10:27:28AM -0500, Jeff Layton wrote:
> > These should all be freezable and we might even be able to get away
> > with WQ_UNBOUND for some of these.
>
> In general, I would recommend specifying as few special attribute as
> possible. If WQ_UNBOUND is necessary (large amount of CPU cycles
> consumed, extremely high concurrency), sure, but I think we're
> generally better off using as default attributes as possible. It just
> makes things much easier later when we need to implement new features
> or update the implementation.
>
> > I think we put most of these in system_nrt_wq because Tejun put an
> > earlier job into that queue when he converted it from slow_work and we
> > just cargo-cult copied that...
> >
> > I'll spend some time looking at this in the next day or two, but I
> > suspect that the right answer is to just move these off of the "public"
> > workqueues altogether.
>
> If freezing & nrt is everything necessary, just create
> system_nrt_freezable_wq and use that.
Here's my proposed patch. If nobody objects, I'll submit it to Rafael
with an appropriate patch description. Then anybody who wants can
convert over to the new workqueue.
Alan Stern
block/genhd.c | 10 +-
include/linux/workqueue.h |4
kernel/workqueue.c|7 ++-
3 files changed, 15 insertions(+), 6 deletions(-)
Index: usb-3.3/block/genhd.c
===
--- usb-3.3.orig/block/genhd.c
+++ usb-3.3/block/genhd.c
@@ -1475,9 +1475,9 @@ static void __disk_unblock_events(struct
intv = disk_events_poll_jiffies(disk);
set_timer_slack(>dwork.timer, intv / 4);
if (check_now)
- queue_delayed_work(system_nrt_wq, >dwork, 0);
+ queue_delayed_work(system_nrt_freezable_wq, >dwork, 0);
else if (intv)
- queue_delayed_work(system_nrt_wq, >dwork, intv);
+ queue_delayed_work(system_nrt_freezable_wq, >dwork, intv);
out_unlock:
spin_unlock_irqrestore(>lock, flags);
}
@@ -1521,7 +1521,7 @@ void disk_flush_events(struct gendisk *d
ev->clearing |= mask;
if (!ev->block) {
cancel_delayed_work(>dwork);
- queue_delayed_work(system_nrt_wq, >dwork, 0);
+ queue_delayed_work(system_nrt_freezable_wq, >dwork, 0);
}
spin_unlock_irq(>lock);
}
@@ -1558,7 +1558,7 @@ unsigned int disk_clear_events(struct ge
/* uncondtionally schedule event check and wait for it to finish */
disk_block_events(disk);
- queue_delayed_work(system_nrt_wq, >dwork, 0);
+ queue_delayed_work(system_nrt_freezable_wq, >dwork, 0);
flush_delayed_work(>dwork);
__disk_unblock_events(disk, false);
@@ -1595,7 +1595,7 @@ static void disk_events_workfn(struct wo
intv = disk_events_poll_jiffies(disk);
if (!ev->block && intv)
- queue_delayed_work(system_nrt_wq, >dwork, intv);
+ queue_delayed_work(system_nrt_freezable_wq, >dwork, intv);
spin_unlock_irq(>lock);
Index: usb-3.3/include/linux/workqueue.h
===
--- usb-3.3.orig/include/linux/workqueue.h
+++ usb-3.3/include/linux/workqueue.h
@@ -289,12 +289,16 @@ enum {
*
* system_freezable_wq is equivalent to system_wq except that it's
* freezable.
+ *
+ * system_nrt_freezable_wq is equivalent to system_nrt_wq except that
+ * it's freezable.
*/
extern struct workqueue_struct *system_wq;
extern struct workqueue_struct *system_long_wq;
extern struct workqueue_struct *system_nrt_wq;
extern struct workqueue_struct *system_unbound_wq;
extern struct workqueue_struct *system_freezable_wq;
+extern struct workqueue_struct *system_nrt_freezable_wq;
extern struct workqueue_struct *
__alloc_workqueue_key(const char *fmt, unsigned int flags, int max_active,
Index: usb-3.3/kernel/workqueue.c
===
--- usb-3.3.orig/kernel/workqueue.c
+++ usb-3.3/kernel/workqueue.c
@@ -253,11 +253,13 @@ struct workqueue_struct *system_long_wq
struct workqueue_struct *system_nrt_wq __read_mostly;
struct workqueue_struct *system_unbound_wq __read_mostly;
struct workqueue_struct *system_freezable_wq __read_mostly;
+struct workqueue_struct *system_nrt_freezable_wq __read_mostly;
EXPORT_SYMBOL_GPL(system_wq);
EXPORT_SYMBOL_GPL(system_long_wq);
EXPORT_SYMBOL_GPL(system_nrt_wq);
EXPORT_SYMBOL_GPL(system_unbound_wq);
EXPORT_SYMBOL_GPL(system_freezable_wq);
+EXPORT_SYMBOL_GPL(system_nrt_freezable_wq);
#define CREATE_TRACE_POINTS
#include
@@ -3833,8 +3835,11 @@ static int __init init_workqueues(void)
WQ_UNBOUND_MAX_ACTIVE);
system_freezable_wq = alloc_workqueue("events_freezable",
WQ_FREEZABLE, 0);
+
system_nrt_wq, system suspend, and the freezer
On Thu, 16 Feb 2012, Tejun Heo wrote: > Hello, (cc'ing Rafael and Jens) > > On Thu, Feb 16, 2012 at 09:41:34AM -0500, Alan Stern wrote: > > My question to all of you: Should system_nrt_wq be made freezable, or > > should I create a new workqueue that is both freezable and > > non-reentrant? And if I do, which of the usages above should be > > converted to the new workqueue? > > Let's make it explicit that the wq is freezable. I'm a bit > uncomfortable with the way it's headed. What we should be doing is > implementing plugging of request queue for all regular requests while > suspend is in progress and then annotate the ones which should go > through. We're trying to do it the other way around. Um. I don't think I can audit all the calls in the kernel that submit block requests and determine which ones need to be allowed while a system sleep is in progress. > Also, in general, I don't think using freezing widely for kernel > threads / wqs is a good idea. Plugging device access at subsystem > layer should cover most cases and we have notifiers to implement such > support and to handle special cases. There are even code paths which > try to determine whether system went through PM operation by looking > at whether %current went through the freezer. IMHO, we'll be better > off with removing freezer support for kthreads. :( Well, there are some dedicated threads that exist for no other purpose than to do I/O to devices and to handle hotplug/unplug events. I don't see any reason not allow such threads to be freezable. It's a quick, convenient method for getting them out of the way. More general-purpose threads, like the async_schedule reservoir and workqueue threads, are a different story. Alan Stern
system_nrt_wq, system suspend, and the freezer
Hello, Jeff. On Thu, Feb 16, 2012 at 01:59:45PM -0500, Jeff Layton wrote: > The other problem here is that we really ought to be submitting the > write completion handler to a workqueue that has WQ_MEM_RECLAIM set. > Since none of the public wq's have that then I guess we'll have to make > our own? Yeap, if a rescuer should be there, a separate wq is necessary. Thank you. -- tejun
system_nrt_wq, system suspend, and the freezer
On Thu, 16 Feb 2012 09:41:34 -0500 (EST) Alan Stern wrote: > Folks: > > I recently uncovered a bug in the block layer. It uses a workqueue to > periodically probe removable drives for media or other state changes, > and the workqueue it uses is system_nrt_wq. > > The bug is that system_nrt_wq is not freezable, so it keeps on running > even while the system is in the process of suspending or hibernating. > Doing I/O to a suspended drive doesn't work well and in some cases > causes nasty problems. Obviously these polls need to stop during a > suspend transition. > > A search through the kernel shows that system_nrt_wq is also used in a > few other subsystems: > > ./fs/cifs/cifssmb.c: queue_work(system_nrt_wq, >work); > ./fs/cifs/cifssmb.c: queue_work(system_nrt_wq, >work); > ./fs/cifs/misc.c: queue_work(system_nrt_wq, > ./fs/cifs/connect.c: queue_delayed_work(system_nrt_wq, >echo, > SMB_ECHO_INTERVAL); > ./fs/cifs/connect.c: queue_delayed_work(system_nrt_wq, _ses->echo, > SMB_ECHO_INTERVAL); > ./fs/cifs/connect.c: queue_delayed_work(system_nrt_wq, > _sb->prune_tlinks, > ./fs/cifs/connect.c: queue_delayed_work(system_nrt_wq, > _sb->prune_tlinks, > These should all be freezable and we might even be able to get away with WQ_UNBOUND for some of these. I think we put most of these in system_nrt_wq because Tejun put an earlier job into that queue when he converted it from slow_work and we just cargo-cult copied that... I'll spend some time looking at this in the next day or two, but I suspect that the right answer is to just move these off of the "public" workqueues altogether. > ./drivers/mmc/core/host.c:queue_work(system_nrt_wq, > >clk_gate_work); > > ./drivers/gpu/drm/drm_crtc_helper.c: > queue_delayed_work(system_nrt_wq, delayed_work, DRM_OUTPUT_POLL_PERIOD); > ./drivers/gpu/drm/drm_crtc_helper.c: > queue_delayed_work(system_nrt_wq, >mode_config.output_poll_work, > DRM_OUTPUT_POLL_PERIOD); > ./drivers/gpu/drm/drm_crtc_helper.c: > queue_delayed_work(system_nrt_wq, >mode_config.output_poll_work, 0); > > ./security/keys/gc.c: queue_work(system_nrt_wq, _gc_work); > ./security/keys/gc.c: queue_work(system_nrt_wq, _gc_work); > ./security/keys/gc.c: queue_work(system_nrt_wq, _gc_work); > ./security/keys/gc.c: queue_work(system_nrt_wq, _gc_work); > ./security/keys/key.c:queue_work(system_nrt_wq, > _gc_work); > > My question to all of you: Should system_nrt_wq be made freezable, or > should I create a new workqueue that is both freezable and > non-reentrant? And if I do, which of the usages above should be > converted to the new workqueue? > > Thanks, > > Alan Stern > > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Jeff Layton
system_nrt_wq, system suspend, and the freezer
Folks: I recently uncovered a bug in the block layer. It uses a workqueue to periodically probe removable drives for media or other state changes, and the workqueue it uses is system_nrt_wq. The bug is that system_nrt_wq is not freezable, so it keeps on running even while the system is in the process of suspending or hibernating. Doing I/O to a suspended drive doesn't work well and in some cases causes nasty problems. Obviously these polls need to stop during a suspend transition. A search through the kernel shows that system_nrt_wq is also used in a few other subsystems: ./fs/cifs/cifssmb.c:queue_work(system_nrt_wq, >work); ./fs/cifs/cifssmb.c:queue_work(system_nrt_wq, >work); ./fs/cifs/misc.c: queue_work(system_nrt_wq, ./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, >echo, SMB_ECHO_INTERVAL); ./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, _ses->echo, SMB_ECHO_INTERVAL); ./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, _sb->prune_tlinks, ./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, _sb->prune_tlinks, ./drivers/mmc/core/host.c: queue_work(system_nrt_wq, >clk_gate_work); ./drivers/gpu/drm/drm_crtc_helper.c: queue_delayed_work(system_nrt_wq, delayed_work, DRM_OUTPUT_POLL_PERIOD); ./drivers/gpu/drm/drm_crtc_helper.c: queue_delayed_work(system_nrt_wq, >mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD); ./drivers/gpu/drm/drm_crtc_helper.c: queue_delayed_work(system_nrt_wq, >mode_config.output_poll_work, 0); ./security/keys/gc.c: queue_work(system_nrt_wq, _gc_work); ./security/keys/gc.c: queue_work(system_nrt_wq, _gc_work); ./security/keys/gc.c: queue_work(system_nrt_wq, _gc_work); ./security/keys/gc.c: queue_work(system_nrt_wq, _gc_work); ./security/keys/key.c: queue_work(system_nrt_wq, _gc_work); My question to all of you: Should system_nrt_wq be made freezable, or should I create a new workqueue that is both freezable and non-reentrant? And if I do, which of the usages above should be converted to the new workqueue? Thanks, Alan Stern
system_nrt_wq, system suspend, and the freezer
Hello, On Thu, Feb 16, 2012 at 11:37:33AM -0500, Alan Stern wrote: > Um. I don't think I can audit all the calls in the kernel that submit > block requests and determine which ones need to be allowed while a > system sleep is in progress. ??? we need to do that anyway and the ones which should go through are much smaller than the ones which shouldn't go through. > > Also, in general, I don't think using freezing widely for kernel > > threads / wqs is a good idea. Plugging device access at subsystem > > layer should cover most cases and we have notifiers to implement such > > support and to handle special cases. There are even code paths which > > try to determine whether system went through PM operation by looking > > at whether %current went through the freezer. IMHO, we'll be better > > off with removing freezer support for kthreads. :( > > Well, there are some dedicated threads that exist for no other purpose > than to do I/O to devices and to handle hotplug/unplug events. I don't > see any reason not allow such threads to be freezable. It's a quick, > convenient method for getting them out of the way. Well, it's convenient to use incorrectly. If you look at most of freezable kthreads, they're sadly broken. I mean, a lot of kthread users don't even get kthread_should_stop() right. With freezable() thrown into the mix, it seems hopeless. With wq, it's better as freezing is handled by wq proper. Even then, I don't know. It just seems to lead people to think "ooh, I marked it freezable so I don't have to think about synchronization across PM events. Freezer will magically solve this for me!". :( Thanks. -- tejun
system_nrt_wq, system suspend, and the freezer
On Thu, Feb 16, 2012 at 04:35:51PM +, David Howells wrote: > The key_garbage_collector work item is marked neither freezable nor > unfreezable that I can see. Heh, was too brief apparently. :) I was trying to say that if it doesn't require freezing, please don't put it on a freezable workqueue. So, IOW, I was just saying that nothing should change for key_garbage_collector. Thanks. -- tejun
system_nrt_wq, system suspend, and the freezer
Hello, On Thu, Feb 16, 2012 at 10:27:28AM -0500, Jeff Layton wrote: > These should all be freezable and we might even be able to get away > with WQ_UNBOUND for some of these. In general, I would recommend specifying as few special attribute as possible. If WQ_UNBOUND is necessary (large amount of CPU cycles consumed, extremely high concurrency), sure, but I think we're generally better off using as default attributes as possible. It just makes things much easier later when we need to implement new features or update the implementation. > I think we put most of these in system_nrt_wq because Tejun put an > earlier job into that queue when he converted it from slow_work and we > just cargo-cult copied that... > > I'll spend some time looking at this in the next day or two, but I > suspect that the right answer is to just move these off of the "public" > workqueues altogether. If freezing & nrt is everything necessary, just create system_nrt_freezable_wq and use that. Thanks. -- tejun
system_nrt_wq, system suspend, and the freezer
On Thu, Feb 16, 2012 at 03:22:24PM +, David Howells wrote: > Alan Stern wrote: > > > My question to all of you: Should system_nrt_wq be made freezable, or > > should I create a new workqueue that is both freezable and > > non-reentrant? And if I do, which of the usages above should be > > converted to the new workqueue? > > As far as keys are concerned, it's only for garbage collection purposes, so > having it freezable shouldn't be a problem. If freezing is not strictly necessary, please avoid marking it as freezable. Thanks. -- tejun
system_nrt_wq, system suspend, and the freezer
Hello, (cc'ing Rafael and Jens) On Thu, Feb 16, 2012 at 09:41:34AM -0500, Alan Stern wrote: > My question to all of you: Should system_nrt_wq be made freezable, or > should I create a new workqueue that is both freezable and > non-reentrant? And if I do, which of the usages above should be > converted to the new workqueue? Let's make it explicit that the wq is freezable. I'm a bit uncomfortable with the way it's headed. What we should be doing is implementing plugging of request queue for all regular requests while suspend is in progress and then annotate the ones which should go through. We're trying to do it the other way around. Also, in general, I don't think using freezing widely for kernel threads / wqs is a good idea. Plugging device access at subsystem layer should cover most cases and we have notifiers to implement such support and to handle special cases. There are even code paths which try to determine whether system went through PM operation by looking at whether %current went through the freezer. IMHO, we'll be better off with removing freezer support for kthreads. :( Thanks. -- tejun
system_nrt_wq, system suspend, and the freezer
Folks: I recently uncovered a bug in the block layer. It uses a workqueue to periodically probe removable drives for media or other state changes, and the workqueue it uses is system_nrt_wq. The bug is that system_nrt_wq is not freezable, so it keeps on running even while the system is in the process of suspending or hibernating. Doing I/O to a suspended drive doesn't work well and in some cases causes nasty problems. Obviously these polls need to stop during a suspend transition. A search through the kernel shows that system_nrt_wq is also used in a few other subsystems: ./fs/cifs/cifssmb.c:queue_work(system_nrt_wq, rdata-work); ./fs/cifs/cifssmb.c:queue_work(system_nrt_wq, wdata-work); ./fs/cifs/misc.c: queue_work(system_nrt_wq, ./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, server-echo, SMB_ECHO_INTERVAL); ./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, tcp_ses-echo, SMB_ECHO_INTERVAL); ./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, cifs_sb-prune_tlinks, ./fs/cifs/connect.c:queue_delayed_work(system_nrt_wq, cifs_sb-prune_tlinks, ./drivers/mmc/core/host.c: queue_work(system_nrt_wq, host-clk_gate_work); ./drivers/gpu/drm/drm_crtc_helper.c: queue_delayed_work(system_nrt_wq, delayed_work, DRM_OUTPUT_POLL_PERIOD); ./drivers/gpu/drm/drm_crtc_helper.c: queue_delayed_work(system_nrt_wq, dev-mode_config.output_poll_work, DRM_OUTPUT_POLL_PERIOD); ./drivers/gpu/drm/drm_crtc_helper.c: queue_delayed_work(system_nrt_wq, dev-mode_config.output_poll_work, 0); ./security/keys/gc.c: queue_work(system_nrt_wq, key_gc_work); ./security/keys/gc.c: queue_work(system_nrt_wq, key_gc_work); ./security/keys/gc.c: queue_work(system_nrt_wq, key_gc_work); ./security/keys/gc.c: queue_work(system_nrt_wq, key_gc_work); ./security/keys/key.c: queue_work(system_nrt_wq, key_gc_work); My question to all of you: Should system_nrt_wq be made freezable, or should I create a new workqueue that is both freezable and non-reentrant? And if I do, which of the usages above should be converted to the new workqueue? Thanks, Alan Stern ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: system_nrt_wq, system suspend, and the freezer
Hello, (cc'ing Rafael and Jens) On Thu, Feb 16, 2012 at 09:41:34AM -0500, Alan Stern wrote: My question to all of you: Should system_nrt_wq be made freezable, or should I create a new workqueue that is both freezable and non-reentrant? And if I do, which of the usages above should be converted to the new workqueue? Let's make it explicit that the wq is freezable. I'm a bit uncomfortable with the way it's headed. What we should be doing is implementing plugging of request queue for all regular requests while suspend is in progress and then annotate the ones which should go through. We're trying to do it the other way around. Also, in general, I don't think using freezing widely for kernel threads / wqs is a good idea. Plugging device access at subsystem layer should cover most cases and we have notifiers to implement such support and to handle special cases. There are even code paths which try to determine whether system went through PM operation by looking at whether %current went through the freezer. IMHO, we'll be better off with removing freezer support for kthreads. :( Thanks. -- tejun ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: system_nrt_wq, system suspend, and the freezer
On Thu, Feb 16, 2012 at 03:22:24PM +, David Howells wrote: Alan Stern st...@rowland.harvard.edu wrote: My question to all of you: Should system_nrt_wq be made freezable, or should I create a new workqueue that is both freezable and non-reentrant? And if I do, which of the usages above should be converted to the new workqueue? As far as keys are concerned, it's only for garbage collection purposes, so having it freezable shouldn't be a problem. If freezing is not strictly necessary, please avoid marking it as freezable. Thanks. -- tejun ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: system_nrt_wq, system suspend, and the freezer
Hello, On Thu, Feb 16, 2012 at 10:27:28AM -0500, Jeff Layton wrote: These should all be freezable and we might even be able to get away with WQ_UNBOUND for some of these. In general, I would recommend specifying as few special attribute as possible. If WQ_UNBOUND is necessary (large amount of CPU cycles consumed, extremely high concurrency), sure, but I think we're generally better off using as default attributes as possible. It just makes things much easier later when we need to implement new features or update the implementation. I think we put most of these in system_nrt_wq because Tejun put an earlier job into that queue when he converted it from slow_work and we just cargo-cult copied that... I'll spend some time looking at this in the next day or two, but I suspect that the right answer is to just move these off of the public workqueues altogether. If freezing nrt is everything necessary, just create system_nrt_freezable_wq and use that. Thanks. -- tejun ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: system_nrt_wq, system suspend, and the freezer
On Thu, 16 Feb 2012, Tejun Heo wrote: Hello, (cc'ing Rafael and Jens) On Thu, Feb 16, 2012 at 09:41:34AM -0500, Alan Stern wrote: My question to all of you: Should system_nrt_wq be made freezable, or should I create a new workqueue that is both freezable and non-reentrant? And if I do, which of the usages above should be converted to the new workqueue? Let's make it explicit that the wq is freezable. I'm a bit uncomfortable with the way it's headed. What we should be doing is implementing plugging of request queue for all regular requests while suspend is in progress and then annotate the ones which should go through. We're trying to do it the other way around. Um. I don't think I can audit all the calls in the kernel that submit block requests and determine which ones need to be allowed while a system sleep is in progress. Also, in general, I don't think using freezing widely for kernel threads / wqs is a good idea. Plugging device access at subsystem layer should cover most cases and we have notifiers to implement such support and to handle special cases. There are even code paths which try to determine whether system went through PM operation by looking at whether %current went through the freezer. IMHO, we'll be better off with removing freezer support for kthreads. :( Well, there are some dedicated threads that exist for no other purpose than to do I/O to devices and to handle hotplug/unplug events. I don't see any reason not allow such threads to be freezable. It's a quick, convenient method for getting them out of the way. More general-purpose threads, like the async_schedule reservoir and workqueue threads, are a different story. Alan Stern ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: system_nrt_wq, system suspend, and the freezer
On Thu, Feb 16, 2012 at 04:35:51PM +, David Howells wrote: The key_garbage_collector work item is marked neither freezable nor unfreezable that I can see. Heh, was too brief apparently. :) I was trying to say that if it doesn't require freezing, please don't put it on a freezable workqueue. So, IOW, I was just saying that nothing should change for key_garbage_collector. Thanks. -- tejun ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: system_nrt_wq, system suspend, and the freezer
On Thu, 16 Feb 2012, Tejun Heo wrote:
Hello,
On Thu, Feb 16, 2012 at 10:27:28AM -0500, Jeff Layton wrote:
These should all be freezable and we might even be able to get away
with WQ_UNBOUND for some of these.
In general, I would recommend specifying as few special attribute as
possible. If WQ_UNBOUND is necessary (large amount of CPU cycles
consumed, extremely high concurrency), sure, but I think we're
generally better off using as default attributes as possible. It just
makes things much easier later when we need to implement new features
or update the implementation.
I think we put most of these in system_nrt_wq because Tejun put an
earlier job into that queue when he converted it from slow_work and we
just cargo-cult copied that...
I'll spend some time looking at this in the next day or two, but I
suspect that the right answer is to just move these off of the public
workqueues altogether.
If freezing nrt is everything necessary, just create
system_nrt_freezable_wq and use that.
Here's my proposed patch. If nobody objects, I'll submit it to Rafael
with an appropriate patch description. Then anybody who wants can
convert over to the new workqueue.
Alan Stern
block/genhd.c | 10 +-
include/linux/workqueue.h |4
kernel/workqueue.c|7 ++-
3 files changed, 15 insertions(+), 6 deletions(-)
Index: usb-3.3/block/genhd.c
===
--- usb-3.3.orig/block/genhd.c
+++ usb-3.3/block/genhd.c
@@ -1475,9 +1475,9 @@ static void __disk_unblock_events(struct
intv = disk_events_poll_jiffies(disk);
set_timer_slack(ev-dwork.timer, intv / 4);
if (check_now)
- queue_delayed_work(system_nrt_wq, ev-dwork, 0);
+ queue_delayed_work(system_nrt_freezable_wq, ev-dwork, 0);
else if (intv)
- queue_delayed_work(system_nrt_wq, ev-dwork, intv);
+ queue_delayed_work(system_nrt_freezable_wq, ev-dwork, intv);
out_unlock:
spin_unlock_irqrestore(ev-lock, flags);
}
@@ -1521,7 +1521,7 @@ void disk_flush_events(struct gendisk *d
ev-clearing |= mask;
if (!ev-block) {
cancel_delayed_work(ev-dwork);
- queue_delayed_work(system_nrt_wq, ev-dwork, 0);
+ queue_delayed_work(system_nrt_freezable_wq, ev-dwork, 0);
}
spin_unlock_irq(ev-lock);
}
@@ -1558,7 +1558,7 @@ unsigned int disk_clear_events(struct ge
/* uncondtionally schedule event check and wait for it to finish */
disk_block_events(disk);
- queue_delayed_work(system_nrt_wq, ev-dwork, 0);
+ queue_delayed_work(system_nrt_freezable_wq, ev-dwork, 0);
flush_delayed_work(ev-dwork);
__disk_unblock_events(disk, false);
@@ -1595,7 +1595,7 @@ static void disk_events_workfn(struct wo
intv = disk_events_poll_jiffies(disk);
if (!ev-block intv)
- queue_delayed_work(system_nrt_wq, ev-dwork, intv);
+ queue_delayed_work(system_nrt_freezable_wq, ev-dwork, intv);
spin_unlock_irq(ev-lock);
Index: usb-3.3/include/linux/workqueue.h
===
--- usb-3.3.orig/include/linux/workqueue.h
+++ usb-3.3/include/linux/workqueue.h
@@ -289,12 +289,16 @@ enum {
*
* system_freezable_wq is equivalent to system_wq except that it's
* freezable.
+ *
+ * system_nrt_freezable_wq is equivalent to system_nrt_wq except that
+ * it's freezable.
*/
extern struct workqueue_struct *system_wq;
extern struct workqueue_struct *system_long_wq;
extern struct workqueue_struct *system_nrt_wq;
extern struct workqueue_struct *system_unbound_wq;
extern struct workqueue_struct *system_freezable_wq;
+extern struct workqueue_struct *system_nrt_freezable_wq;
extern struct workqueue_struct *
__alloc_workqueue_key(const char *fmt, unsigned int flags, int max_active,
Index: usb-3.3/kernel/workqueue.c
===
--- usb-3.3.orig/kernel/workqueue.c
+++ usb-3.3/kernel/workqueue.c
@@ -253,11 +253,13 @@ struct workqueue_struct *system_long_wq
struct workqueue_struct *system_nrt_wq __read_mostly;
struct workqueue_struct *system_unbound_wq __read_mostly;
struct workqueue_struct *system_freezable_wq __read_mostly;
+struct workqueue_struct *system_nrt_freezable_wq __read_mostly;
EXPORT_SYMBOL_GPL(system_wq);
EXPORT_SYMBOL_GPL(system_long_wq);
EXPORT_SYMBOL_GPL(system_nrt_wq);
EXPORT_SYMBOL_GPL(system_unbound_wq);
EXPORT_SYMBOL_GPL(system_freezable_wq);
+EXPORT_SYMBOL_GPL(system_nrt_freezable_wq);
#define CREATE_TRACE_POINTS
#include trace/events/workqueue.h
@@ -3833,8 +3835,11 @@ static int __init init_workqueues(void)
WQ_UNBOUND_MAX_ACTIVE);
system_freezable_wq = alloc_workqueue(events_freezable,
Re: system_nrt_wq, system suspend, and the freezer
Hello, On Thu, Feb 16, 2012 at 11:37:33AM -0500, Alan Stern wrote: Um. I don't think I can audit all the calls in the kernel that submit block requests and determine which ones need to be allowed while a system sleep is in progress. ??? we need to do that anyway and the ones which should go through are much smaller than the ones which shouldn't go through. Also, in general, I don't think using freezing widely for kernel threads / wqs is a good idea. Plugging device access at subsystem layer should cover most cases and we have notifiers to implement such support and to handle special cases. There are even code paths which try to determine whether system went through PM operation by looking at whether %current went through the freezer. IMHO, we'll be better off with removing freezer support for kthreads. :( Well, there are some dedicated threads that exist for no other purpose than to do I/O to devices and to handle hotplug/unplug events. I don't see any reason not allow such threads to be freezable. It's a quick, convenient method for getting them out of the way. Well, it's convenient to use incorrectly. If you look at most of freezable kthreads, they're sadly broken. I mean, a lot of kthread users don't even get kthread_should_stop() right. With freezable() thrown into the mix, it seems hopeless. With wq, it's better as freezing is handled by wq proper. Even then, I don't know. It just seems to lead people to think ooh, I marked it freezable so I don't have to think about synchronization across PM events. Freezer will magically solve this for me!. :( Thanks. -- tejun ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: system_nrt_wq, system suspend, and the freezer
On Thu, 16 Feb 2012, Tejun Heo wrote: Hello, On Thu, Feb 16, 2012 at 11:37:33AM -0500, Alan Stern wrote: Um. I don't think I can audit all the calls in the kernel that submit block requests and determine which ones need to be allowed while a system sleep is in progress. ??? we need to do that anyway and the ones which should go through are much smaller than the ones which shouldn't go through. Agreed. I'm just saying that it's too big a job for me to handle by myself. And all the marking has to be done before you plug the unmarked requests; otherwise you could break hibernation. Well, there are some dedicated threads that exist for no other purpose than to do I/O to devices and to handle hotplug/unplug events. I don't see any reason not allow such threads to be freezable. It's a quick, convenient method for getting them out of the way. Well, it's convenient to use incorrectly. If you look at most of freezable kthreads, they're sadly broken. I mean, a lot of kthread users don't even get kthread_should_stop() right. With freezable() thrown into the mix, it seems hopeless. With wq, it's better as freezing is handled by wq proper. Even then, I don't know. It just seems to lead people to think ooh, I marked it freezable so I don't have to think about synchronization across PM events. Freezer will magically solve this for me!. :( Certainly there are issues which need to be considered carefully. That doesn't mean it should never be used. Alan Stern ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: system_nrt_wq, system suspend, and the freezer
Hello, Jeff. On Thu, Feb 16, 2012 at 01:59:45PM -0500, Jeff Layton wrote: The other problem here is that we really ought to be submitting the write completion handler to a workqueue that has WQ_MEM_RECLAIM set. Since none of the public wq's have that then I guess we'll have to make our own? Yeap, if a rescuer should be there, a separate wq is necessary. Thank you. -- tejun ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: system_nrt_wq, system suspend, and the freezer
Alan Stern st...@rowland.harvard.edu wrote: My question to all of you: Should system_nrt_wq be made freezable, or should I create a new workqueue that is both freezable and non-reentrant? And if I do, which of the usages above should be converted to the new workqueue? As far as keys are concerned, it's only for garbage collection purposes, so having it freezable shouldn't be a problem. David ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: system_nrt_wq, system suspend, and the freezer
Tejun Heo t...@kernel.org wrote: My question to all of you: Should system_nrt_wq be made freezable, or should I create a new workqueue that is both freezable and non-reentrant? And if I do, which of the usages above should be converted to the new workqueue? As far as keys are concerned, it's only for garbage collection purposes, so having it freezable shouldn't be a problem. If freezing is not strictly necessary, please avoid marking it as freezable. ? The key_garbage_collector work item is marked neither freezable nor unfreezable that I can see. David ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: system_nrt_wq, system suspend, and the freezer
On Thu, 16 Feb 2012 08:29:51 -0800 Tejun Heo t...@kernel.org wrote: Hello, On Thu, Feb 16, 2012 at 10:27:28AM -0500, Jeff Layton wrote: These should all be freezable and we might even be able to get away with WQ_UNBOUND for some of these. In general, I would recommend specifying as few special attribute as possible. If WQ_UNBOUND is necessary (large amount of CPU cycles consumed, extremely high concurrency), sure, but I think we're generally better off using as default attributes as possible. It just makes things much easier later when we need to implement new features or update the implementation. Ok, fair enough. Probably no need to make it unbound... I think we put most of these in system_nrt_wq because Tejun put an earlier job into that queue when he converted it from slow_work and we just cargo-cult copied that... I'll spend some time looking at this in the next day or two, but I suspect that the right answer is to just move these off of the public workqueues altogether. If freezing nrt is everything necessary, just create system_nrt_freezable_wq and use that. The other problem here is that we really ought to be submitting the write completion handler to a workqueue that has WQ_MEM_RECLAIM set. Since none of the public wq's have that then I guess we'll have to make our own? -- Jeff Layton jlay...@samba.org ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
