Re: [PATCH] sparse fix: add many lock annotations

[Ira Snyder]

On Wed, 22 Nov 2006 21:33:07 +0300
Alexey Dobriyan <adobriyan@xxxxxxxxx> wrote:

> On Wed, Nov 22, 2006 at 12:11:46AM -0800, Ira Snyder wrote:
> > This patch adds many lock annotations to the kernel source to quiet
> > warnings from sparse. In almost every case, it quiets the warning caused
> > by locks that are intentionally grabbed in one function and released in
> > another.
> >
> > In the other cases, __acquire() and __release() are used to make sparse
> > believe that a lock was grabbed (even though it was not), in order to
> > make all exit points have equal lock counts. These follow the style in
> > kernel/sched.c.
> 
> > --- a/arch/i386/kernel/smp.c
> > +++ b/arch/i386/kernel/smp.c
> > @@ -507,11 +507,13 @@ struct call_data_struct {
> >  };
> >
> >  void lock_ipi_call_lock(void)
> > +__acquires(call_lock)
> >  {
> >  	spin_lock_irq(&call_lock);
> >  }
> >
> >  void unlock_ipi_call_lock(void)
> > +__releases(call_lock)
> >  {
> >  	spin_unlock_irq(&call_lock);
> >  }
> 
> Wrong place. Prototypes should be marked instead. How else would you
> know about:
> 
> 	lock_ipi_call_lock();
> 	if (foo)
> 		return -E;
> 	lock_ipi_call_lock();
> 
> on another compilation unit?
> 

Are you saying I should use something like this instead?:
diff --git a/include/asm-i386/smp.h b/include/asm-i386/smp.h
index bd59c15..9489602 100644
--- a/include/asm-i386/smp.h
+++ b/include/asm-i386/smp.h
@@ -38,8 +38,8 @@ extern cpumask_t cpu_core_map[];
 
 extern void (*mtrr_hook) (void);
 extern void zap_low_mappings (void);
-extern void lock_ipi_call_lock(void);
-extern void unlock_ipi_call_lock(void);
+extern void lock_ipi_call_lock(void) __acquires(call_lock);
+extern void unlock_ipi_call_lock(void) __releases(call_lock);
 
 #define MAX_APICID 256
 extern u8 x86_cpu_to_apicid[];

If so, it doesn't remove the warning from sparse, it still shows:
  CHECK
arch/i386/kernel/smp.c arch/i386/kernel/smp.c:509:6: warning: context
imbalance in 'lock_ipi_call_lock' - wrong count at exit
arch/i386/kernel/smp.c:514:6: warning: context imbalance in
'unlock_ipi_call_lock' - unexpected unlock 

Those go away with the original patch.

I was following some examples currently in the source, such as in
arch/i386/kernel/efi.c on function efi_call_phys_prelog(). Or
expand_fdtable() in fs/file.c is another good example.

> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -1004,6 +1004,7 @@ EXPORT_SYMBOL(max_cstate);
> >   */
> >
> >  acpi_cpu_flags acpi_os_acquire_lock(acpi_spinlock lockp)
> > +__acquires(lockp)
> >  {
> >  	acpi_cpu_flags flags;
> >  	spin_lock_irqsave(lockp, flags);
> > @@ -1015,6 +1016,7 @@ acpi_cpu_flags acpi_os_acquire_lock(acpi
> >   */
> >
> >  void acpi_os_release_lock(acpi_spinlock lockp, acpi_cpu_flags flags)
> > +__releases(lockp)
> >  {
> >  	spin_unlock_irqrestore(lockp, flags);
> >  }
> 
> Again, wrong. IMO, sparse should deduce itself that lock is grabbed in such
> trivial cases.
> 

I'm not sure that it can. See: http://lwn.net/Articles/109066/ where
Linus talks about addings __acquires(lockname) and __releases(lockname)
to functions whose purpose is to grab and hold a lock at exit.

Anyway, this is my first patch on the LKML, so I'd like to try and get
it right.

Thanks,
Ira
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/