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: Arve Hjønnevåg on 28 Apr 2010 18:10 2010/4/28 Rafael J. Wysocki <rjw(a)sisk.pl>: > 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? > I need to fix the race, but I can easily fix it in cancel_suspend_blocking_work_sync instead. If the suspend blocker is active for a long time, and DEBUG_SUSPEND_BLOCKER is enabled, we can tell if the work is constantly re-queued or if the workqueue is stuck. >> 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 > -- Arve Hj�nnev�g -- 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 18:20 On Thursday 29 April 2010, Arve Hj�nnev�g wrote: > 2010/4/28 Rafael J. Wysocki <rjw(a)sisk.pl>: > > 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? > > > > I need to fix the race, but I can easily fix it in > cancel_suspend_blocking_work_sync instead. If the suspend blocker is > active for a long time, and DEBUG_SUSPEND_BLOCKER is enabled, we can > tell if the work is constantly re-queued or if the workqueue is stuck. Well, perhaps that's worth adding a comment to the code. The debug part is not immediately visible from the code itself. 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: Arve Hjønnevåg on 28 Apr 2010 23:50 2010/4/28 Rafael J. Wysocki <rjw(a)sisk.pl>: > On Thursday 29 April 2010, Arve Hj�nnev�g wrote: >> 2010/4/28 Rafael J. Wysocki <rjw(a)sisk.pl>: >> > 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? >> > >> >> I need to fix the race, but I can easily fix it in >> cancel_suspend_blocking_work_sync instead. If the suspend blocker is >> active for a long time, and DEBUG_SUSPEND_BLOCKER is enabled, we can >> tell if the work is constantly re-queued or if the workqueue is stuck. > > Well, perhaps that's worth adding a comment to the code. �The debug part is not > immediately visible from the code itself. On second thought, this only makes a difference if both conditions are true. If we are constantly re-queuing the work but it is not stuck, either method will show the debug message, so I used Oleg's suggestion. -- Arve Hj�nnev�g -- 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 30 Apr 2010 14:00 On Thursday 29 April 2010, Arve Hj�nnev�g wrote: > 2010/4/28 Rafael J. Wysocki <rjw(a)sisk.pl>: > > On Thursday 29 April 2010, Arve Hj�nnev�g wrote: > >> 2010/4/28 Rafael J. Wysocki <rjw(a)sisk.pl>: > >> > 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? > >> > > >> > >> I need to fix the race, but I can easily fix it in > >> cancel_suspend_blocking_work_sync instead. If the suspend blocker is > >> active for a long time, and DEBUG_SUSPEND_BLOCKER is enabled, we can > >> tell if the work is constantly re-queued or if the workqueue is stuck. > > > > Well, perhaps that's worth adding a comment to the code. The debug part is not > > immediately visible from the code itself. > > On second thought, this only makes a difference if both conditions are > true. If we are constantly re-queuing the work but it is not stuck, > either method will show the debug message, so I used Oleg's > suggestion. OK, great. 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: Pavel Machek on 30 Apr 2010 14:00
Hi! > 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. > > Signed-off-by: Arve Hj??nnev??g <arve(a)android.com> Seems sane. ACK. > +struct suspend_blocking_work { > + struct work_struct work; > +#ifdef CONFIG_OPPORTUNISTIC_SUSPEND Cound we name it C_AUTO_SUSPEND... to reduce length and typo potential? > + struct suspend_blocker suspend_blocker; > + work_func_t func; > + spinlock_t lock; > + int active; Is the lock internal-use, or is API user allowed to use it? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- 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/ |