Hi, As stated in the patch log entry, the main use case for recursive lock is porting legacy code on top of ODP. It’s not feasible to modify large legacy projects to not use recursive locking when it’s widely used and expected by the programming model.
For example, FreeBSD uses recursive mutexes and read-write locks. The link under high lights recursive lock creations in user space FreeBSD TCP/IP stack project. https://github.com/pkelsey/libuinet/search?utf8=%E2%9C%93&q=recurse Also, DPDK provides recursive spinlock (but not rwlock). With another look into this, it would be better to define recursive versions of these two lock types: odp_spinlock_rec_xxx() odp_rwlock_rec_xxx() These could be implemented also as helpers, but proper API would allow HW specific optimization of these fast path APIs. -Petri From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of ext Bill Fischofer Sent: Sunday, July 05, 2015 11:56 PM To: Maxim Uvarov Cc: LNG ODP Mailman List Subject: Re: [lng-odp] [API-NEXT PATCH] api: reclock: added recursive lock An application that needed this functionality for legacy use could construct these semantics on top of the ODP locking primitives. The only reason to have it be an ODP API would be if the primitive itself were in common use across multiple platforms that could implement it differently. Needs further discussion, but seems a low-priority item for now. On Fri, Jul 3, 2015 at 10:23 AM, Maxim Uvarov <maxim.uva...@linaro.org<mailto:maxim.uva...@linaro.org>> wrote: On 3 July 2015 at 18:19, Ivan Khoronzhuk <ivan.khoronz...@linaro.org<mailto:ivan.khoronz...@linaro.org>> wrote: Maxim, On 03.07.15 18:13, Maxim Uvarov wrote: On 3 July 2015 at 17:59, Ivan Khoronzhuk <ivan.khoronz...@linaro.org<mailto:ivan.khoronz...@linaro.org> <mailto:ivan.khoronz...@linaro.org<mailto:ivan.khoronz...@linaro.org>>> wrote: Maxim, I've never used such kind of lock. They are not used in LK for a lot of reasons. It's not the same as rwlocks, it's intended for other purposes - to avoid deadlocks ) by hiding bad design. If app calls twice spinlock_lock() (in subroutine once again for instance) it will deadlock... forever... But in case with reclock, it will continue to work. I suppose some apps use this feature .... :).. The main drawback that it hides unintentional recursive locks. The reclock can be absolutely avoided by better design, on my opinion. Yes, I also as many of others here, worked with kernel before. I even looked to fresh sources if there is something changed. But that kind of locks are not there and even there were number of reasons to not accept it. My question was to Petri to clarify use case for that. Maybe we can propose better approach than that. btw, I also never used recursive mutex, only locked one. :) Maxim. I suppose there is no any reason except support of legacy apps. That's why I proposed to disable it`s usage for new apps somehow. that is api-next. I.e. that api is under discussion to accept it or not. Ivan, I agree with you that we need to avoid it's usage. Maxim. On 03.07.15 17:44, Maxim Uvarov wrote: On 07/03/15 17:00, Ivan Khoronzhuk wrote: Maxim, mutex and spinlock is not the same ... yes, I know. What is the use case of that locks? Defend some data between control thread and worker thread? I.e. odp worker holds write lock and updates something. While control thread needs do periodical read and needs to hold read lock? Is that something like linux rwlocks? Hide spinlocks like that will be vary hard to debug in future. Maxim. On 03.07.15 16:54, Maxim Uvarov wrote: Petri, looks like you invented mutex (mtx_recursive) : https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.aix.basetrf1/mtx_destroy.htm Thanks, Maxim. On 07/03/15 16:25, Petri Savolainen wrote: Applications can use recursive locks to avoid deadlock from single thread acquiring the same lock multiple times. Recursive locks are commonly used by legacy applications. Signed-off-by: Petri Savolainen <petri.savolai...@nokia.com<mailto:petri.savolai...@nokia.com> <mailto:petri.savolai...@nokia.com<mailto:petri.savolai...@nokia.com>>> --- include/odp.h | 1 + include/odp/api/reclock.h | 82 ++++++++++++++++++++++ platform/linux-generic/Makefile.am | 4 ++ .../linux-generic/include/odp/plat/reclock_types.h | 46 ++++++++++++ platform/linux-generic/include/odp/reclock.h | 28 ++++++++ platform/linux-generic/odp_reclock.c | 68 ++++++++++++++++++ 6 files changed, 229 insertions(+) create mode 100644 include/odp/api/reclock.h create mode 100644 platform/linux-generic/include/odp/plat/reclock_types.h create mode 100644 platform/linux-generic/include/odp/reclock.h create mode 100644 platform/linux-generic/odp_reclock.c diff --git a/include/odp.h b/include/odp.h index fe1dc74..9acd1c4 100644 --- a/include/odp.h +++ b/include/odp.h @@ -54,6 +54,7 @@ extern "C" { #include <odp/random.h> #include <odp/errno.h> #include <odp/thrmask.h> +#include <odp/reclock.h> #ifdef __cplusplus } diff --git a/include/odp/api/reclock.h b/include/odp/api/reclock.h new file mode 100644 index 0000000..06ea702 --- /dev/null +++ b/include/odp/api/reclock.h @@ -0,0 +1,82 @@ +/* Copyright (c) 2015, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +/** + * @file + * + * ODP recursive lock + */ + +#ifndef ODP_API_RECLOCK_H_ +#define ODP_API_RECLOCK_H_ + +#ifdef __cplusplus +extern "C" { +#endif + +/** @addtogroup odp_synchronizers + * Operations on recursive locks. + * @{ + */ + +/** + * @typedef odp_reclock_t + * Recursive lock + * + * A thread can acquire the lock multiple times without a deadlock. To release + * the lock, the thread must unlock it the same number of times. + */ + +/** + * Initialize recursive lock. + * + * @param lock Pointer to a lock + */ +void odp_reclock_init(odp_reclock_t *lock); + +/** + * Acquire recursive lock. + * + * @param lock Pointer to a lock + */ +void odp_reclock_lock(odp_reclock_t *lock); + +/** + * Try to acquire recursive lock. + * + * @param lock Pointer to a lock + * + * @retval 1 lock acquired + * @retval 0 lock not acquired + */ +int odp_reclock_trylock(odp_reclock_t *lock); + +/** + * Release recursive lock. + * + * @param lock Pointer to a lock + */ +void odp_reclock_unlock(odp_reclock_t *lock); + +/** + * Check if recursive lock is locked. + * + * @param lock Pointer to a lock + * + * @retval 1 lock is locked + * @retval 0 lock is not locked + */ +int odp_reclock_is_locked(odp_reclock_t *lock); + +/** + * @} + */ + +#ifdef __cplusplus +} +#endif + +#endif diff --git a/platform/linux-generic/Makefile.am b/platform/linux-generic/Makefile.am index 765db34..b6fd611 100644 --- a/platform/linux-generic/Makefile.am +++ b/platform/linux-generic/Makefile.am @@ -32,6 +32,7 @@ odpinclude_HEADERS = \ $(top_srcdir)/platform/linux-generic/include/odp/pool.h \ $(top_srcdir)/platform/linux-generic/include/odp/queue.h \ $(top_srcdir)/platform/linux-generic/include/odp/random.h \ + $(top_srcdir)/platform/linux-generic/include/odp/reclock.h \ $(top_srcdir)/platform/linux-generic/include/odp/rwlock.h \ $(top_srcdir)/platform/linux-generic/include/odp/schedule.h \ $(top_srcdir)/platform/linux-generic/include/odp/shared_memory.h \ @@ -60,6 +61,7 @@ odpplatinclude_HEADERS = \ $(top_srcdir)/platform/linux-generic/include/odp/plat/packet_io_types.h \ $(top_srcdir)/platform/linux-generic/include/odp/plat/pool_types.h \ $(top_srcdir)/platform/linux-generic/include/odp/plat/queue_types.h \ + $(top_srcdir)/platform/linux-generic/include/odp/plat/reclock_types.h \ $(top_srcdir)/platform/linux-generic/include/odp/plat/rwlock_types.h \ $(top_srcdir)/platform/linux-generic/include/odp/plat/schedule_types.h \ $(top_srcdir)/platform/linux-generic/include/odp/plat/shared_memory_types.h \ @@ -94,6 +96,7 @@ odpapiinclude_HEADERS = \ $(top_srcdir)/include/odp/api/pool.h \ $(top_srcdir)/include/odp/api/queue.h \ $(top_srcdir)/include/odp/api/random.h \ + $(top_srcdir)/include/odp/api/reclock.h \ $(top_srcdir)/include/odp/api/rwlock.h \ $(top_srcdir)/include/odp/api/schedule.h \ $(top_srcdir)/include/odp/api/shared_memory.h \ @@ -162,6 +165,7 @@ __LIB__libodp_la_SOURCES = \ odp_pool.c \ odp_queue.c \ ../../helper/ring.c \ + odp_reclock.c \ odp_rwlock.c \ odp_schedule.c \ odp_shared_memory.c \ diff --git a/platform/linux-generic/include/odp/plat/reclock_types.h b/platform/linux-generic/include/odp/plat/reclock_types.h new file mode 100644 index 0000000..07ec0b9 --- /dev/null +++ b/platform/linux-generic/include/odp/plat/reclock_types.h @@ -0,0 +1,46 @@ +/* Copyright (c) 2015, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +/** + * @file + * + * ODP recursive lock + */ + +#ifndef ODP_RECLOCK_TYPES_H_ +#define ODP_RECLOCK_TYPES_H_ + +#ifdef __cplusplus +extern "C" { +#endif + +#include <odp/spinlock.h> + +/** + * @internal + * ODP recursive lock + */ +struct odp_reclock_s { + odp_spinlock_t lock; /**< the lock */ + int owner; /**< thread inside the lock */ + int num; /**< number of times locked */ +}; + +/** @addtogroup odp_synchronizers + * @{ + */ + +typedef struct odp_reclock_s odp_reclock_t; + +/** + * @} + */ + +#ifdef __cplusplus +} +#endif + +#endif diff --git a/platform/linux-generic/include/odp/reclock.h b/platform/linux-generic/include/odp/reclock.h new file mode 100644 index 0000000..3f46f51 --- /dev/null +++ b/platform/linux-generic/include/odp/reclock.h @@ -0,0 +1,28 @@ +/* Copyright (c) 2015, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +/** + * @file + * + * ODP resursive lock + */ + +#ifndef ODP_PLAT_RECLOCK_H_ +#define ODP_PLAT_RECLOCK_H_ + +#ifdef __cplusplus +extern "C" { +#endif + +#include <odp/plat/reclock_types.h> + +#include <odp/api/reclock.h> + +#ifdef __cplusplus +} +#endif + +#endif diff --git a/platform/linux-generic/odp_reclock.c b/platform/linux-generic/odp_reclock.c new file mode 100644 index 0000000..674bc45 --- /dev/null +++ b/platform/linux-generic/odp_reclock.c @@ -0,0 +1,68 @@ +/* Copyright (c) 2013, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +#include <odp/reclock.h> +#include <odp/thread.h> + +#define NO_OWNER (-1) + +void odp_reclock_init(odp_reclock_t *reclock) +{ + odp_spinlock_init(&reclock->lock); + reclock->owner = NO_OWNER; + reclock->num = 0; +} + +void odp_reclock_lock(odp_reclock_t *reclock) +{ + int thr = odp_thread_id(); + + if (reclock->owner == thr) { + reclock->num++; + return; + } + + odp_spinlock_lock(&reclock->lock); + reclock->owner = thr; +} + +int odp_reclock_trylock(odp_reclock_t *reclock) +{ + int thr = odp_thread_id(); + + if (reclock->owner == thr) { + reclock->num++; + return 1; + } + + if (odp_spinlock_trylock(&reclock->lock)) { + reclock->owner = thr; + return 1; + } else { + return 0; + } +} + +void odp_reclock_unlock(odp_reclock_t *reclock) +{ + reclock->num--; + + if (reclock->num > 0) + return; + + reclock->owner = NO_OWNER; + odp_spinlock_unlock(&reclock->lock); +} + +int odp_reclock_is_locked(odp_reclock_t *reclock) +{ + int thr = odp_thread_id(); + + if (reclock->owner == thr) + return 1; + + return odp_spinlock_is_locked(&reclock->lock); +} _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org> <mailto:lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org>> https://lists.linaro.org/mailman/listinfo/lng-odp _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org> https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
