PM: Add suspend blocking work.
in [Kernel]
|
Prev: KVM MMU: allow more page become unsync at getting sp time
Next: [PATCH RESEND 1/2] Composite framework: Add suspended sysfs entry
From: Tejun Heo on 28 Apr 2010 03:20 Hello, On 04/28/2010 09:02 AM, Arve Hj�nnev�g wrote: >> Maybe work->active can be an atomic_t and the lock can be removed? > > I need the spinlock to prevent the work from getting re-queued before > suspend_unblock. OIC. > I'm not sure what the best terminology is here, but cancel_work_sync() > only waits for work running on all the cpu-workqueues of the last > workqueue. So, if the caller queued the work on more than one > workqueue, suspend_blocking_work_destroy does not ensure that the > suspend_blocking_work structure is not still in use (it should trigger > the WARN_ON though). Right, I was thinking about different cpu_workqueues and yeah, the terminology gets pretty confusing. Acked-by: Tejun Heo <tj(a)kernel.org> Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 28 Apr 2010 15:50 On 04/27, Arve Hj�nnev�g wrote: > > Allow work to be queued that will block suspend while it is pending > or executing. To get the same functionality in the calling code often > requires a separate suspend_blocker for pending and executing work, or > additional state and locking. This implementation does add additional > state and locking, but this can be removed later if we add support for > suspend blocking work to the core workqueue code. I think this patch is fine. Just one silly question, > +int queue_suspend_blocking_work(struct workqueue_struct *wq, > + struct suspend_blocking_work *work) > +{ > + int ret; > + unsigned long flags; > + > + spin_lock_irqsave(&work->lock, flags); > + suspend_block(&work->suspend_blocker); > + ret = queue_work(wq, &work->work); > + if (ret) > + work->active++; why not ret = queue_work(wq, &work->work); if (ret) { suspend_block(&work->suspend_blocker); work->active++; } ? Afaics, we can't race with work->func() doing unblock, we hold work-lock. And this way the code looks more clear. Sorry, I had no chance to read the previous patches. After the quick look at 1/8 I think it is OK to call suspend_block() twice, but still... Or I missed something? Just curious. Hmm... actually, queue_work() can also fail if we race with cancel_ which temporary sets WORK_STRUCT_PENDING. In that case suspend_block() won't be paired by unblock ? > +int schedule_suspend_blocking_work(struct suspend_blocking_work *work) > +{ > ... > + ret = schedule_work(&work->work); Off-topic. We should probably export keventd_wq to avoid the duplications like this. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on 28 Apr 2010 16:30 On 04/28/2010 09:40 PM, Oleg Nesterov wrote: >> +int schedule_suspend_blocking_work(struct suspend_blocking_work *work) >> +{ >> ... >> + ret = schedule_work(&work->work); > > Off-topic. We should probably export keventd_wq to avoid the duplications > like this. Yeah, had about the same thought. cmwq exports it so I didn't suggest it at this point but then again we don't really know whether or when that series is going in so it might be a good idea to make that change now. Hmm... Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Rafael J. Wysocki on 28 Apr 2010 17:10 On Wednesday 28 April 2010, Tejun Heo wrote: > On 04/28/2010 09:40 PM, Oleg Nesterov wrote: > >> +int schedule_suspend_blocking_work(struct suspend_blocking_work *work) > >> +{ > >> ... > >> + ret = schedule_work(&work->work); > > > > Off-topic. We should probably export keventd_wq to avoid the duplications > > like this. > > Yeah, had about the same thought. cmwq exports it so I didn't suggest > it at this point but then again we don't really know whether or when > that series is going in As soon as there are no major objections. At least to my tree. > so it might be a good idea to make that change now. Hmm... I'd rather like a follow-up patch changing that, if poss. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Rafael J. Wysocki on 28 Apr 2010 17:10
On Wednesday 28 April 2010, Oleg Nesterov wrote: > On 04/27, Arve Hj�nnev�g wrote: > > > > Allow work to be queued that will block suspend while it is pending > > or executing. To get the same functionality in the calling code often > > requires a separate suspend_blocker for pending and executing work, or > > additional state and locking. This implementation does add additional > > state and locking, but this can be removed later if we add support for > > suspend blocking work to the core workqueue code. > > I think this patch is fine. > > Just one silly question, > > > +int queue_suspend_blocking_work(struct workqueue_struct *wq, > > + struct suspend_blocking_work *work) > > +{ > > + int ret; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&work->lock, flags); > > + suspend_block(&work->suspend_blocker); > > + ret = queue_work(wq, &work->work); > > + if (ret) > > + work->active++; > > why not > > ret = queue_work(wq, &work->work); > if (ret) { > suspend_block(&work->suspend_blocker); > work->active++; > } > > ? > > Afaics, we can't race with work->func() doing unblock, we hold work-lock. > And this way the code looks more clear. Agreed. Arve, any objections to doing that? > Sorry, I had no chance to read the previous patches. After the quick look > at 1/8 I think it is OK to call suspend_block() twice, but still... It is. > Or I missed something? Just curious. > > > Hmm... actually, queue_work() can also fail if we race with cancel_ which > temporary sets WORK_STRUCT_PENDING. In that case suspend_block() won't > be paired by unblock ? > > > > +int schedule_suspend_blocking_work(struct suspend_blocking_work *work) > > +{ > > ... > > + ret = schedule_work(&work->work); > > Off-topic. We should probably export keventd_wq to avoid the duplications > like this. Please see my reply to Tejun. :-) Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |