On Mon 2016-01-25 13:57:47, Tejun Heo wrote:
> On Mon, Jan 25, 2016 at 04:44:56PM +0100, Petr Mladek wrote:
> > +static void insert_kthread_work_sanity_check(struct kthread_worker *worker,
> > + struct kthread_work *work)
> > +{
> > + lockdep_assert_held(&worker->lock);
> > + WARN_ON_ONCE(!irqs_disabled());
>
> Isn't worker->lock gonna be a irq-safe lock? If so, why would this
> need to be tested separately?
I see. I'll remove it.
> > + WARN_ON_ONCE(!list_empty(&work->node));
> > + /* Do not use a work with more workers, see queue_kthread_work() */
> > + WARN_ON_ONCE(work->worker && work->worker != worker);
> > +}
>
> Is this sanity check function gonna be used from multiple places?
It will be reused when implementing __queue_delayed_kthread_work().
We will want to do these checks also before setting the timer.
See the 8th patch, e.g. at
http://thread.gmane.org/gmane.linux.kernel.mm/144964/focus=144973
> > /* insert @work before @pos in @worker */
> > static void insert_kthread_work(struct kthread_worker *worker,
> > - struct kthread_work *work,
> > - struct list_head *pos)
> > + struct kthread_work *work,
> > + struct list_head *pos)
> > {
> > - lockdep_assert_held(&worker->lock);
> > + insert_kthread_work_sanity_check(worker, work);
> >
> > list_add_tail(&work->node, pos);
> > work->worker = worker;
> > @@ -717,6 +730,15 @@ static void insert_kthread_work(struct kthread_worker
> > *worker,
> > * Queue @work to work processor @task for async execution. @task
> > * must have been created with kthread_worker_create(). Returns %true
> > * if @work was successfully queued, %false if it was already pending.
> > + *
> > + * Never queue a work into a worker when it is being processed by another
> > + * one. Otherwise, some operations, e.g. cancel or flush, will not work
> > + * correctly or the work might run in parallel. This is not enforced
> > + * because it would make the code too complex. There are only warnings
> > + * printed when such a situation is detected.
>
> I'm not sure the above paragraph adds much. It isn't that accurate to
> begin with as what's being disallowed is larger scope than the above.
> Isn't the paragraph below enough?
Makes sense. I'll remove it.
> > + * Reinitialize the work if it needs to be used by another worker.
> > + * For example, when the worker was stopped and started again.
> > */
> > bool queue_kthread_work(struct kthread_worker *worker,
> > struct kthread_work *work)
Thanks,
Petr
