Messages in this thread | | | Date | Wed, 28 Apr 2010 21:40:28 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 6/8] PM: Add suspend blocking work. |
| |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |