Re: [PATCH v4 08/22] kthread: Initial support for delayed kthread work

Hello,

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

+/*
+ * Returns true when there is a pending operation for this work.
+ * In particular, it checks if the work is:
+ *	- queued
+ *	- a timer is running to queue this delayed work
+ *
+ * This function must be called with locked work.
+ */
+static inline bool kthread_work_pending(const struct kthread_work *work)
+{
+	return !list_empty(&work->node) ||
+	       (work->timer && timer_active(work->timer));
+}

Why not just put the work item on a separate list so that
lits_empty(&work->node) is always enough?  IOW, put delayed work items
on timers on worker->delayed or sth.

+/*
+ * Queue @work right into the worker queue.
+ */
+static void __queue_kthread_work(struct kthread_worker *worker,
+			  struct kthread_work *work)
+{
+	insert_kthread_work(worker, work, &worker->work_list);
+}

Does this really need to be an inline function?  This sort of one
liner helpers tend to be obfuscating more than anything else.

		return false;

+
+	spin_lock(&worker->lock);
+	if (worker != work->worker) {
+		spin_unlock(&worker->lock);
+		goto try_again;
+	}

	return true;

+	ret = true;
+
+out:
+	return ret;
+}

Stop building unnecessary structures.  Keep it simple.

+static inline void unlock_kthread_work(struct kthread_work *work)
+{
+	spin_unlock(&work->worker->lock);
+}

Ditto.  Just open code it.  It doesn't add anything.

+/**
+ * delayed_kthread_work_timer_fn - callback that queues the associated delayed
+ *	kthread work when the timer expires.
+ * @__data: pointer to the data associated with the timer
+ *
+ * The format of the function is defined by struct timer_list.
+ * It should have been called from irqsafe timer with irq already off.
+ */
+void delayed_kthread_work_timer_fn(unsigned long __data)
+{
+	struct delayed_kthread_work *dwork =
+		(struct delayed_kthread_work *)__data;
+	struct kthread_work *work = &dwork->work;
+
+	if (!try_lock_kthread_work(work))

Can you please explain why try_lock is necessary here?  That's the
most important and non-obvious thing going on here and there's no
explanation of that at all.

Thanks.

-- 
tejun