Re: [PATCH v4 09/22] kthread: Allow to cancel kthread work

Hello,

On Mon, Jan 25, 2016 at 04:44:58PM +0100, Petr Mladek wrote:

So, the reason ->canceling test is necessary is to ensure that
self-requeueing work items can be canceled reliably.  It's not to
block "further queueing" in general.  It's probably worthwhile to
clear that up in the description and comment.

+/*
+ * Get the worker lock if any worker is associated with the work.
+ * Depending on @check_canceling, it might need to give up the busy
+ * wait when work->canceling gets set.
+ */

While mentioning @check_canceling, the above doesn't actually explain
what it does.

Why READ_ONCE?

Again, this is the trickest part of the whole thing.  Please add a
comment explaining why this is necessary.

 		return;
 
-	__queue_kthread_work(work->worker, work);
+	if (!work->canceling)
+		__queue_kthread_work(work->worker, work);
+

...

+static int
+try_to_cancel_kthread_work(struct kthread_work *work,
+				   spinlock_t *lock,
+				   unsigned long *flags)

bool?

+{
+	int ret = 0;
+
+	/* Try to cancel the timer if pending. */
+	if (work->timer && del_timer_sync(work->timer)) {
+		ret = 1;
+		goto out;
+	}
+
+	/* Try to remove queued work before it is being executed. */
+	if (!list_empty(&work->node)) {
+		list_del_init(&work->node);
+		ret = 1;
+	}
+
+out:
+	return ret;

Again, what's up with unnecessary goto exits?

+static bool __cancel_kthread_work_sync(struct kthread_work *work)
+{
+	struct kthread_worker *worker;
+	unsigned long flags;
+	int ret;
+
+	local_irq_save(flags);
+	if (!try_lock_kthread_work(work, false)) {
+		local_irq_restore(flags);

Can't try_lock_kthread_work() take &flags?

+		ret = 0;
+		goto out;
+	}
+	worker = work->worker;
+
+	/*
+	 * Block further queueing. It must be set before trying to cancel
+	 * the kthread work. It avoids a possible deadlock between
+	 * del_timer_sync() and the timer callback.
+	 */

So, "blocking further queueing" and "a possible deadlock between
del_timer_sync() and the timer callback" don't have anything to do
with each other, do they?  Those are two separate things.  You need
the former to guarantee cancelation of self-requeueing work items and
the latter for deadlock avoidance, no?

+	work->canceling++;
+	ret = try_to_cancel_kthread_work(work, &worker->lock, &flags);
+
+	if (worker->current_work != work)
+		goto out_fast;

If there are two racing cancellers, wouldn't this allow the losing one
to return while the work item is still running?

+	spin_unlock_irqrestore(&worker->lock, flags);
+	flush_kthread_work(work);
+	/*
+	 * Nobody is allowed to switch the worker or queue the work
+	 * when .canceling is set.
+	 */
+	spin_lock_irqsave(&worker->lock, flags);
+
+out_fast:
+	work->canceling--;
+	spin_unlock_irqrestore(&worker->lock, flags);
+out:
+	return ret;
+}

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>