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?