Re: [PATCH RT] Align rt_mutex inlining with upstream behavior
On 2017-02-13 12:20:52 [+0100], Peter Zijlstra wrote:
> > --- a/include/linux/mutex_rt.h
> > +++ b/include/linux/mutex_rt.h
> > @@ -43,7 +43,12 @@ extern void __lockfunc _mutex_unlock(str
> > #define mutex_lock_killable(l) _mutex_lock_killable(l)
> > #define mutex_trylock(l) _mutex_trylock(l)
> > #define mutex_unlock(l)_mutex_unlock(l)
> > +
> > +#ifdef CONFIG_DEBUG_MUTEXES
> > #define mutex_destroy(l) rt_mutex_destroy(&(l)->lock)
> > +#else
> > +static inline void mutex_destroy(struct mutex *lock) {}
> > +#endif
> >
> > #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > # define mutex_lock_nested(l, s) _mutex_lock_nested(l, s)
> > --- a/kernel/locking/rtmutex.c
> > +++ b/kernel/locking/rtmutex.c
> > @@ -2027,8 +2027,7 @@ void rt_mutex_destroy(struct rt_mutex *l
> > lock->magic = NULL;
> > #endif
> > }
> > -
> > -EXPORT_SYMBOL_GPL(rt_mutex_destroy);
> > +EXPORT_SYMBOL(rt_mutex_destroy);
>
> I don't understand:
>
> $ git grep "EXPORT_SYMBOL.*mutex_destroy"
> kernel/locking/mutex-debug.c:EXPORT_SYMBOL_GPL(mutex_destroy);
> kernel/locking/rtmutex.c:EXPORT_SYMBOL_GPL(rt_mutex_destroy);
>
> Your proposed patch makes it different from mainline.
as discussed off-list, I will drop that _GPL removal hunk and keep only
the (first) inline hunk.
!GPL users should compile with !CONFIG_DEBUG_MUTEXES and
!CONFIG_DEBUG_RT_MUTEXES so there should be no regression.
Sebastian
Re: [PATCH RT] Align rt_mutex inlining with upstream behavior
On Fri, Feb 10, 2017 at 06:50:50PM +0100, Sebastian Andrzej Siewior wrote:
> Alex Goins reported that mutex_destroy() on RT will force a GPL only symbol
> which won't link and therefore fail on a non-GPL kernel module.
> This does not happen on !RT and is a regression on RT which we would like to
> avoid.
> I try here the easy thing and to not use rt_mutex_destroy() if
> CONFIG_DEBUG_MUTEXES is not enabled. This will still break for the DEBUG
> configs so instead of adding a wrapper around rt_mutex_destroy() (which we
> have
> for rt_mutex_lock() for instance) I am simply dropping the GPL part from the
> export.
>
> Reported-by: Alex Goins
> Signed-off-by: Sebastian Andrzej Siewior
> ---
> include/linux/mutex_rt.h |5 +
> kernel/locking/rtmutex.c |3 +--
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> --- a/include/linux/mutex_rt.h
> +++ b/include/linux/mutex_rt.h
> @@ -43,7 +43,12 @@ extern void __lockfunc _mutex_unlock(str
> #define mutex_lock_killable(l) _mutex_lock_killable(l)
> #define mutex_trylock(l) _mutex_trylock(l)
> #define mutex_unlock(l) _mutex_unlock(l)
> +
> +#ifdef CONFIG_DEBUG_MUTEXES
> #define mutex_destroy(l) rt_mutex_destroy(&(l)->lock)
> +#else
> +static inline void mutex_destroy(struct mutex *lock) {}
> +#endif
>
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> # define mutex_lock_nested(l, s) _mutex_lock_nested(l, s)
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -2027,8 +2027,7 @@ void rt_mutex_destroy(struct rt_mutex *l
> lock->magic = NULL;
> #endif
> }
> -
> -EXPORT_SYMBOL_GPL(rt_mutex_destroy);
> +EXPORT_SYMBOL(rt_mutex_destroy);
I don't understand:
$ git grep "EXPORT_SYMBOL.*mutex_destroy"
kernel/locking/mutex-debug.c:EXPORT_SYMBOL_GPL(mutex_destroy);
kernel/locking/rtmutex.c:EXPORT_SYMBOL_GPL(rt_mutex_destroy);
Your proposed patch makes it different from mainline.
Re: [PATCH RT] Align rt_mutex inlining with upstream behavior
On Sat, 11 Feb 2017, Ingo Molnar wrote: > > * Sebastian Andrzej Siewior wrote: > > > On 2017-02-10 10:09:29 [-0800], Andy Ritger wrote: > > > Is the > > > > > > WARN_ON(rt_mutex_is_locked(lock)); > > > > > > in rt_mutex_destroy() valuable in non-CONFIG_DEBUG_MUTEXES kernels, > > > such that it would be better to always call it, and not noop away > > > mutex_destroy() > > > non-CONFIG_DEBUG_MUTEXES kernels? I thought that was your objection to > > > Alex's original patch. > > > > It kind of was… > > So first I removed the GPL symbol. Then I wasn't too happy about it > > especially since it was not introduced as part of RT. So I reverted that > > changed and aligned with mainline behaviour (the mutex_rt.h hunk). But > > then I noticed that with CONFIG_DEBUG_MUTEXES=n and > > CONFIG_DEBUG_RT_MUTEXES=y we still have a regression compared to !RT and > > this was the initial motivation to fix things. > > Then I got curious why mutex_lock() (which is essential rt_mutex_lock()) > > works and noticed the wrapper around it. And while looking at it I > > decided to go back to strip the GPL part from export symbol instead of > > adding a wrapper. And here I am. > > Then I was looking at the patch and decided to align with mainline (and > > keep that one hunk) in case Ingo ask for his GPL symbol. > > tglx and Peter Zijlstra are main co-authors of kernel/locking/rtmutex.c, and > every > author (copyright holder) has to agree to changing a GPL export of a kernel > subsystem's API to a non-GPL export. I'm fine with that change as it really hurts users and makes RT behave the same way as mainline. Acked-by: Thomas Gleixner Thanks, tglx
Re: [PATCH RT] Align rt_mutex inlining with upstream behavior
* Sebastian Andrzej Siewior wrote: > On 2017-02-10 10:09:29 [-0800], Andy Ritger wrote: > > Is the > > > > WARN_ON(rt_mutex_is_locked(lock)); > > > > in rt_mutex_destroy() valuable in non-CONFIG_DEBUG_MUTEXES kernels, > > such that it would be better to always call it, and not noop away > > mutex_destroy() > > non-CONFIG_DEBUG_MUTEXES kernels? I thought that was your objection to > > Alex's original patch. > > It kind of was… > So first I removed the GPL symbol. Then I wasn't too happy about it > especially since it was not introduced as part of RT. So I reverted that > changed and aligned with mainline behaviour (the mutex_rt.h hunk). But > then I noticed that with CONFIG_DEBUG_MUTEXES=n and > CONFIG_DEBUG_RT_MUTEXES=y we still have a regression compared to !RT and > this was the initial motivation to fix things. > Then I got curious why mutex_lock() (which is essential rt_mutex_lock()) > works and noticed the wrapper around it. And while looking at it I > decided to go back to strip the GPL part from export symbol instead of > adding a wrapper. And here I am. > Then I was looking at the patch and decided to align with mainline (and > keep that one hunk) in case Ingo ask for his GPL symbol. tglx and Peter Zijlstra are main co-authors of kernel/locking/rtmutex.c, and every author (copyright holder) has to agree to changing a GPL export of a kernel subsystem's API to a non-GPL export. Thanks, Ingo
Re: [PATCH RT] Align rt_mutex inlining with upstream behavior
This should work. Reviewed-by: Alex Goins Thanks, Alex On Fri, 10 Feb 2017, Sebastian Andrzej Siewior wrote: > On 2017-02-10 10:09:29 [-0800], Andy Ritger wrote: > > Is the > > > > WARN_ON(rt_mutex_is_locked(lock)); > > > > in rt_mutex_destroy() valuable in non-CONFIG_DEBUG_MUTEXES kernels, > > such that it would be better to always call it, and not noop away > > mutex_destroy() > > non-CONFIG_DEBUG_MUTEXES kernels? I thought that was your objection to > > Alex's original patch. > > It kind of was… > So first I removed the GPL symbol. Then I wasn't too happy about it > especially since it was not introduced as part of RT. So I reverted that > changed and aligned with mainline behaviour (the mutex_rt.h hunk). But > then I noticed that with CONFIG_DEBUG_MUTEXES=n and > CONFIG_DEBUG_RT_MUTEXES=y we still have a regression compared to !RT and > this was the initial motivation to fix things. > Then I got curious why mutex_lock() (which is essential rt_mutex_lock()) > works and noticed the wrapper around it. And while looking at it I > decided to go back to strip the GPL part from export symbol instead of > adding a wrapper. And here I am. > Then I was looking at the patch and decided to align with mainline (and > keep that one hunk) in case Ingo ask for his GPL symbol. > > > Thanks, > > - Andy > Sebastian >
Re: [PATCH RT] Align rt_mutex inlining with upstream behavior
On 2017-02-03 08:49:24 [-0800], Andy Ritger wrote:
> > So your problem is simply that your non-GPL module can't link anymore
> > with -RT. Would it help you if I simply replace the export for
> > mutex_destroy with EXPORT_SYMBOL and leave it the function as is?
>
> Yes, definitely.
So this is what I intend to add to the RT patch and I hope Ingo won't
object:
Alex Goins reported that mutex_destroy() on RT will force a GPL only symbol
which won't link and therefore fail on a non-GPL kernel module.
This does not happen on !RT and is a regression on RT which we would like to
avoid.
I try here the easy thing and to not use rt_mutex_destroy() if
CONFIG_DEBUG_MUTEXES is not enabled. This will still break for the DEBUG
configs so instead of adding a wrapper around rt_mutex_destroy() (which we have
for rt_mutex_lock() for instance) I am simply dropping the GPL part from the
export.
Reported-by: Alex Goins
Signed-off-by: Sebastian Andrzej Siewior
---
include/linux/mutex_rt.h |5 +
kernel/locking/rtmutex.c |3 +--
2 files changed, 6 insertions(+), 2 deletions(-)
--- a/include/linux/mutex_rt.h
+++ b/include/linux/mutex_rt.h
@@ -43,7 +43,12 @@ extern void __lockfunc _mutex_unlock(str
#define mutex_lock_killable(l) _mutex_lock_killable(l)
#define mutex_trylock(l) _mutex_trylock(l)
#define mutex_unlock(l)_mutex_unlock(l)
+
+#ifdef CONFIG_DEBUG_MUTEXES
#define mutex_destroy(l) rt_mutex_destroy(&(l)->lock)
+#else
+static inline void mutex_destroy(struct mutex *lock) {}
+#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
# define mutex_lock_nested(l, s) _mutex_lock_nested(l, s)
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -2027,8 +2027,7 @@ void rt_mutex_destroy(struct rt_mutex *l
lock->magic = NULL;
#endif
}
-
-EXPORT_SYMBOL_GPL(rt_mutex_destroy);
+EXPORT_SYMBOL(rt_mutex_destroy);
/**
* __rt_mutex_init - initialize the rt lock
Sebastian
Re: [PATCH RT] Align rt_mutex inlining with upstream behavior
On 2017-02-10 10:09:29 [-0800], Andy Ritger wrote: > Is the > > WARN_ON(rt_mutex_is_locked(lock)); > > in rt_mutex_destroy() valuable in non-CONFIG_DEBUG_MUTEXES kernels, > such that it would be better to always call it, and not noop away > mutex_destroy() > non-CONFIG_DEBUG_MUTEXES kernels? I thought that was your objection to > Alex's original patch. It kind of was… So first I removed the GPL symbol. Then I wasn't too happy about it especially since it was not introduced as part of RT. So I reverted that changed and aligned with mainline behaviour (the mutex_rt.h hunk). But then I noticed that with CONFIG_DEBUG_MUTEXES=n and CONFIG_DEBUG_RT_MUTEXES=y we still have a regression compared to !RT and this was the initial motivation to fix things. Then I got curious why mutex_lock() (which is essential rt_mutex_lock()) works and noticed the wrapper around it. And while looking at it I decided to go back to strip the GPL part from export symbol instead of adding a wrapper. And here I am. Then I was looking at the patch and decided to align with mainline (and keep that one hunk) in case Ingo ask for his GPL symbol. > Thanks, > - Andy Sebastian
Re: [PATCH RT] Align rt_mutex inlining with upstream behavior
On Fri, Feb 10, 2017 at 06:50:50PM +0100, Sebastian Andrzej Siewior wrote:
> On 2017-02-03 08:49:24 [-0800], Andy Ritger wrote:
> > > So your problem is simply that your non-GPL module can't link anymore
> > > with -RT. Would it help you if I simply replace the export for
> > > mutex_destroy with EXPORT_SYMBOL and leave it the function as is?
> >
> > Yes, definitely.
>
> So this is what I intend to add to the RT patch and I hope Ingo won't
> object:
>
> Alex Goins reported that mutex_destroy() on RT will force a GPL only symbol
> which won't link and therefore fail on a non-GPL kernel module.
> This does not happen on !RT and is a regression on RT which we would like to
> avoid.
> I try here the easy thing and to not use rt_mutex_destroy() if
> CONFIG_DEBUG_MUTEXES is not enabled. This will still break for the DEBUG
> configs so instead of adding a wrapper around rt_mutex_destroy() (which we
> have
> for rt_mutex_lock() for instance) I am simply dropping the GPL part from the
> export.
Is the
WARN_ON(rt_mutex_is_locked(lock));
in rt_mutex_destroy() valuable in non-CONFIG_DEBUG_MUTEXES kernels,
such that it would be better to always call it, and not noop away
mutex_destroy()
non-CONFIG_DEBUG_MUTEXES kernels? I thought that was your objection to
Alex's original patch.
But, with or without the noop-mutex_destroy diff hunk,
Reviewed-by: Andy Ritger
Thanks,
- Andy
> Reported-by: Alex Goins
> Signed-off-by: Sebastian Andrzej Siewior
> ---
> include/linux/mutex_rt.h |5 +
> kernel/locking/rtmutex.c |3 +--
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> --- a/include/linux/mutex_rt.h
> +++ b/include/linux/mutex_rt.h
> @@ -43,7 +43,12 @@ extern void __lockfunc _mutex_unlock(str
> #define mutex_lock_killable(l) _mutex_lock_killable(l)
> #define mutex_trylock(l) _mutex_trylock(l)
> #define mutex_unlock(l) _mutex_unlock(l)
> +
> +#ifdef CONFIG_DEBUG_MUTEXES
> #define mutex_destroy(l) rt_mutex_destroy(&(l)->lock)
> +#else
> +static inline void mutex_destroy(struct mutex *lock) {}
> +#endif
>
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> # define mutex_lock_nested(l, s) _mutex_lock_nested(l, s)
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -2027,8 +2027,7 @@ void rt_mutex_destroy(struct rt_mutex *l
> lock->magic = NULL;
> #endif
> }
> -
> -EXPORT_SYMBOL_GPL(rt_mutex_destroy);
> +EXPORT_SYMBOL(rt_mutex_destroy);
>
> /**
> * __rt_mutex_init - initialize the rt lock
>
> Sebastian
Re: [PATCH RT] Align rt_mutex inlining with upstream behavior
On Fri, Feb 03, 2017 at 04:54:34PM +0100, Sebastian Andrzej Siewior wrote: > On 2017-01-30 09:35:34 [-0800], Andy Ritger wrote: > > The problem is that various static inline functions such as > > reservation_object_fini() indirectly call mutex_destroy. On DEBUG_MUTEX > > kernels, mutex_destroy is EXPORT_SYMBOL_GPL. > > So your problem is simply that your non-GPL module can't link anymore > with -RT. Would it help you if I simply replace the export for > mutex_destroy with EXPORT_SYMBOL and leave it the function as is? Yes, definitely. Thanks, - Andy
Re: [PATCH RT] Align rt_mutex inlining with upstream behavior
On 2017-01-30 09:35:34 [-0800], Andy Ritger wrote: > The problem is that various static inline functions such as > reservation_object_fini() indirectly call mutex_destroy. On DEBUG_MUTEX > kernels, mutex_destroy is EXPORT_SYMBOL_GPL. So your problem is simply that your non-GPL module can't link anymore with -RT. Would it help you if I simply replace the export for mutex_destroy with EXPORT_SYMBOL and leave it the function as is? Sebastian
Re: [PATCH RT] Align rt_mutex inlining with upstream behavior
On Thu, Jan 26, 2017 at 06:01:09PM +0100, Sebastian Andrzej Siewior wrote: > On 2017-01-24 18:45:50 [-0800], Alex Goins wrote: > > mutex_destroy is no-op inline when DEBUG_MUTEX is not enabled. The RT Linux > > patches replace mutex_destroy() with rt_mutex_destroy(). This patch aligns > > rt_mutex_destroy() with mutex_destroy() by using the same no-op inline > > technique. > > > > Signed-off-by: Alex Goins > > Reviewed-by: Andy Ritger > > So what is the problem? Why are we doing this? There is still a check to > see if the lock is in use which is also done for the case where > DEBUG_MUTEX is disabled. The problem is that various static inline functions such as reservation_object_fini() indirectly call mutex_destroy. On DEBUG_MUTEX kernels, mutex_destroy is EXPORT_SYMBOL_GPL. In upstream, non-DEBUG_MUTEX kernels define mutex_destroy to a noop. This gives users the option of disabling DEBUG_MUTEX if they want to use non-GPL, reservation_object_fini()-using, kernel modules. In PREEMPTRT, non-DEBUG_MUTEX kernels export rt_mutex_destroy as EXPORT_SYMBOL_GPL, so users no longer have the work around of using DEBUG_MUTEX. This patch gives PREEMPTRT users the option of disabling DEBUG_MUTEX if they want to use such kernel modules, matching upstream behavior.
Re: [PATCH RT] Align rt_mutex inlining with upstream behavior
On 2017-01-24 18:45:50 [-0800], Alex Goins wrote: > mutex_destroy is no-op inline when DEBUG_MUTEX is not enabled. The RT Linux > patches replace mutex_destroy() with rt_mutex_destroy(). This patch aligns > rt_mutex_destroy() with mutex_destroy() by using the same no-op inline > technique. > > Signed-off-by: Alex Goins > Reviewed-by: Andy Ritger So what is the problem? Why are we doing this? There is still a check to see if the lock is in use which is also done for the case where DEBUG_MUTEX is disabled. Sebastian
[PATCH RT] Align rt_mutex inlining with upstream behavior
mutex_destroy is no-op inline when DEBUG_MUTEX is not enabled. The RT Linux
patches replace mutex_destroy() with rt_mutex_destroy(). This patch aligns
rt_mutex_destroy() with mutex_destroy() by using the same no-op inline
technique.
Signed-off-by: Alex Goins
Reviewed-by: Andy Ritger
---
include/linux/rtmutex.h | 7 ++-
kernel/locking/rtmutex.c | 5 ++---
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/linux/rtmutex.h b/include/linux/rtmutex.h
index 1abba5c..741e844 100644
--- a/include/linux/rtmutex.h
+++ b/include/linux/rtmutex.h
@@ -56,6 +56,12 @@ struct rt_mutex {
#endif
#ifdef CONFIG_DEBUG_RT_MUTEXES
+ extern void rt_mutex_destroy(struct rt_mutex *lock);
+#else
+ static inline void rt_mutex_destroy(struct rt_mutex *lock) {}
+#endif
+
+#ifdef CONFIG_DEBUG_RT_MUTEXES
# define __DEBUG_RT_MUTEX_INITIALIZER(mutexname) \
, .name = #mutexname, .file = __FILE__, .line = __LINE__
# define rt_mutex_init(mutex) __rt_mutex_init(mutex, __func__)
@@ -87,7 +93,6 @@ static inline int rt_mutex_is_locked(struct rt_mutex *lock)
}
extern void __rt_mutex_init(struct rt_mutex *lock, const char *name);
-extern void rt_mutex_destroy(struct rt_mutex *lock);
extern void rt_mutex_lock(struct rt_mutex *lock);
extern int rt_mutex_lock_interruptible(struct rt_mutex *lock);
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 2c49d76..8ff12fb 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1577,6 +1577,7 @@ bool __sched rt_mutex_futex_unlock(struct rt_mutex *lock,
return rt_mutex_slowunlock(lock, wqh);
}
+#ifdef CONFIG_DEBUG_RT_MUTEXES
/**
* rt_mutex_destroy - mark a mutex unusable
* @lock: the mutex to be destroyed
@@ -1588,12 +1589,10 @@ bool __sched rt_mutex_futex_unlock(struct rt_mutex
*lock,
void rt_mutex_destroy(struct rt_mutex *lock)
{
WARN_ON(rt_mutex_is_locked(lock));
-#ifdef CONFIG_DEBUG_RT_MUTEXES
lock->magic = NULL;
-#endif
}
-
EXPORT_SYMBOL_GPL(rt_mutex_destroy);
+#endif
/**
* __rt_mutex_init - initialize the rt lock
--
1.9.1
