Messages in this thread Patch in this message | | | Date | Wed, 22 Nov 2006 11:34:08 -0800 | From | Ira Snyder <> | Subject | Re: [PATCH] sparse fix: add many lock annotations |
| |
On Wed, 22 Nov 2006 21:33:07 +0300 Alexey Dobriyan <adobriyan@gmail.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |