1. TIME subsytem : patch 1, an __acquires(timer) annotation is added. as
the function despite having a nested lock the outer one allows entry to
critical section only.
2. Within futex.c file or path 2, a __releases() annotation is added. as the
function releases the lock at exit.
3. MUTEX subsystem : patch 3, __acquires(lock) and __releases(lock) are
added to mutex_lock() and mutex_unlock() to fix issues raised in other
files.
Jules Irenge (3):
time: Add missing annotation to lock_hrtimer_base()
futex: Add missing annotation for wake_futex_pi()
mutex: Add missing annotations
include/linux/mutex.h | 4 ++--
kernel/futex.c | 1 +
kernel/time/hrtimer.c | 1 +
3 files changed, 4 insertions(+), 2 deletions(-)
--
2.24.1
Sparse reports a warning at wake_futex_pi()
|warning: context imbalance in wake_futex_pi() - unexpected unlock
To fix this,
a __releases(&pi_state->pi_mutex.wait_lock) annotation is added
Given that wake_futex_pi() does actually call
raw_spin_unlock_irq(&pi_state->pi_mutex.wait_lock)
This not only fixes the warning
but also improves on the readability of the code.
Signed-off-by: Jules Irenge <[email protected]>
---
kernel/futex.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/futex.c b/kernel/futex.c
index 03c518e9747e..8bc288a7187f 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -1549,6 +1549,7 @@ static void mark_wake_futex(struct wake_q_head *wake_q, struct futex_q *q)
* Caller must hold a reference on @pi_state.
*/
static int wake_futex_pi(u32 __user *uaddr, u32 uval, struct futex_pi_state *pi_state)
+ __releases(&pi_state->pi_mutex.wait_lock)
{
u32 uninitialized_var(curval), newval;
struct task_struct *new_owner;
--
2.24.1
Sparse reports a warning at lock_hrtimer_base()
|warning: context imbalance in lock_hrtimer_base() - wrong count at exit
|warning: context imbalance in hrtimer_start_range_ns() - unexpected unlock
|warning: context imbalance in hrtimer_try_to_cancel() - unexpected unlock
|warning: context imbalance in __hrtimer_get_remaining()
|- unexpected unlock
To fix this , an __acquires(timer) annotation is added.
Given that lock_hrtimer_base() does actually call READ_ONCE(timer->base).
This not only fixes the warnings
but also improves on readability of the code.
Signed-off-by: Jules Irenge <[email protected]>
---
kernel/time/hrtimer.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 8de90ea31280..8f555b49395a 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -160,6 +160,7 @@ static inline bool is_migration_base(struct hrtimer_clock_base *base)
static
struct hrtimer_clock_base *lock_hrtimer_base(const struct hrtimer *timer,
unsigned long *flags)
+ __acquires(timer)
{
struct hrtimer_clock_base *base;
--
2.24.1
Jules,
Jules Irenge <[email protected]> writes:
Please use the proper subsystem prefixes when sending patches.
git log --oneline path/to/file
gives you usally a pretty good hint.
> Sparse reports a warning at lock_hrtimer_base()
>
> |warning: context imbalance in lock_hrtimer_base() - wrong count at exit
This leading '|' is pointless
> |warning: context imbalance in hrtimer_start_range_ns() - unexpected unlock
> |warning: context imbalance in hrtimer_try_to_cancel() - unexpected unlock
> |warning: context imbalance in __hrtimer_get_remaining()
> |- unexpected unlock
How are the last 3 related to:
> Sparse reports a warning at lock_hrtimer_base()
?
> To fix this , an __acquires(timer) annotation is added.
Add the missing __acquires(timer) annotation.
Is precise and follows the recommendations of Documentation/process/...
> Given that lock_hrtimer_base() does actually call READ_ONCE(timer->base).
Given that the above sentence uses 'Given that' it should not terminate
right after explaining the 'Given'.
> This not only fixes the warnings but also improves on readability of
> the code.
I tend to disagree. In fact the annotation disturbes the reading flow
because it's on a separate line.
Can you please stop using this boilerplate which is neither helping
review nor giving someone who looks at the commit later on any useful
information?
Here is a suggestion for a change log for this:
Sparse reports several warnings;
warning: context imbalance in lock_hrtimer_base() - wrong count at exit
warning: context imbalance in hrtimer_start_range_ns() - unexpected unlock
warning: context imbalance in hrtimer_try_to_cancel() - unexpected unlock
warning: context imbalance in __hrtimer_get_remaining()- unexpected unlock
The root cause is a missing annotation of lock_hrtimer_base() which
causes also the 'unexpected unlock' warnings.
Add the missing __acquires(timer) annotation.
Hmm?
The other 2 patches of this series have similar issues. The futex
changelog is also horribly formatted.
Thanks,
tglx
Thanks Thomas. I really appreciate your feedback, I take good note and I
will send a different version with all the changes in reference to the
email.
Thanks again,
Kind regards
Jules
On Fri, 24 Jan 2020, Thomas Gleixner wrote:
> Jules,
>
> Jules Irenge <[email protected]> writes:
>
> Please use the proper subsystem prefixes when sending patches.
>
> git log --oneline path/to/file
>
> gives you usally a pretty good hint.
>
> > Sparse reports a warning at lock_hrtimer_base()
> >
> > |warning: context imbalance in lock_hrtimer_base() - wrong count at exit
>
> This leading '|' is pointless
>
> > |warning: context imbalance in hrtimer_start_range_ns() - unexpected unlock
> > |warning: context imbalance in hrtimer_try_to_cancel() - unexpected unlock
> > |warning: context imbalance in __hrtimer_get_remaining()
> > |- unexpected unlock
>
> How are the last 3 related to:
>
> > Sparse reports a warning at lock_hrtimer_base()
>
> ?
>
> > To fix this , an __acquires(timer) annotation is added.
>
> Add the missing __acquires(timer) annotation.
>
> Is precise and follows the recommendations of Documentation/process/...
>
> > Given that lock_hrtimer_base() does actually call READ_ONCE(timer->base).
>
> Given that the above sentence uses 'Given that' it should not terminate
> right after explaining the 'Given'.
>
> > This not only fixes the warnings but also improves on readability of
> > the code.
>
> I tend to disagree. In fact the annotation disturbes the reading flow
> because it's on a separate line.
>
> Can you please stop using this boilerplate which is neither helping
> review nor giving someone who looks at the commit later on any useful
> information?
>
> Here is a suggestion for a change log for this:
>
> Sparse reports several warnings;
> warning: context imbalance in lock_hrtimer_base() - wrong count at exit
> warning: context imbalance in hrtimer_start_range_ns() - unexpected unlock
> warning: context imbalance in hrtimer_try_to_cancel() - unexpected unlock
> warning: context imbalance in __hrtimer_get_remaining()- unexpected unlock
>
> The root cause is a missing annotation of lock_hrtimer_base() which
> causes also the 'unexpected unlock' warnings.
>
> Add the missing __acquires(timer) annotation.
>
> Hmm?
>
> The other 2 patches of this series have similar issues. The futex
> changelog is also horribly formatted.
>
> Thanks,
>
> tglx
>