Message-ID: <20160713102625.GA30921@twins.programming.kicks-ass.net>
Date:	Wed, 13 Jul 2016 12:26:25 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Chris Wilson <chris@...is-wilson.co.uk>
Cc:	linux-kernel@...r.kernel.org,
	Sumit Semwal <sumit.semwal@...aro.org>,
	Shuah Khan <shuahkh@....samsung.com>,
	Tejun Heo <tj@...nel.org>,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	Kees Cook <keescook@...omium.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	Davidlohr Bueso <dave@...olabs.net>,
	Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
	"David S. Miller" <davem@...emloft.net>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH 2/9] async: Introduce kfence, a N:M completion mechanism

On Fri, Jun 24, 2016 at 10:08:46AM +0100, Chris Wilson wrote:
> +struct kfence {
> +	wait_queue_head_t wait;
> +	unsigned long flags;
> +	struct kref kref;
> +	atomic_t pending;
> +};

> +#define KFENCE_CHECKED_BIT	0
> +
> +static void kfence_free(struct kref *kref)
> +{
> +	struct kfence *fence = container_of(kref, typeof(*fence), kref);
> +
> +	WARN_ON(atomic_read(&fence->pending) > 0);
> +
> +	kfree(fence);
> +}
> +
> +/**
> + * kfence_put - release a reference to a kfence
> + * @fence: the kfence being disposed of
> + */
> +void kfence_put(struct kfence *fence)
> +{
> +	if (fence)
> +		kref_put(&fence->kref, kfence_free);

It seems very poor semantics to allow to put NULL, that would indicate a
severe logic fail.

> +}
> +EXPORT_SYMBOL_GPL(kfence_put);

> +/**
> + * kfence_get - acquire a reference to a kfence
> + * @fence: the kfence being used
> + *
> + * Returns the pointer to the kfence, with its reference count incremented.
> + */
> +struct kfence *kfence_get(struct kfence *fence)
> +{
> +	if (fence)
> +		kref_get(&fence->kref);

Similar, getting NULL is just horrible taste.

> +	return fence;
> +}
> +EXPORT_SYMBOL_GPL(kfence_get);


> +static void __kfence_wake_up_all(struct kfence *fence,
> +				 struct list_head *continuation)
> +{
> +	wait_queue_head_t *x = &fence->wait;
> +	unsigned long flags;
> +
> +	/* To prevent unbounded recursion as we traverse the graph

Broken comment style.

> +	 * of kfences, we move the task_list from this ready fence
> +	 * to the tail of the current fence we are signaling.
> +	 */
> +	spin_lock_irqsave_nested(&x->lock, flags, 1 + !!continuation);
> +	if (continuation)
> +		list_splice_tail_init(&x->task_list, continuation);
> +	else while (!list_empty(&x->task_list))
> +		__wake_up_locked_key(x, TASK_NORMAL, &x->task_list);
> +	spin_unlock_irqrestore(&x->lock, flags);
> +}
> +
> +static void __kfence_signal(struct kfence *fence,
> +			    struct list_head *continuation)
> +{
> +	if (!atomic_dec_and_test(&fence->pending))
> +		return;
> +
> +	atomic_dec(&fence->pending);

You decrement twice?

> +	__kfence_wake_up_all(fence, continuation);
> +}
> +
> +/**
> + * kfence_pending - mark the fence as pending a signal

I would say: increment the pending count, requiring one more completion
before the fence is done.

'Mark' completely misses the point. You need to balance these increments
with decrements, its not a boolean state.

> + * @fence: the kfence to be signaled
> + *
> + */
> +void kfence_pending(struct kfence *fence)
> +{
> +	WARN_ON(atomic_inc_return(&fence->pending) <= 1);
> +}
> +EXPORT_SYMBOL_GPL(kfence_pending);


> +/**
> + * kfence_create - create a fence
> + * @gfp: the allowed allocation type
> + *
> + * A fence is created with a reference count of one, and pending a signal.
> + * After you have completed setting up the fence for use, call kfence_signal()
> + * to signal completion.
> + *
> + * Returns the newly allocated fence, or NULL on error.
> + */
> +struct kfence *kfence_create(gfp_t gfp)
> +{
> +	struct kfence *fence;
> +
> +	fence = kmalloc(sizeof(*fence), gfp);
> +	if (!fence)
> +		return NULL;
> +
> +	kfence_init(fence);
> +	return fence;
> +}
> +EXPORT_SYMBOL_GPL(kfence_create);

Why? What is the purpose of this here thing? We never provide allocation
wrappers.

> +
> +/**
> + * kfence_add - set one fence to wait upon another

Since you're going to do a whole lot other kfence_add_$foo() thingies,
why isn't this called kfence_add_kfence() ?

> + * @fence: this kfence
> + * @signaler: target kfence to wait upon
> + * @gfp: the allowed allocation type
> + *
> + * kfence_add() causes the @fence to wait upon completion of @signaler.
> + * Internally the @fence is marked as pending a signal from @signaler.
> + *
> + * Returns 1 if the @fence was added to the waiqueue of @signaler, 0
> + * if @signaler was already complete, or a negative error code.
> + */
> +int kfence_add(struct kfence *fence, struct kfence *signaler, gfp_t gfp)
> +{
> +	wait_queue_t *wq;
> +	unsigned long flags;
> +	int pending;
> +
> +	if (!signaler || kfence_complete(signaler))

Again, wth would you allow adding NULL? That's just horrible.

> +		return 0;
> +
> +	/* The dependency graph must be acyclic */
> +	if (unlikely(kfence_check_if_after(fence, signaler)))
> +		return -EINVAL;
> +
> +	wq = kmalloc(sizeof(*wq), gfp);
> +	if (unlikely(!wq)) {
> +		if (!gfpflags_allow_blocking(gfp))
> +			return -ENOMEM;
> +
> +		kfence_wait(signaler);
> +		return 0;
> +	}
> +
> +	wq->flags = 0;
> +	wq->func = kfence_wake;
> +	wq->private = kfence_get(fence);
> +
> +	kfence_pending(fence);
> +
> +	spin_lock_irqsave(&signaler->wait.lock, flags);
> +	if (likely(!kfence_complete(signaler))) {
> +		__add_wait_queue_tail(&signaler->wait, wq);
> +		pending = 1;
> +	} else {
> +		INIT_LIST_HEAD(&wq->task_list);
> +		kfence_wake(wq, 0, 0, NULL);
> +		pending = 0;
> +	}
> +	spin_unlock_irqrestore(&signaler->wait.lock, flags);
> +
> +	return pending;
> +}
> +EXPORT_SYMBOL_GPL(kfence_add);
> +
> +/**
> + * kfence_add_completion - set the fence to wait upon a completion
> + * @fence: this kfence
> + * @x: target completion to wait upon
> + * @gfp: the allowed allocation type
> + *
> + * kfence_add_completiond() causes the @fence to wait upon a completion.
> + * Internally the @fence is marked as pending a signal from @x.
> + *
> + * Returns 1 if the @fence was added to the waiqueue of @x, 0
> + * if @x was already complete, or a negative error code.
> + */
> +int kfence_add_completion(struct kfence *fence, struct completion *x, gfp_t gfp)
> +{
> +	wait_queue_t *wq;
> +	unsigned long flags;
> +	int pending;
> +
> +	if (!x || completion_done(x))
> +		return 0;
> +
> +	wq = kmalloc(sizeof(*wq), gfp);
> +	if (unlikely(!wq)) {
> +		if (!gfpflags_allow_blocking(gfp))
> +			return -ENOMEM;
> +
> +		wait_for_completion(x);
> +		return 0;
> +	}
> +
> +	wq->flags = 0;
> +	wq->func = kfence_wake;
> +	wq->private = kfence_get(fence);
> +
> +	kfence_pending(fence);
> +
> +	spin_lock_irqsave(&x->wait.lock, flags);
> +	if (likely(!READ_ONCE(x->done))) {
> +		__add_wait_queue_tail(&x->wait, wq);
> +		pending = 1;
> +	} else {
> +		INIT_LIST_HEAD(&wq->task_list);
> +		kfence_wake(wq, 0, 0, NULL);
> +		pending = 0;
> +	}
> +	spin_unlock_irqrestore(&x->wait.lock, flags);
> +
> +	return pending;
> +}
> +EXPORT_SYMBOL_GPL(kfence_add_completion);

It appears to me these two function share a _lot_ of code, surely that
can be reduced a bit?

Powered by blists - more mailing lists