Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

5 participants
2541 discussions

[PATCH 1/2] locking: Implement an algorithm choice for Wound-Wait mutexes

by Thomas Hellstrom

The current Wound-Wait mutex algorithm is actually not Wound-Wait but Wait-Die. Implement also Wound-Wait as a per-ww-class choice. Wound-Wait is, contrary to Wait-Die a preemptive algorithm and is known to generate fewer backoffs. Testing reveals that this is true if the number of simultaneous contending transactions is small. As the number of simultaneous contending threads increases, Wait-Wound becomes inferior to Wait-Die in terms of elapsed time. Possibly due to the larger number of held locks of sleeping transactions. Update documentation and callers. Timings using git://people.freedesktop.org/~thomash/ww_mutex_test tag patch-18-06-04 Each thread runs 100000 batches of lock / unlock 800 ww mutexes randomly chosen out of 100000. Four core Intel x86_64: Algorithm #threads Rollbacks time Wound-Wait 4 ~100 ~17s. Wait-Die 4 ~150000 ~19s. Wound-Wait 16 ~360000 ~109s. Wait-Die 16 ~450000 ~82s. Cc: Peter Zijlstra <peterz(a)infradead.org> Cc: Ingo Molnar <mingo(a)redhat.com> Cc: Jonathan Corbet <corbet(a)lwn.net> Cc: Gustavo Padovan <gustavo(a)padovan.org> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Sean Paul <seanpaul(a)chromium.org> Cc: David Airlie <airlied(a)linux.ie> Cc: Davidlohr Bueso <dave(a)stgolabs.net> Cc: "Paul E. McKenney" <paulmck(a)linux.vnet.ibm.com> Cc: Josh Triplett <josh(a)joshtriplett.org> Cc: Thomas Gleixner <tglx(a)linutronix.de> Cc: Kate Stewart <kstewart(a)linuxfoundation.org> Cc: Philippe Ombredanne <pombredanne(a)nexb.com> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> Cc: linux-doc(a)vger.kernel.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Signed-off-by: Thomas Hellstrom <thellstrom(a)vmware.com> --- Documentation/locking/ww-mutex-design.txt | 57 ++++++++++++++---- drivers/dma-buf/reservation.c | 2 +- drivers/gpu/drm/drm_modeset_lock.c | 2 +- include/linux/ww_mutex.h | 19 ++++-- kernel/locking/locktorture.c | 2 +- kernel/locking/mutex.c | 98 ++++++++++++++++++++++++++++--- kernel/locking/test-ww_mutex.c | 2 +- lib/locking-selftest.c | 2 +- 8 files changed, 152 insertions(+), 32 deletions(-) diff --git a/Documentation/locking/ww-mutex-design.txt b/Documentation/locking/ww-mutex-design.txt index 34c3a1b50b9a..29c85623b551 100644 --- a/Documentation/locking/ww-mutex-design.txt +++ b/Documentation/locking/ww-mutex-design.txt @@ -1,4 +1,4 @@ -Wait/Wound Deadlock-Proof Mutex Design +Wound/Wait Deadlock-Proof Mutex Design ====================================== Please read mutex-design.txt first, as it applies to wait/wound mutexes too. @@ -32,10 +32,23 @@ the oldest task) wins, and the one with the higher reservation id (i.e. the younger task) unlocks all of the buffers that it has already locked, and then tries again. -In the RDBMS literature this deadlock handling approach is called wait/wound: -The older tasks waits until it can acquire the contended lock. The younger tasks -needs to back off and drop all the locks it is currently holding, i.e. the -younger task is wounded. +In the RDBMS literature, a reservation ticket is associated with a transaction. +and the deadlock handling approach is called Wait-Die. The name is based on +the actions of a locking thread when it encounters an already locked mutex. +If the transaction holding the lock is younger, the locking transaction waits. +If the transaction holding the lock is older, the locking transaction backs off +and dies. Hence Wait-Die. +There is also another algorithm called Wound-Wait: +If the transaction holding the lock is younger, the locking transaction +preempts the transaction holding the lock, requiring it to back off. It +Wounds the other transaction. +If the transaction holding the lock is older, it waits for the other +transaction. Hence Wound-Wait. +The two algorithms are both fair in that a transaction will eventually succeed. +However, the Wound-Wait algorithm is typically stated to generate fewer backoffs +compared to Wait-Die, but is, on the other hand, associated with more work than +Wait-Die when recovering from a backoff. Wound-Wait is also a preemptive +algorithm which requires a reliable way to preempt another transaction. Concepts -------- @@ -47,10 +60,12 @@ Acquire context: To ensure eventual forward progress it is important the a task trying to acquire locks doesn't grab a new reservation id, but keeps the one it acquired when starting the lock acquisition. This ticket is stored in the acquire context. Furthermore the acquire context keeps track of debugging state -to catch w/w mutex interface abuse. +to catch w/w mutex interface abuse. An acquire context is representing a +transaction. W/w class: In contrast to normal mutexes the lock class needs to be explicit for -w/w mutexes, since it is required to initialize the acquire context. +w/w mutexes, since it is required to initialize the acquire context. The lock +class also specifies what algorithm to use, Wound-Wait or Wait-Die. Furthermore there are three different class of w/w lock acquire functions: @@ -90,10 +105,15 @@ provided. Usage ----- +The algorithm (Wait-Die vs Wound-Wait) is chosen using the _is_wait_die +argument to DEFINE_WW_CLASS(). As a rough rule of thumb, use Wound-Wait iff you +typically expect the number of simultaneous competing transactions to be small, +and the rollback cost can be substantial. + Three different ways to acquire locks within the same w/w class. Common definitions for methods #1 and #2: -static DEFINE_WW_CLASS(ww_class); +static DEFINE_WW_CLASS(ww_class, false); struct obj { struct ww_mutex lock; @@ -243,7 +263,7 @@ struct obj { struct list_head locked_list; }; -static DEFINE_WW_CLASS(ww_class); +static DEFINE_WW_CLASS(ww_class, false); void __unlock_objs(struct list_head *list) { @@ -312,12 +332,23 @@ Design: We maintain the following invariants for the wait list: (1) Waiters with an acquire context are sorted by stamp order; waiters without an acquire context are interspersed in FIFO order. - (2) Among waiters with contexts, only the first one can have other locks - acquired already (ctx->acquired > 0). Note that this waiter may come - after other waiters without contexts in the list. + (2) For Wait-Die, among waiters with contexts, only the first one can have + other locks acquired already (ctx->acquired > 0). Note that this waiter + may come after other waiters without contexts in the list. + + The Wound-Wait preemption is implemented with a lazy-preemption scheme: + The wounded status of the transaction is checked only when there is + contention for a new lock and hence a true chance of deadlock. In that + situation, if the transaction is wounded, it backs off, clears the + wounded status and retries. A great benefit of implementing preemption in + this way is that the wounded transaction can identify a contending lock to + wait for before restarting the transaction. Just blindly restarting the + transaction would likely make the transaction end up in a situation where + it would have to back off again. In general, not much contention is expected. The locks are typically used to - serialize access to resources for devices. + serialize access to resources for devices, and optimization focus should + therefore be directed towards the uncontended cases. Lockdep: Special care has been taken to warn for as many cases of api abuse diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index 314eb1071cce..039571b9fea1 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -46,7 +46,7 @@ * write-side updates. */ -DEFINE_WW_CLASS(reservation_ww_class); +DEFINE_WW_CLASS(reservation_ww_class, true); EXPORT_SYMBOL(reservation_ww_class); struct lock_class_key reservation_seqcount_class; diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c index 8a5100685875..f22a7ef41de1 100644 --- a/drivers/gpu/drm/drm_modeset_lock.c +++ b/drivers/gpu/drm/drm_modeset_lock.c @@ -70,7 +70,7 @@ * lists and lookup data structures. */ -static DEFINE_WW_CLASS(crtc_ww_class); +static DEFINE_WW_CLASS(crtc_ww_class, true); /** * drm_modeset_lock_all - take all modeset locks diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h index 39fda195bf78..6278077f288b 100644 --- a/include/linux/ww_mutex.h +++ b/include/linux/ww_mutex.h @@ -8,6 +8,8 @@ * * Wound/wait implementation: * Copyright (C) 2013 Canonical Ltd. + * Choice of algorithm: + * Copyright (C) 2018 WMWare Inc. * * This file contains the main data structure and API definitions. */ @@ -23,15 +25,17 @@ struct ww_class { struct lock_class_key mutex_key; const char *acquire_name; const char *mutex_name; + bool is_wait_die; }; struct ww_acquire_ctx { struct task_struct *task; unsigned long stamp; unsigned acquired; + bool wounded; + struct ww_class *ww_class; #ifdef CONFIG_DEBUG_MUTEXES unsigned done_acquire; - struct ww_class *ww_class; struct ww_mutex *contending_lock; #endif #ifdef CONFIG_DEBUG_LOCK_ALLOC @@ -58,17 +62,19 @@ struct ww_mutex { # define __WW_CLASS_MUTEX_INITIALIZER(lockname, class) #endif -#define __WW_CLASS_INITIALIZER(ww_class) \ +#define __WW_CLASS_INITIALIZER(ww_class, _is_wait_die) \ { .stamp = ATOMIC_LONG_INIT(0) \ , .acquire_name = #ww_class "_acquire" \ - , .mutex_name = #ww_class "_mutex" } + , .mutex_name = #ww_class "_mutex" \ + , .is_wait_die = _is_wait_die } #define __WW_MUTEX_INITIALIZER(lockname, class) \ { .base = __MUTEX_INITIALIZER(lockname.base) \ __WW_CLASS_MUTEX_INITIALIZER(lockname, class) } -#define DEFINE_WW_CLASS(classname) \ - struct ww_class classname = __WW_CLASS_INITIALIZER(classname) +#define DEFINE_WW_CLASS(classname, _is_wait_die) \ + struct ww_class classname = __WW_CLASS_INITIALIZER(classname, \ + _is_wait_die) #define DEFINE_WW_MUTEX(mutexname, ww_class) \ struct ww_mutex mutexname = __WW_MUTEX_INITIALIZER(mutexname, ww_class) @@ -123,8 +129,9 @@ static inline void ww_acquire_init(struct ww_acquire_ctx *ctx, ctx->task = current; ctx->stamp = atomic_long_inc_return_relaxed(&ww_class->stamp); ctx->acquired = 0; -#ifdef CONFIG_DEBUG_MUTEXES ctx->ww_class = ww_class; + ctx->wounded = false; +#ifdef CONFIG_DEBUG_MUTEXES ctx->done_acquire = 0; ctx->contending_lock = NULL; #endif diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c index 6850ffd69125..778ed026382f 100644 --- a/kernel/locking/locktorture.c +++ b/kernel/locking/locktorture.c @@ -365,7 +365,7 @@ static struct lock_torture_ops mutex_lock_ops = { }; #include <linux/ww_mutex.h> -static DEFINE_WW_CLASS(torture_ww_class); +static DEFINE_WW_CLASS(torture_ww_class, true); static DEFINE_WW_MUTEX(torture_ww_mutex_0, &torture_ww_class); static DEFINE_WW_MUTEX(torture_ww_mutex_1, &torture_ww_class); static DEFINE_WW_MUTEX(torture_ww_mutex_2, &torture_ww_class); diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c index 2048359f33d2..b449a012c6f9 100644 --- a/kernel/locking/mutex.c +++ b/kernel/locking/mutex.c @@ -290,12 +290,47 @@ __ww_ctx_stamp_after(struct ww_acquire_ctx *a, struct ww_acquire_ctx *b) (a->stamp != b->stamp || a > b); } +/* + * Wound the lock holder transaction if it's younger than the contending + * transaction, and there is a possibility of a deadlock. + * Also if the lock holder transaction isn't the current transaction, + * Make sure it's woken up in case it's sleeping on another ww mutex. + */ +static bool __ww_mutex_wound(struct mutex *lock, + struct ww_acquire_ctx *ww_ctx, + struct ww_acquire_ctx *hold_ctx) +{ + struct task_struct *owner = + __owner_task(atomic_long_read(&lock->owner)); + + lockdep_assert_held(&lock->wait_lock); + + if (owner && hold_ctx && __ww_ctx_stamp_after(hold_ctx, ww_ctx) && + ww_ctx->acquired > 0) { + WRITE_ONCE(hold_ctx->wounded, true); + if (owner != current) { + /* + * wake_up_process() inserts a write memory barrier to + * make sure owner sees it is wounded before + * TASK_RUNNING in case it's sleeping on another + * ww_mutex. Note that owner points to a valid + * task_struct as long as we hold the wait_lock. + */ + wake_up_process(owner); + } + return true; + } + + return false; +} + /* * Wake up any waiters that may have to back off when the lock is held by the * given context. * * Due to the invariants on the wait list, this can only affect the first - * waiter with a context. + * waiter with a context, unless the Wound-Wait algorithm is used where + * also subsequent waiters with a context main wound the lock holder. * * The current task must not be on the wait list. */ @@ -303,6 +338,7 @@ static void __sched __ww_mutex_wakeup_for_backoff(struct mutex *lock, struct ww_acquire_ctx *ww_ctx) { struct mutex_waiter *cur; + bool is_wait_die = ww_ctx->ww_class->is_wait_die; lockdep_assert_held(&lock->wait_lock); @@ -310,13 +346,14 @@ __ww_mutex_wakeup_for_backoff(struct mutex *lock, struct ww_acquire_ctx *ww_ctx) if (!cur->ww_ctx) continue; - if (cur->ww_ctx->acquired > 0 && + if (is_wait_die && cur->ww_ctx->acquired > 0 && __ww_ctx_stamp_after(cur->ww_ctx, ww_ctx)) { debug_mutex_wake_waiter(lock, cur); wake_up_process(cur->task); } - break; + if (is_wait_die || __ww_mutex_wound(lock, cur->ww_ctx, ww_ctx)) + break; } } @@ -338,12 +375,17 @@ ww_mutex_set_context_fastpath(struct ww_mutex *lock, struct ww_acquire_ctx *ctx) * and keep spinning, or it will acquire wait_lock, add itself * to waiter list and sleep. */ - smp_mb(); /* ^^^ */ + smp_mb(); /* See comments above and below. */ /* - * Check if lock is contended, if not there is nobody to wake up + * Check if lock is contended, if not there is nobody to wake up. + * Checking MUTEX_FLAG_WAITERS is not enough here, since we need to + * order against the lock->ctx check in __ww_mutex_wound called from + * __ww_mutex_add_waiter. We can use list_empty without taking the + * wait_lock, given the memory barrier above and the list_empty + * documentation. */ - if (likely(!(atomic_long_read(&lock->base.owner) & MUTEX_FLAG_WAITERS))) + if (likely(list_empty(&lock->base.wait_list))) return; /* @@ -653,6 +695,17 @@ __ww_mutex_lock_check_stamp(struct mutex *lock, struct mutex_waiter *waiter, struct ww_acquire_ctx *hold_ctx = READ_ONCE(ww->ctx); struct mutex_waiter *cur; + /* + * If we miss a wounded == true here, we will have a pending + * TASK_RUNNING and pick it up on the next schedule fall-through. + */ + if (!ctx->ww_class->is_wait_die) { + if (READ_ONCE(ctx->wounded)) + goto deadlock; + else + return 0; + } + if (hold_ctx && __ww_ctx_stamp_after(ctx, hold_ctx)) goto deadlock; @@ -683,12 +736,15 @@ __ww_mutex_add_waiter(struct mutex_waiter *waiter, { struct mutex_waiter *cur; struct list_head *pos; + bool is_wait_die; if (!ww_ctx) { list_add_tail(&waiter->list, &lock->wait_list); return 0; } + is_wait_die = ww_ctx->ww_class->is_wait_die; + /* * Add the waiter before the first waiter with a higher stamp. * Waiters without a context are skipped to avoid starving @@ -701,7 +757,7 @@ __ww_mutex_add_waiter(struct mutex_waiter *waiter, if (__ww_ctx_stamp_after(ww_ctx, cur->ww_ctx)) { /* Back off immediately if necessary. */ - if (ww_ctx->acquired > 0) { + if (is_wait_die && ww_ctx->acquired > 0) { #ifdef CONFIG_DEBUG_MUTEXES struct ww_mutex *ww; @@ -721,13 +777,26 @@ __ww_mutex_add_waiter(struct mutex_waiter *waiter, * Wake up the waiter so that it gets a chance to back * off. */ - if (cur->ww_ctx->acquired > 0) { + if (is_wait_die && cur->ww_ctx->acquired > 0) { debug_mutex_wake_waiter(lock, cur); wake_up_process(cur->task); } } list_add_tail(&waiter->list, pos); + if (!is_wait_die) { + struct ww_mutex *ww = container_of(lock, struct ww_mutex, base); + + /* + * Make sure a racing lock taker sees a non-empty waiting list + * before we read ww->ctx, so that if we miss ww->ctx, the + * racing lock taker will call __ww_mutex_wake_up_for_backoff() + * and wound itself. + */ + smp_mb(); + __ww_mutex_wound(lock, ww_ctx, ww->ctx); + } + return 0; } @@ -750,6 +819,14 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, if (use_ww_ctx && ww_ctx) { if (unlikely(ww_ctx == READ_ONCE(ww->ctx))) return -EALREADY; + + /* + * Reset the wounded flag after a backoff. + * No other process can race and wound us here since they + * can't have a valid owner pointer at this time + */ + if (ww_ctx->acquired == 0) + ww_ctx->wounded = false; } preempt_disable(); @@ -858,6 +935,11 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, acquired: __set_current_state(TASK_RUNNING); + /* We stole the lock. Need to check wounded status. */ + if (use_ww_ctx && ww_ctx && !ww_ctx->ww_class->is_wait_die && + !__mutex_waiter_is_first(lock, &waiter)) + __ww_mutex_wakeup_for_backoff(lock, ww_ctx); + mutex_remove_waiter(lock, &waiter, current); if (likely(list_empty(&lock->wait_list))) __mutex_clear_flag(lock, MUTEX_FLAGS); diff --git a/kernel/locking/test-ww_mutex.c b/kernel/locking/test-ww_mutex.c index 0e4cd64ad2c0..c7fc112d691d 100644 --- a/kernel/locking/test-ww_mutex.c +++ b/kernel/locking/test-ww_mutex.c @@ -26,7 +26,7 @@ #include <linux/slab.h> #include <linux/ww_mutex.h> -static DEFINE_WW_CLASS(ww_class); +static DEFINE_WW_CLASS(ww_class, true); struct workqueue_struct *wq; struct test_mutex { diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c index b5c1293ce147..e52065f2acbf 100644 --- a/lib/locking-selftest.c +++ b/lib/locking-selftest.c @@ -29,7 +29,7 @@ */ static unsigned int debug_locks_verbose; -static DEFINE_WW_CLASS(ww_lockdep); +static DEFINE_WW_CLASS(ww_lockdep, true); static int __init setup_debug_locks_verbose(char *str) { -- 2.14.3

6 years, 7 months

Re: [Linaro-mm-sig] [PATCH v2 1/2] locking: Implement an algorithm choice for Wound-Wait mutexes

by Thomas Hellstrom

On 06/14/2018 03:29 PM, Matthew Wilcox wrote: > On Thu, Jun 14, 2018 at 01:54:15PM +0200, Thomas Hellstrom wrote: >> On 06/14/2018 01:36 PM, Peter Zijlstra wrote: >>> Currently you don't allow mixing WD and WW contexts (which is not >>> immediately obvious from the above code), and the above hard relies on >>> that. Are there sensible use cases for mixing them? IOW will your >>> current restriction stand without hassle? >> Contexts _must_ agree on the algorithm used to resolve deadlocks. With >> Wait-Die, for example, older transactions will wait if a lock is held by a >> younger transaction and with Wound-Wait, younger transactions will wait if a >> lock is held by an older transaction so there is no way of mixing them. > Maybe the compiler should be enforcing that; ie make it a different type? It's intended to be enforced by storing the algorithm choice in the WW_MUTEX_CLASS which must be common for an acquire context and the ww_mutexes it acquires. However, I don't think there is a check that that holds. I guess we could add it as a DEBUG_MUTEX test in ww_mutex_lock(). Thanks, Thomas

6 years, 7 months

[PATCH v2 0/2] locking, drm: Fix ww mutex naming / algorithm inconsistency

by Thomas Hellstrom

This is a small fallout from a work to allow batching WW mutex locks and unlocks. Our Wound-Wait mutexes actually don't use the Wound-Wait algorithm but the Wait-Die algorithm. One could perhaps rename those mutexes tree-wide to "Wait-Die mutexes" or "Deadlock Avoidance mutexes". Another approach suggested here is to implement also the "Wound-Wait" algorithm as a per-WW-class choice, as it has advantages in some cases. See for example http://www.mathcs.emory.edu/~cheung/Courses/554/Syllabus/8-recv+serial/dead… Now Wound-Wait is a preemptive algorithm, and the preemption is implemented using a lazy scheme: If a wounded transaction is about to go to sleep on a contended WW mutex, we return -EDEADLK. That is sufficient for deadlock prevention. Since with WW mutexes we also require the aborted transaction to sleep waiting to lock the WW mutex it was aborted on, this choice also provides a suitable WW mutex to sleep on. If we were to return -EDEADLK on the first WW mutex lock after the transaction was wounded whether the WW mutex was contended or not, the transaction might frequently be restarted without a wait, which is far from optimal. Note also that with the lazy preemption scheme, contrary to Wait-Die there will be no rollbacks on lock contention of locks held by a transaction that has completed its locking sequence. The modeset locks are then changed from Wait-Die to Wound-Wait since the typical locking pattern of those locks very well matches the criterion for a substantial reduction in the number of rollbacks. For reservation objects, the benefit is more unclear at this point and they remain using Wait-Die. Cc: Peter Zijlstra <peterz(a)infradead.org> Cc: Ingo Molnar <mingo(a)redhat.com> Cc: Jonathan Corbet <corbet(a)lwn.net> Cc: Gustavo Padovan <gustavo(a)padovan.org> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Sean Paul <seanpaul(a)chromium.org> Cc: David Airlie <airlied(a)linux.ie> Cc: Davidlohr Bueso <dave(a)stgolabs.net> Cc: "Paul E. McKenney" <paulmck(a)linux.vnet.ibm.com> Cc: Josh Triplett <josh(a)joshtriplett.org> Cc: Thomas Gleixner <tglx(a)linutronix.de> Cc: Kate Stewart <kstewart(a)linuxfoundation.org> Cc: Philippe Ombredanne <pombredanne(a)nexb.com> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> Cc: linux-doc(a)vger.kernel.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org v2: Updated DEFINE_WW_CLASS() API, minor code- and comment fixes as detailed in each patch.

6 years, 7 months

Re: [Linaro-mm-sig] [PATCH v2 1/2] locking: Implement an algorithm choice for Wound-Wait mutexes

by Thomas Hellstrom

Resending hopefully better formatted.. On 06/14/2018 01:49 PM, Andrea Parri wrote: > [...] > >>>> + /* >>>> + * wake_up_process() paired with set_current_state() inserts >>>> + * sufficient barriers to make sure @owner either sees it's >>>> + * wounded or has a wakeup pending to re-read the wounded >>>> + * state. >>> IIUC, "sufficient barriers" = full memory barriers (here). (You may >>> want to be more specific.) >> Thanks for reviewing! >> OK. What about if someone relaxes that in the future? > This is actually one of my main concerns ;-) as, IIUC, those barriers are > not only sufficient but also necessary: anything "less than a full barrier" > (in either wake_up_process() or set_current_state()) would _not_ guarantee > the "condition" above unless I'm misunderstanding it. > > But am I misunderstanding it? Which barriers/guarantee do you _need_ from > the above mentioned pairing? (hence my comment...) > > Andrea No you are probably not misunderstanding me at all. My comment originated from the reading of the kerneldoc of set_current_state() /* * set_current_state() includes a barrier so that the write of current->state * is correctly serialised wrt the caller's subsequent test of whether to * actually sleep: * * for (;;) { * set_current_state(TASK_UNINTERRUPTIBLE); * if (!need_sleep) * break; * * schedule(); * } * __set_current_state(TASK_RUNNING); * * If the caller does not need such serialisation (because, for instance, the * condition test and condition change and wakeup are under the same lock) then * use __set_current_state(). * * The above is typically ordered against the wakeup, which does: * * need_sleep = false; * wake_up_state(p, TASK_UNINTERRUPTIBLE); * * Where wake_up_state() (and all other wakeup primitives) imply enough * barriers to order the store of the variable against wakeup. --- */ And with ctx->wounded := !need_sleep this exactly matches what's happening in my code. So what I was trying to say in the comment was that this above contract is sufficient to guarantee the "condition" above, whitout me actually knowing exactly what barriers are required. Thanks, Thomas

6 years, 7 months

Re: [Linaro-mm-sig] [PATCH v2 1/2] locking: Implement an algorithm choice for Wound-Wait mutexes

by Thomas Hellstrom

On 06/14/2018 01:49 PM, Andrea Parri wrote: > [...] > >>>> + /* >>>> + * wake_up_process() paired with set_current_state() inserts >>>> + * sufficient barriers to make sure @owner either sees it's >>>> + * wounded or has a wakeup pending to re-read the wounded >>>> + * state. >>> IIUC, "sufficient barriers" = full memory barriers (here). (You may >>> want to be more specific.) >> Thanks for reviewing! >> OK. What about if someone relaxes that in the future? > This is actually one of my main concerns ;-) as, IIUC, those barriers are > not only sufficient but also necessary: anything "less than a full barrier" > (in either wake_up_process() or set_current_state()) would _not_ guarantee > the "condition" above unless I'm misunderstanding it. > > But am I misunderstanding it? Which barriers/guarantee do you _need_ from > the above mentioned pairing? (hence my comment...) > > Andrea No you are probably not misunderstanding me at all. My comment originated from the reading of the kerneldoc of set_current_state() * set_current_state() includes a barrier so that the write of current->state * is correctly serialised wrt the caller's subsequent test of whether to * actually sleep: * * for (;;) { * set_current_state(TASK_UNINTERRUPTIBLE); * if (!need_sleep) * break; * * schedule(); * } * __set_current_state(TASK_RUNNING); * * If the caller does not need such serialisation (because, for instance, the * condition test and condition change and wakeup are under the same lock) then * use __set_current_state(). * * The above is typically ordered against the wakeup, which does: * * need_sleep = false; * wake_up_state(p, TASK_UNINTERRUPTIBLE); * * Where wake_up_state() (and all other wakeup primitives) imply enough * barriers to order the store of the variable against wakeup. And with ctx->wounded := !need_sleep this exactly matches what's happening in my code. So what I was trying to say in the comment was that this above contract is sufficient to guarantee the "condition" above, whitout me actually knowing exactly what barriers are required. Thanks, Thomas

6 years, 7 months

Re: [Linaro-mm-sig] [PATCH v2 1/2] locking: Implement an algorithm choice for Wound-Wait mutexes

by Thomas Hellstrom

On 06/14/2018 12:38 PM, Andrea Parri wrote: > Hi Thomas, > > On Thu, Jun 14, 2018 at 09:29:21AM +0200, Thomas Hellstrom wrote: >> The current Wound-Wait mutex algorithm is actually not Wound-Wait but >> Wait-Die. Implement also Wound-Wait as a per-ww-class choice. Wound-Wait >> is, contrary to Wait-Die a preemptive algorithm and is known to generate >> fewer backoffs. Testing reveals that this is true if the >> number of simultaneous contending transactions is small. >> As the number of simultaneous contending threads increases, Wait-Wound >> becomes inferior to Wait-Die in terms of elapsed time. >> Possibly due to the larger number of held locks of sleeping transactions. >> >> Update documentation and callers. >> >> Timings using git://people.freedesktop.org/~thomash/ww_mutex_test >> tag patch-18-06-14 >> >> Each thread runs 100000 batches of lock / unlock 800 ww mutexes randomly >> chosen out of 100000. Four core Intel x86_64: >> >> Algorithm #threads Rollbacks time >> Wound-Wait 4 ~100 ~17s. >> Wait-Die 4 ~150000 ~19s. >> Wound-Wait 16 ~360000 ~109s. >> Wait-Die 16 ~450000 ~82s. >> >> Cc: Peter Zijlstra <peterz(a)infradead.org> >> Cc: Ingo Molnar <mingo(a)redhat.com> >> Cc: Jonathan Corbet <corbet(a)lwn.net> >> Cc: Gustavo Padovan <gustavo(a)padovan.org> >> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> >> Cc: Sean Paul <seanpaul(a)chromium.org> >> Cc: David Airlie <airlied(a)linux.ie> >> Cc: Davidlohr Bueso <dave(a)stgolabs.net> >> Cc: "Paul E. McKenney" <paulmck(a)linux.vnet.ibm.com> >> Cc: Josh Triplett <josh(a)joshtriplett.org> >> Cc: Thomas Gleixner <tglx(a)linutronix.de> >> Cc: Kate Stewart <kstewart(a)linuxfoundation.org> >> Cc: Philippe Ombredanne <pombredanne(a)nexb.com> >> Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> >> Cc: linux-doc(a)vger.kernel.org >> Cc: linux-media(a)vger.kernel.org >> Cc: linaro-mm-sig(a)lists.linaro.org >> Signed-off-by: Thomas Hellstrom <thellstrom(a)vmware.com> >> >> --- >> v2: >> * Update API according to review comment by Greg Kroah-Hartman. >> * Address review comments by Peter Zijlstra: >> - Avoid _Bool in composites >> - Fix typo >> - Use __mutex_owner() where applicable >> - Rely on built-in barriers for the main loop exit condition, >> struct ww_acquire_ctx::wounded. Update code comments. >> - Explain unlocked use of list_empty(). >> --- >> Documentation/locking/ww-mutex-design.txt | 54 ++++++++++++---- >> drivers/dma-buf/reservation.c | 2 +- >> drivers/gpu/drm/drm_modeset_lock.c | 2 +- >> include/linux/ww_mutex.h | 19 ++++-- >> kernel/locking/locktorture.c | 2 +- >> kernel/locking/mutex.c | 103 +++++++++++++++++++++++++++--- >> kernel/locking/test-ww_mutex.c | 2 +- >> lib/locking-selftest.c | 2 +- >> 8 files changed, 156 insertions(+), 30 deletions(-) >> >> diff --git a/Documentation/locking/ww-mutex-design.txt b/Documentation/locking/ww-mutex-design.txt >> index 34c3a1b50b9a..b9597def9581 100644 >> --- a/Documentation/locking/ww-mutex-design.txt >> +++ b/Documentation/locking/ww-mutex-design.txt >> @@ -1,4 +1,4 @@ >> -Wait/Wound Deadlock-Proof Mutex Design >> +Wound/Wait Deadlock-Proof Mutex Design >> ====================================== >> >> Please read mutex-design.txt first, as it applies to wait/wound mutexes too. >> @@ -32,10 +32,23 @@ the oldest task) wins, and the one with the higher reservation id (i.e. the >> younger task) unlocks all of the buffers that it has already locked, and then >> tries again. >> >> -In the RDBMS literature this deadlock handling approach is called wait/wound: >> -The older tasks waits until it can acquire the contended lock. The younger tasks >> -needs to back off and drop all the locks it is currently holding, i.e. the >> -younger task is wounded. >> +In the RDBMS literature, a reservation ticket is associated with a transaction. >> +and the deadlock handling approach is called Wait-Die. The name is based on >> +the actions of a locking thread when it encounters an already locked mutex. >> +If the transaction holding the lock is younger, the locking transaction waits. >> +If the transaction holding the lock is older, the locking transaction backs off >> +and dies. Hence Wait-Die. >> +There is also another algorithm called Wound-Wait: >> +If the transaction holding the lock is younger, the locking transaction >> +preempts the transaction holding the lock, requiring it to back off. It >> +Wounds the other transaction. >> +If the transaction holding the lock is older, it waits for the other >> +transaction. Hence Wound-Wait. >> +The two algorithms are both fair in that a transaction will eventually succeed. >> +However, the Wound-Wait algorithm is typically stated to generate fewer backoffs >> +compared to Wait-Die, but is, on the other hand, associated with more work than >> +Wait-Die when recovering from a backoff. Wound-Wait is also a preemptive >> +algorithm which requires a reliable way to preempt another transaction. >> >> Concepts >> -------- >> @@ -47,10 +60,12 @@ Acquire context: To ensure eventual forward progress it is important the a task >> trying to acquire locks doesn't grab a new reservation id, but keeps the one it >> acquired when starting the lock acquisition. This ticket is stored in the >> acquire context. Furthermore the acquire context keeps track of debugging state >> -to catch w/w mutex interface abuse. >> +to catch w/w mutex interface abuse. An acquire context is representing a >> +transaction. >> >> W/w class: In contrast to normal mutexes the lock class needs to be explicit for >> -w/w mutexes, since it is required to initialize the acquire context. >> +w/w mutexes, since it is required to initialize the acquire context. The lock >> +class also specifies what algorithm to use, Wound-Wait or Wait-Die. >> >> Furthermore there are three different class of w/w lock acquire functions: >> >> @@ -90,6 +105,12 @@ provided. >> Usage >> ----- >> >> +The algorithm (Wait-Die vs Wound-Wait) is chosen by using either >> +DEFINE_WW_CLASS_WDIE() for Wait-Die or DEFINE_WW_CLASS() for Wound-Wait. >> +As a rough rule of thumb, use Wound-Wait iff you typically expect the number >> +of simultaneous competing transactions to be small, and the rollback cost can >> +be substantial. >> + >> Three different ways to acquire locks within the same w/w class. Common >> definitions for methods #1 and #2: >> >> @@ -312,12 +333,23 @@ Design: >> We maintain the following invariants for the wait list: >> (1) Waiters with an acquire context are sorted by stamp order; waiters >> without an acquire context are interspersed in FIFO order. >> - (2) Among waiters with contexts, only the first one can have other locks >> - acquired already (ctx->acquired > 0). Note that this waiter may come >> - after other waiters without contexts in the list. >> + (2) For Wait-Die, among waiters with contexts, only the first one can have >> + other locks acquired already (ctx->acquired > 0). Note that this waiter >> + may come after other waiters without contexts in the list. >> + >> + The Wound-Wait preemption is implemented with a lazy-preemption scheme: >> + The wounded status of the transaction is checked only when there is >> + contention for a new lock and hence a true chance of deadlock. In that >> + situation, if the transaction is wounded, it backs off, clears the >> + wounded status and retries. A great benefit of implementing preemption in >> + this way is that the wounded transaction can identify a contending lock to >> + wait for before restarting the transaction. Just blindly restarting the >> + transaction would likely make the transaction end up in a situation where >> + it would have to back off again. >> >> In general, not much contention is expected. The locks are typically used to >> - serialize access to resources for devices. >> + serialize access to resources for devices, and optimization focus should >> + therefore be directed towards the uncontended cases. >> >> Lockdep: >> Special care has been taken to warn for as many cases of api abuse >> diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c >> index 314eb1071cce..b94a4bab2ecd 100644 >> --- a/drivers/dma-buf/reservation.c >> +++ b/drivers/dma-buf/reservation.c >> @@ -46,7 +46,7 @@ >> * write-side updates. >> */ >> >> -DEFINE_WW_CLASS(reservation_ww_class); >> +DEFINE_WW_CLASS_WDIE(reservation_ww_class); >> EXPORT_SYMBOL(reservation_ww_class); >> >> struct lock_class_key reservation_seqcount_class; >> diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c >> index 8a5100685875..ff00a814f617 100644 >> --- a/drivers/gpu/drm/drm_modeset_lock.c >> +++ b/drivers/gpu/drm/drm_modeset_lock.c >> @@ -70,7 +70,7 @@ >> * lists and lookup data structures. >> */ >> >> -static DEFINE_WW_CLASS(crtc_ww_class); >> +static DEFINE_WW_CLASS_WDIE(crtc_ww_class); >> >> /** >> * drm_modeset_lock_all - take all modeset locks >> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h >> index 39fda195bf78..3880813b7db5 100644 >> --- a/include/linux/ww_mutex.h >> +++ b/include/linux/ww_mutex.h >> @@ -8,6 +8,8 @@ >> * >> * Wound/wait implementation: >> * Copyright (C) 2013 Canonical Ltd. >> + * Choice of algorithm: >> + * Copyright (C) 2018 WMWare Inc. >> * >> * This file contains the main data structure and API definitions. >> */ >> @@ -23,15 +25,17 @@ struct ww_class { >> struct lock_class_key mutex_key; >> const char *acquire_name; >> const char *mutex_name; >> + unsigned int is_wait_die; >> }; >> >> struct ww_acquire_ctx { >> struct task_struct *task; >> unsigned long stamp; >> unsigned acquired; >> + unsigned int wounded; >> + struct ww_class *ww_class; >> #ifdef CONFIG_DEBUG_MUTEXES >> unsigned done_acquire; >> - struct ww_class *ww_class; >> struct ww_mutex *contending_lock; >> #endif >> #ifdef CONFIG_DEBUG_LOCK_ALLOC >> @@ -58,17 +62,21 @@ struct ww_mutex { >> # define __WW_CLASS_MUTEX_INITIALIZER(lockname, class) >> #endif >> >> -#define __WW_CLASS_INITIALIZER(ww_class) \ >> +#define __WW_CLASS_INITIALIZER(ww_class, _is_wait_die) \ >> { .stamp = ATOMIC_LONG_INIT(0) \ >> , .acquire_name = #ww_class "_acquire" \ >> - , .mutex_name = #ww_class "_mutex" } >> + , .mutex_name = #ww_class "_mutex" \ >> + , .is_wait_die = _is_wait_die } >> >> #define __WW_MUTEX_INITIALIZER(lockname, class) \ >> { .base = __MUTEX_INITIALIZER(lockname.base) \ >> __WW_CLASS_MUTEX_INITIALIZER(lockname, class) } >> >> #define DEFINE_WW_CLASS(classname) \ >> - struct ww_class classname = __WW_CLASS_INITIALIZER(classname) >> + struct ww_class classname = __WW_CLASS_INITIALIZER(classname, 0) >> + >> +#define DEFINE_WW_CLASS_WDIE(classname) \ >> + struct ww_class classname = __WW_CLASS_INITIALIZER(classname, 1) >> >> #define DEFINE_WW_MUTEX(mutexname, ww_class) \ >> struct ww_mutex mutexname = __WW_MUTEX_INITIALIZER(mutexname, ww_class) >> @@ -123,8 +131,9 @@ static inline void ww_acquire_init(struct ww_acquire_ctx *ctx, >> ctx->task = current; >> ctx->stamp = atomic_long_inc_return_relaxed(&ww_class->stamp); >> ctx->acquired = 0; >> -#ifdef CONFIG_DEBUG_MUTEXES >> ctx->ww_class = ww_class; >> + ctx->wounded = false; >> +#ifdef CONFIG_DEBUG_MUTEXES >> ctx->done_acquire = 0; >> ctx->contending_lock = NULL; >> #endif >> diff --git a/kernel/locking/locktorture.c b/kernel/locking/locktorture.c >> index 6850ffd69125..e861c1bf0e1e 100644 >> --- a/kernel/locking/locktorture.c >> +++ b/kernel/locking/locktorture.c >> @@ -365,7 +365,7 @@ static struct lock_torture_ops mutex_lock_ops = { >> }; >> >> #include <linux/ww_mutex.h> >> -static DEFINE_WW_CLASS(torture_ww_class); >> +static DEFINE_WW_CLASS_WDIE(torture_ww_class); >> static DEFINE_WW_MUTEX(torture_ww_mutex_0, &torture_ww_class); >> static DEFINE_WW_MUTEX(torture_ww_mutex_1, &torture_ww_class); >> static DEFINE_WW_MUTEX(torture_ww_mutex_2, &torture_ww_class); >> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c >> index 2048359f33d2..ffa00b5aaf03 100644 >> --- a/kernel/locking/mutex.c >> +++ b/kernel/locking/mutex.c >> @@ -290,12 +290,49 @@ __ww_ctx_stamp_after(struct ww_acquire_ctx *a, struct ww_acquire_ctx *b) >> (a->stamp != b->stamp || a > b); >> } >> >> +/* >> + * Wound the lock holder transaction if it's younger than the contending >> + * transaction, and there is a possibility of a deadlock. >> + * Also if the lock holder transaction isn't the current transaction, >> + * make sure it's woken up in case it's sleeping on another ww mutex. >> + */ >> +static bool __ww_mutex_wound(struct mutex *lock, >> + struct ww_acquire_ctx *ww_ctx, >> + struct ww_acquire_ctx *hold_ctx) >> +{ >> + struct task_struct *owner = __mutex_owner(lock); >> + >> + lockdep_assert_held(&lock->wait_lock); >> + >> + if (owner && hold_ctx && __ww_ctx_stamp_after(hold_ctx, ww_ctx) && >> + ww_ctx->acquired > 0) { >> + hold_ctx->wounded = 1; >> + >> + /* >> + * wake_up_process() paired with set_current_state() inserts >> + * sufficient barriers to make sure @owner either sees it's >> + * wounded or has a wakeup pending to re-read the wounded >> + * state. > IIUC, "sufficient barriers" = full memory barriers (here). (You may > want to be more specific.) Thanks for reviewing! OK. What about if someone relaxes that in the future? I mean, what we care about in this code is just that we have sufficient barriers for that statement to be true, regardless what type of barriers those really are? > >> + * >> + * The value of hold_ctx->wounded in >> + * __ww_mutex_lock_check_stamp(); > Missing parts/incomplete sentence? Oops. I'll fix in next version. > > Andrea Thanks, Thomas

6 years, 7 months

[PATCH] staging: android: ion: Return an ERR_PTR in ion_map_kernel

by Laura Abbott

The expected return value from ion_map_kernel is an ERR_PTR. The error path for a vmalloc failure currently just returns NULL, triggering a warning in ion_buffer_kmap_get. Encode the vmalloc failure as an ERR_PTR. Reported-by: syzbot+55b1d9f811650de944c6(a)syzkaller.appspotmail.com Signed-off-by: Laura Abbott <labbott(a)redhat.com> --- drivers/staging/android/ion/ion_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/android/ion/ion_heap.c b/drivers/staging/android/ion/ion_heap.c index 772dad65396e..f32c12439eee 100644 --- a/drivers/staging/android/ion/ion_heap.c +++ b/drivers/staging/android/ion/ion_heap.c @@ -29,7 +29,7 @@ void *ion_heap_map_kernel(struct ion_heap *heap, struct page **tmp = pages; if (!pages) - return NULL; + return ERR_PTR(-ENOMEM); if (buffer->flags & ION_FLAG_CACHED) pgprot = PAGE_KERNEL; -- 2.17.0

6 years, 7 months

[PATCH resend] drivers: dma-buf: Change %p to %pK in debug messages

by Daniel Rosenberg

The format specifier %p can leak kernel addresses while not valuing the kptr_restrict system settings. Use %pK instead of %p, which also evaluates whether kptr_restrict is set. Signed-off-by: Divya Ponnusamy <pdivya(a)codeaurora.org> Signed-off-by: Daniel Rosenberg <drosen(a)google.com> Cc: stable <stable(a)vger.kernel.org> --- drivers/dma-buf/sync_debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c index c4c8ecb24aa9..d8d340542a79 100644 --- a/drivers/dma-buf/sync_debug.c +++ b/drivers/dma-buf/sync_debug.c @@ -133,7 +133,7 @@ static void sync_print_sync_file(struct seq_file *s, char buf[128]; int i; - seq_printf(s, "[%p] %s: %s\n", sync_file, + seq_printf(s, "[%pK] %s: %s\n", sync_file, sync_file_get_name(sync_file, buf, sizeof(buf)), sync_status_str(dma_fence_get_status(sync_file->fence))); -- 2.17.0.441.gb46fe60e1d-goog

6 years, 7 months

[PATCH v5] Add udmabuf misc device

by Gerd Hoffmann

A driver to let userspace turn memfd regions into dma-bufs. Use case: Allows qemu create dmabufs for the vga framebuffer or virtio-gpu ressources. Then they can be passed around to display those guest things on the host. To spice client for classic full framebuffer display, and hopefully some day to wayland server for seamless guest window display. qemu test branch: https://git.kraxel.org/cgit/qemu/log/?h=sirius/udmabuf Cc: David Airlie <airlied(a)linux.ie> Cc: Tomeu Vizoso <tomeu.vizoso(a)collabora.com> Cc: Daniel Vetter <daniel(a)ffwll.ch> Signed-off-by: Gerd Hoffmann <kraxel(a)redhat.com> --- Documentation/ioctl/ioctl-number.txt | 1 + include/uapi/linux/udmabuf.h | 33 +++ drivers/dma-buf/udmabuf.c | 302 ++++++++++++++++++++++ tools/testing/selftests/drivers/dma-buf/udmabuf.c | 96 +++++++ MAINTAINERS | 8 + drivers/dma-buf/Kconfig | 8 + drivers/dma-buf/Makefile | 1 + tools/testing/selftests/drivers/dma-buf/Makefile | 5 + 8 files changed, 454 insertions(+) create mode 100644 include/uapi/linux/udmabuf.h create mode 100644 drivers/dma-buf/udmabuf.c create mode 100644 tools/testing/selftests/drivers/dma-buf/udmabuf.c create mode 100644 tools/testing/selftests/drivers/dma-buf/Makefile diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt index 7f7413e597..1720c9e04a 100644 --- a/Documentation/ioctl/ioctl-number.txt +++ b/Documentation/ioctl/ioctl-number.txt @@ -271,6 +271,7 @@ Code Seq#(hex) Include File Comments 't' 90-91 linux/toshiba.h toshiba and toshiba_acpi SMM 'u' 00-1F linux/smb_fs.h gone 'u' 20-3F linux/uvcvideo.h USB video class host driver +'u' 40-4f linux/udmabuf.h userspace dma-buf misc device 'v' 00-1F linux/ext2_fs.h conflict! 'v' 00-1F linux/fs.h conflict! 'v' 00-0F linux/sonypi.h conflict! diff --git a/include/uapi/linux/udmabuf.h b/include/uapi/linux/udmabuf.h new file mode 100644 index 0000000000..46b6532ed8 --- /dev/null +++ b/include/uapi/linux/udmabuf.h @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI_LINUX_UDMABUF_H +#define _UAPI_LINUX_UDMABUF_H + +#include <linux/types.h> +#include <linux/ioctl.h> + +#define UDMABUF_FLAGS_CLOEXEC 0x01 + +struct udmabuf_create { + __u32 memfd; + __u32 flags; + __u64 offset; + __u64 size; +}; + +struct udmabuf_create_item { + __u32 memfd; + __u32 __pad; + __u64 offset; + __u64 size; +}; + +struct udmabuf_create_list { + __u32 flags; + __u32 count; + struct udmabuf_create_item list[]; +}; + +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create) +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct udmabuf_create_list) + +#endif /* _UAPI_LINUX_UDMABUF_H */ diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c new file mode 100644 index 0000000000..ae1390d2bd --- /dev/null +++ b/drivers/dma-buf/udmabuf.c @@ -0,0 +1,302 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/init.h> +#include <linux/module.h> +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/miscdevice.h> +#include <linux/dma-buf.h> +#include <linux/highmem.h> +#include <linux/cred.h> +#include <linux/shmem_fs.h> + +#include <uapi/linux/udmabuf.h> + +struct udmabuf { + u32 pagecount; + struct page **pages; +}; + +static int udmabuf_vm_fault(struct vm_fault *vmf) +{ + struct vm_area_struct *vma = vmf->vma; + struct udmabuf *ubuf = vma->vm_private_data; + + if (WARN_ON(vmf->pgoff >= ubuf->pagecount)) + return VM_FAULT_SIGBUS; + + vmf->page = ubuf->pages[vmf->pgoff]; + get_page(vmf->page); + return 0; +} + +static const struct vm_operations_struct udmabuf_vm_ops = { + .fault = udmabuf_vm_fault, +}; + +static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma) +{ + struct udmabuf *ubuf = buf->priv; + + if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0) + return -EINVAL; + + vma->vm_ops = &udmabuf_vm_ops; + vma->vm_private_data = ubuf; + return 0; +} + +static struct sg_table *map_udmabuf(struct dma_buf_attachment *at, + enum dma_data_direction direction) +{ + struct udmabuf *ubuf = at->dmabuf->priv; + struct sg_table *sg; + + sg = kzalloc(sizeof(*sg), GFP_KERNEL); + if (!sg) + goto err1; + if (sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount, + 0, ubuf->pagecount << PAGE_SHIFT, + GFP_KERNEL) < 0) + goto err2; + if (!dma_map_sg(at->dev, sg->sgl, sg->nents, direction)) + goto err3; + + return sg; + +err3: + sg_free_table(sg); +err2: + kfree(sg); +err1: + return ERR_PTR(-ENOMEM); +} + +static void unmap_udmabuf(struct dma_buf_attachment *at, + struct sg_table *sg, + enum dma_data_direction direction) +{ + sg_free_table(sg); + kfree(sg); +} + +static void release_udmabuf(struct dma_buf *buf) +{ + struct udmabuf *ubuf = buf->priv; + pgoff_t pg; + + for (pg = 0; pg < ubuf->pagecount; pg++) + put_page(ubuf->pages[pg]); + kfree(ubuf->pages); + kfree(ubuf); +} + +static void *kmap_atomic_udmabuf(struct dma_buf *buf, unsigned long page_num) +{ + struct udmabuf *ubuf = buf->priv; + struct page *page = ubuf->pages[page_num]; + + return kmap_atomic(page); +} + +static void kunmap_atomic_udmabuf(struct dma_buf *buf, unsigned long page_num, + void *vaddr) +{ + kunmap_atomic(vaddr); +} + +static void *kmap_udmabuf(struct dma_buf *buf, unsigned long page_num) +{ + struct udmabuf *ubuf = buf->priv; + struct page *page = ubuf->pages[page_num]; + + return kmap(page); +} + +static void kunmap_udmabuf(struct dma_buf *buf, unsigned long page_num, + void *vaddr) +{ + kunmap(vaddr); +} + +static struct dma_buf_ops udmabuf_ops = { + .map_dma_buf = map_udmabuf, + .unmap_dma_buf = unmap_udmabuf, + .release = release_udmabuf, + .map_atomic = kmap_atomic_udmabuf, + .unmap_atomic = kunmap_atomic_udmabuf, + .map = kmap_udmabuf, + .unmap = kunmap_udmabuf, + .mmap = mmap_udmabuf, +}; + +#define SEALS_WANTED (F_SEAL_SHRINK) +#define SEALS_DENIED (F_SEAL_WRITE) + +static long udmabuf_create(struct udmabuf_create_list *head, + struct udmabuf_create_item *list) +{ + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); + struct file *memfd = NULL; + struct udmabuf *ubuf; + struct dma_buf *buf; + pgoff_t pgoff, pgcnt, pgidx, pgbuf; + struct page *page; + int seals, ret = -EINVAL; + u32 i, flags; + + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL); + if (!ubuf) + return -ENOMEM; + + for (i = 0; i < head->count; i++) { + if (!IS_ALIGNED(list[i].offset, PAGE_SIZE)) + goto err_free_ubuf; + if (!IS_ALIGNED(list[i].size, PAGE_SIZE)) + goto err_free_ubuf; + ubuf->pagecount += list[i].size >> PAGE_SHIFT; + } + ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page *), + GFP_KERNEL); + if (!ubuf->pages) { + ret = -ENOMEM; + goto err_free_ubuf; + } + + pgbuf = 0; + for (i = 0; i < head->count; i++) { + memfd = fget(list[i].memfd); + if (!memfd) + goto err_put_pages; + if (!shmem_mapping(file_inode(memfd)->i_mapping)) + goto err_put_pages; + seals = memfd_fcntl(memfd, F_GET_SEALS, 0); + if (seals == -EINVAL || + (seals & SEALS_WANTED) != SEALS_WANTED || + (seals & SEALS_DENIED) != 0) + goto err_put_pages; + pgoff = list[i].offset >> PAGE_SHIFT; + pgcnt = list[i].size >> PAGE_SHIFT; + for (pgidx = 0; pgidx < pgcnt; pgidx++) { + page = shmem_read_mapping_page( + file_inode(memfd)->i_mapping, pgoff + pgidx); + if (IS_ERR(page)) { + ret = PTR_ERR(page); + goto err_put_pages; + } + ubuf->pages[pgbuf++] = page; + } + fput(memfd); + } + memfd = NULL; + + exp_info.ops = &udmabuf_ops; + exp_info.size = ubuf->pagecount << PAGE_SHIFT; + exp_info.priv = ubuf; + + buf = dma_buf_export(&exp_info); + if (IS_ERR(buf)) { + ret = PTR_ERR(buf); + goto err_put_pages; + } + + flags = 0; + if (head->flags & UDMABUF_FLAGS_CLOEXEC) + flags |= O_CLOEXEC; + return dma_buf_fd(buf, flags); + +err_put_pages: + while (pgbuf > 0) + put_page(ubuf->pages[--pgbuf]); +err_free_ubuf: + fput(memfd); + kfree(ubuf->pages); + kfree(ubuf); + return ret; +} + +static long udmabuf_ioctl_create(struct file *filp, unsigned long arg) +{ + struct udmabuf_create create; + struct udmabuf_create_list head; + struct udmabuf_create_item list; + + if (copy_from_user(&create, (void __user *)arg, + sizeof(struct udmabuf_create))) + return -EFAULT; + + head.flags = create.flags; + head.count = 1; + list.memfd = create.memfd; + list.offset = create.offset; + list.size = create.size; + + return udmabuf_create(&head, &list); +} + +static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg) +{ + struct udmabuf_create_list head; + struct udmabuf_create_item *list; + int ret = -EINVAL; + u32 lsize; + + if (copy_from_user(&head, (void __user *)arg, sizeof(head))) + return -EFAULT; + if (head.count > 1024) + return -EINVAL; + lsize = sizeof(struct udmabuf_create_item) * head.count; + list = memdup_user((void __user *)(arg + sizeof(head)), lsize); + if (IS_ERR(list)) + return PTR_ERR(list); + + ret = udmabuf_create(&head, list); + kfree(list); + return ret; +} + +static long udmabuf_ioctl(struct file *filp, unsigned int ioctl, + unsigned long arg) +{ + long ret; + + switch (ioctl) { + case UDMABUF_CREATE: + ret = udmabuf_ioctl_create(filp, arg); + break; + case UDMABUF_CREATE_LIST: + ret = udmabuf_ioctl_create_list(filp, arg); + break; + default: + ret = -EINVAL; + break; + } + return ret; +} + +static const struct file_operations udmabuf_fops = { + .owner = THIS_MODULE, + .unlocked_ioctl = udmabuf_ioctl, +}; + +static struct miscdevice udmabuf_misc = { + .minor = MISC_DYNAMIC_MINOR, + .name = "udmabuf", + .fops = &udmabuf_fops, +}; + +static int __init udmabuf_dev_init(void) +{ + return misc_register(&udmabuf_misc); +} + +static void __exit udmabuf_dev_exit(void) +{ + misc_deregister(&udmabuf_misc); +} + +module_init(udmabuf_dev_init) +module_exit(udmabuf_dev_exit) + +MODULE_AUTHOR("Gerd Hoffmann <kraxel(a)redhat.com>"); +MODULE_LICENSE("GPL v2"); diff --git a/tools/testing/selftests/drivers/dma-buf/udmabuf.c b/tools/testing/selftests/drivers/dma-buf/udmabuf.c new file mode 100644 index 0000000000..376b1d6730 --- /dev/null +++ b/tools/testing/selftests/drivers/dma-buf/udmabuf.c @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <string.h> +#include <errno.h> +#include <fcntl.h> +#include <malloc.h> + +#include <sys/ioctl.h> +#include <sys/syscall.h> +#include <linux/memfd.h> +#include <linux/udmabuf.h> + +#define TEST_PREFIX "drivers/dma-buf/udmabuf" +#define NUM_PAGES 4 + +static int memfd_create(const char *name, unsigned int flags) +{ + return syscall(__NR_memfd_create, name, flags); +} + +int main(int argc, char *argv[]) +{ + struct udmabuf_create create; + int devfd, memfd, buf, ret; + off_t size; + void *mem; + + devfd = open("/dev/udmabuf", O_RDWR); + if (devfd < 0) { + printf("%s: [skip,no-udmabuf]\n", TEST_PREFIX); + exit(77); + } + + memfd = memfd_create("udmabuf-test", MFD_CLOEXEC); + if (memfd < 0) { + printf("%s: [skip,no-memfd]\n", TEST_PREFIX); + exit(77); + } + + size = getpagesize() * NUM_PAGES; + ret = ftruncate(memfd, size); + if (ret == -1) { + printf("%s: [FAIL,memfd-truncate]\n", TEST_PREFIX); + exit(1); + } + + memset(&create, 0, sizeof(create)); + + /* should fail (offset not page aligned) */ + create.memfd = memfd; + create.offset = getpagesize()/2; + create.size = getpagesize(); + buf = ioctl(devfd, UDMABUF_CREATE, &create); + if (buf >= 0) { + printf("%s: [FAIL,test-1]\n", TEST_PREFIX); + exit(1); + } + + /* should fail (size not multiple of page) */ + create.memfd = memfd; + create.offset = 0; + create.size = getpagesize()/2; + buf = ioctl(devfd, UDMABUF_CREATE, &create); + if (buf >= 0) { + printf("%s: [FAIL,test-2]\n", TEST_PREFIX); + exit(1); + } + + /* should fail (not memfd) */ + create.memfd = 0; /* stdin */ + create.offset = 0; + create.size = size; + buf = ioctl(devfd, UDMABUF_CREATE, &create); + if (buf >= 0) { + printf("%s: [FAIL,test-3]\n", TEST_PREFIX); + exit(1); + } + + /* should work */ + create.memfd = memfd; + create.offset = 0; + create.size = size; + buf = ioctl(devfd, UDMABUF_CREATE, &create); + if (buf < 0) { + printf("%s: [FAIL,test-4]\n", TEST_PREFIX); + exit(1); + } + + fprintf(stderr, "%s: ok\n", TEST_PREFIX); + close(buf); + close(memfd); + close(devfd); + return 0; +} diff --git a/MAINTAINERS b/MAINTAINERS index 9c125f705f..aa91159afd 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14426,6 +14426,14 @@ S: Maintained F: Documentation/filesystems/udf.txt F: fs/udf/ +UDMABUF DRIVER +M: Gerd Hoffmann <kraxel(a)redhat.com> +L: dri-devel(a)lists.freedesktop.org +S: Maintained +F: drivers/dma-buf/udmabuf.c +F: include/uapi/linux/udmabuf.h +F: tools/testing/selftests/drivers/dma-buf/udmabuf.c + UDRAW TABLET M: Bastien Nocera <hadess(a)hadess.net> L: linux-input(a)vger.kernel.org diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index ed3b785bae..338129eb12 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -30,4 +30,12 @@ config SW_SYNC WARNING: improper use of this can result in deadlocking kernel drivers from userspace. Intended for test and debug only. +config UDMABUF + bool "userspace dmabuf misc driver" + default n + depends on DMA_SHARED_BUFFER + help + A driver to let userspace turn memfd regions into dma-bufs. + Qemu can use this to create host dmabufs for guest framebuffers. + endmenu diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index c33bf88631..0913a6ccab 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -1,3 +1,4 @@ obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o seqno-fence.o obj-$(CONFIG_SYNC_FILE) += sync_file.o obj-$(CONFIG_SW_SYNC) += sw_sync.o sync_debug.o +obj-$(CONFIG_UDMABUF) += udmabuf.o diff --git a/tools/testing/selftests/drivers/dma-buf/Makefile b/tools/testing/selftests/drivers/dma-buf/Makefile new file mode 100644 index 0000000000..4154c3d7aa --- /dev/null +++ b/tools/testing/selftests/drivers/dma-buf/Makefile @@ -0,0 +1,5 @@ +CFLAGS += -I../../../../../usr/include/ + +TEST_GEN_PROGS := udmabuf + +include ../../lib.mk -- 2.9.3

6 years, 7 months

Re: [Linaro-mm-sig] [PATCH 1/5] dma_buf: remove device parameter from attach callback

by Christian König

Am 01.06.2018 um 16:02 schrieb Michel Dänzer: > On 2018-06-01 02:11 PM, Christian König wrote: >> Sorry, accidentally send this series without a cover letter. >> >> This is a cleanup to the DMA-buf interface, which is also a prerequisite >> to unpinned DMA-buf operation. >> >> Patch #1 and #2 just remove unused functionality and clean up callback >> parameters. >> >> Patch #3 and #4 introduce taking the reservation lock during >> mapping/unmapping of DMA-bufs. >> >> This introduces a common lock where both exporter as well as importer >> can then use in the future for unpinned DMA-buf operation. >> >> This of course means that exporters should now not take this reservation >> lock manually any more. The DRM drivers don't seem to actually do that, >> but I'm not 100% sure about other implementations. >> >> Patch #5 then makes use of the new lock to simplify the DMA-buf import >> handling in amdgpu. > Please rebase this series on top of > https://patchwork.freedesktop.org/patch/226311/ and update the > documentation in amdgpu_prime.c as needed in each patch. Sure. In this case can we get your patches committed to amd-staging-drm-next ASAP? Thanks, Christian.

6 years, 7 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig