[patch 1/2] lockdep: remove duplicate CONFIG_DEBUG_LOCKDEP definitions

[patch 1/2] lockdep: remove duplicate CONFIG_DEBUG_LOCKDEP definitions

[David Rientjes]

The atomic debug modifiers are already defined in
kernel/lockdep_internals.h.

Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 kernel/lockdep.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/kernel/lockdep.c b/kernel/lockdep.c
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -430,13 +430,6 @@ atomic_t nr_find_usage_forwards_checks;
 atomic_t nr_find_usage_forwards_recursions;
 atomic_t nr_find_usage_backwards_checks;
 atomic_t nr_find_usage_backwards_recursions;
-# define debug_atomic_inc(ptr)		atomic_inc(ptr)
-# define debug_atomic_dec(ptr)		atomic_dec(ptr)
-# define debug_atomic_read(ptr)		atomic_read(ptr)
-#else
-# define debug_atomic_inc(ptr)		do { } while (0)
-# define debug_atomic_dec(ptr)		do { } while (0)
-# define debug_atomic_read(ptr)		0
 #endif
 
 /*

[patch 2/2] lockdep: initialize lockdep debugging statistics

[David Rientjes]

The CONFIG_DEBUG_LOCKDEP statistics need to be initialized with
ATOMIC_INIT(0) since they are of type atomic_t.

Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 kernel/lockdep.c |   34 +++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/kernel/lockdep.c b/kernel/lockdep.c
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -413,23 +413,23 @@ static struct stack_trace lockdep_init_trace = {
 /*
  * Various lockdep statistics:
  */
-atomic_t chain_lookup_hits;
-atomic_t chain_lookup_misses;
-atomic_t hardirqs_on_events;
-atomic_t hardirqs_off_events;
-atomic_t redundant_hardirqs_on;
-atomic_t redundant_hardirqs_off;
-atomic_t softirqs_on_events;
-atomic_t softirqs_off_events;
-atomic_t redundant_softirqs_on;
-atomic_t redundant_softirqs_off;
-atomic_t nr_unused_locks;
-atomic_t nr_cyclic_checks;
-atomic_t nr_cyclic_check_recursions;
-atomic_t nr_find_usage_forwards_checks;
-atomic_t nr_find_usage_forwards_recursions;
-atomic_t nr_find_usage_backwards_checks;
-atomic_t nr_find_usage_backwards_recursions;
+atomic_t chain_lookup_hits = ATOMIC_INIT(0);
+atomic_t chain_lookup_misses = ATOMIC_INIT(0);
+atomic_t hardirqs_on_events = ATOMIC_INIT(0);
+atomic_t hardirqs_off_events = ATOMIC_INIT(0);
+atomic_t redundant_hardirqs_on = ATOMIC_INIT(0);
+atomic_t redundant_hardirqs_off = ATOMIC_INIT(0);
+atomic_t softirqs_on_events = ATOMIC_INIT(0);
+atomic_t softirqs_off_events = ATOMIC_INIT(0);
+atomic_t redundant_softirqs_on = ATOMIC_INIT(0);
+atomic_t redundant_softirqs_off = ATOMIC_INIT(0);
+atomic_t nr_unused_locks = ATOMIC_INIT(0);
+atomic_t nr_cyclic_checks = ATOMIC_INIT(0);
+atomic_t nr_cyclic_check_recursions = ATOMIC_INIT(0);
+atomic_t nr_find_usage_forwards_checks = ATOMIC_INIT(0);
+atomic_t nr_find_usage_forwards_recursions = ATOMIC_INIT(0);
+atomic_t nr_find_usage_backwards_checks = ATOMIC_INIT(0);
+atomic_t nr_find_usage_backwards_recursions = ATOMIC_INIT(0);
 #endif
 
 /*

[tip:core/locking] lockdep: remove duplicate CONFIG_DEBUG_LOCKDEP definitions

[David Rientjes]

Commit-ID:  03d78913f01e8f6599823f00357ed17b32747d3d
Gitweb:     http://git.kernel.org/tip/03d78913f01e8f6599823f00357ed17b32747d3d
Author:     "David Rientjes" <rientjes@google.com>
AuthorDate: Thu, 5 Mar 2009 02:29:05 -0800
Commit:     Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 5 Mar 2009 11:46:00 +0100

lockdep: remove duplicate CONFIG_DEBUG_LOCKDEP definitions

Impact: cleanup

The atomic debug modifiers are already defined in
kernel/lockdep_internals.h.

Signed-off-by: David Rientjes <rientjes@google.com>
Acked-by: Peter Zijlstra <peterz@infradead.org>
LKML-Reference: <alpine.DEB.2.00.0903050222160.30401@chino.kir.corp.google.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/lockdep.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 02014f7..9a1e2bc 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -433,13 +433,6 @@ atomic_t nr_find_usage_forwards_checks;
 atomic_t nr_find_usage_forwards_recursions;
 atomic_t nr_find_usage_backwards_checks;
 atomic_t nr_find_usage_backwards_recursions;
-# define debug_atomic_inc(ptr)		atomic_inc(ptr)
-# define debug_atomic_dec(ptr)		atomic_dec(ptr)
-# define debug_atomic_read(ptr)		atomic_read(ptr)
-#else
-# define debug_atomic_inc(ptr)		do { } while (0)
-# define debug_atomic_dec(ptr)		do { } while (0)
-# define debug_atomic_read(ptr)		0
 #endif
 
 /*

[tip:core/locking] lockdep: initialize lockdep debugging statistics

[David Rientjes]

Commit-ID:  742bbc4022d117bccf9c8a86c8e500dfa97e2f70
Gitweb:     http://git.kernel.org/tip/742bbc4022d117bccf9c8a86c8e500dfa97e2f70
Author:     "David Rientjes" <rientjes@google.com>
AuthorDate: Thu, 5 Mar 2009 02:29:06 -0800
Commit:     Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 5 Mar 2009 11:46:00 +0100

lockdep: initialize lockdep debugging statistics

Impact: cleanup

The CONFIG_DEBUG_LOCKDEP statistics need to be initialized with
ATOMIC_INIT(0) since they are of type atomic_t.

Signed-off-by: David Rientjes <rientjes@google.com>
Acked-by: Peter Zijlstra <peterz@infradead.org>
LKML-Reference: <alpine.DEB.2.00.0903050223240.30401@chino.kir.corp.google.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 kernel/lockdep.c |   34 +++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 9a1e2bc..3f7ff12 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -416,23 +416,23 @@ static struct stack_trace lockdep_init_trace = {
 /*
  * Various lockdep statistics:
  */
-atomic_t chain_lookup_hits;
-atomic_t chain_lookup_misses;
-atomic_t hardirqs_on_events;
-atomic_t hardirqs_off_events;
-atomic_t redundant_hardirqs_on;
-atomic_t redundant_hardirqs_off;
-atomic_t softirqs_on_events;
-atomic_t softirqs_off_events;
-atomic_t redundant_softirqs_on;
-atomic_t redundant_softirqs_off;
-atomic_t nr_unused_locks;
-atomic_t nr_cyclic_checks;
-atomic_t nr_cyclic_check_recursions;
-atomic_t nr_find_usage_forwards_checks;
-atomic_t nr_find_usage_forwards_recursions;
-atomic_t nr_find_usage_backwards_checks;
-atomic_t nr_find_usage_backwards_recursions;
+atomic_t chain_lookup_hits = ATOMIC_INIT(0);
+atomic_t chain_lookup_misses = ATOMIC_INIT(0);
+atomic_t hardirqs_on_events = ATOMIC_INIT(0);
+atomic_t hardirqs_off_events = ATOMIC_INIT(0);
+atomic_t redundant_hardirqs_on = ATOMIC_INIT(0);
+atomic_t redundant_hardirqs_off = ATOMIC_INIT(0);
+atomic_t softirqs_on_events = ATOMIC_INIT(0);
+atomic_t softirqs_off_events = ATOMIC_INIT(0);
+atomic_t redundant_softirqs_on = ATOMIC_INIT(0);
+atomic_t redundant_softirqs_off = ATOMIC_INIT(0);
+atomic_t nr_unused_locks = ATOMIC_INIT(0);
+atomic_t nr_cyclic_checks = ATOMIC_INIT(0);
+atomic_t nr_cyclic_check_recursions = ATOMIC_INIT(0);
+atomic_t nr_find_usage_forwards_checks = ATOMIC_INIT(0);
+atomic_t nr_find_usage_forwards_recursions = ATOMIC_INIT(0);
+atomic_t nr_find_usage_backwards_checks = ATOMIC_INIT(0);
+atomic_t nr_find_usage_backwards_recursions = ATOMIC_INIT(0);
 #endif
 
 /*

Re: [patch 2/2] lockdep: initialize lockdep debugging statistics

[Peter Zijlstra]

On Thu, 2009-03-05 at 02:29 -0800, David Rientjes wrote:
> The CONFIG_DEBUG_LOCKDEP statistics need to be initialized with
> ATOMIC_INIT(0) since they are of type atomic_t.

And how does ATOMIC_INIT(0) differ from the global/static storage being
initialized to 0?

All I can see this doing it moving the variables from bss to data for
some compilers.

Re: [patch 2/2] lockdep: initialize lockdep debugging statistics

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, 2009-03-05 at 02:29 -0800, David Rientjes wrote:
> > The CONFIG_DEBUG_LOCKDEP statistics need to be initialized with
> > ATOMIC_INIT(0) since they are of type atomic_t.
> 
> And how does ATOMIC_INIT(0) differ from the global/static 
> storage being initialized to 0?
> 
> All I can see this doing it moving the variables from bss to 
> data for some compilers.

ok, i dropped it for now.

	Ingo

Re: [patch 2/2] lockdep: initialize lockdep debugging statistics

[David Rientjes]

On Thu, 5 Mar 2009, Ingo Molnar wrote:

> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Thu, 2009-03-05 at 02:29 -0800, David Rientjes wrote:
> > > The CONFIG_DEBUG_LOCKDEP statistics need to be initialized with
> > > ATOMIC_INIT(0) since they are of type atomic_t.
> > 
> > And how does ATOMIC_INIT(0) differ from the global/static 
> > storage being initialized to 0?
> > 
> > All I can see this doing it moving the variables from bss to 
> > data for some compilers.
> 

It's actually the opposite for gcc 4.1.1, the debugging symbols are now in 
bss.

> ok, i dropped it for now.
> 

It shouldn't be dropped, the initialization is necessary because we can't 
rely on atomic_t's implementation.

Re: [patch 2/2] lockdep: initialize lockdep debugging statistics

[Peter Zijlstra]

On Thu, 2009-03-05 at 13:57 -0800, David Rientjes wrote:
> It shouldn't be dropped, the initialization is necessary because we can't 
> rely on atomic_t's implementation.

I'm a bit slow, please use more words and explain this to me.

Before replying please read:

ea435467500612636f8f4fb639ff6e76b2496e4b

and stare at the output of:

git grep "define[ \t]*ATOMIC_INIT\>"

Re: [patch 2/2] lockdep: initialize lockdep debugging statistics

[David Rientjes]

On Fri, 6 Mar 2009, Peter Zijlstra wrote:

> I'm a bit slow, please use more words and explain this to me.
> 
> Before replying please read:
> 
> ea435467500612636f8f4fb639ff6e76b2496e4b
> 
> and stare at the output of:
> 
> git grep "define[ \t]*ATOMIC_INIT\>"
> 

Peter, this is purely a matter of good software engineering practices.  I 
hadn't realized you were so passionate about avoiding initialization of 
global atomic_t variables, even though it usually suffices.

I assume you wouldn't object to removing all such cases in the kernel.

	$ grep -r "atomic_t.*= ATOMIC_INIT(0)" * | wc -l
	104

Unless David Miller has any input, I'll happily defer to your 
maintainership of lockdep in this matter.  Thanks for looking at the 
patch.

Re: [patch 2/2] lockdep: initialize lockdep debugging statistics

[Li Zefan]

Peter Zijlstra wrote:
> On Thu, 2009-03-05 at 13:57 -0800, David Rientjes wrote:
>> It shouldn't be dropped, the initialization is necessary because we can't 
>> rely on atomic_t's implementation.
> 
> I'm a bit slow, please use more words and explain this to me.
> 

I once was also (and is still not sure) wondering if it's better to use
INIT_HLIST_HEAD() for initialization for the reason David is claiming..

The situation is similar with this ATOMIC_INIT(), that INIT_HLIST_HEAD()
just set 2 pointers to 0, and not every user of hlist calls this INIT()
at initialization phase.

But I'll definitely on David's side if it's not kernel project.

> Before replying please read:
> 
> ea435467500612636f8f4fb639ff6e76b2496e4b
> 
> and stare at the output of:
> 
> git grep "define[ \t]*ATOMIC_INIT\>"
> 

Re: [patch 2/2] lockdep: initialize lockdep debugging statistics

[Peter Zijlstra]

On Fri, 2009-03-06 at 01:56 -0800, David Rientjes wrote:
> 
> Peter, this is purely a matter of good software engineering practices. 

For complex types I would agree, but atomic_t is assumed to be just
another int - just with special ops. For such things we can hold that
memset('0') will properly initialize them to their 0 value.

>  I 
> hadn't realized you were so passionate about avoiding initialization of 
> global atomic_t variables, 

Given that static/global storage is properly initialized to 0, and the
above, it seems to be redundant.

> I assume you wouldn't object to removing all such cases in the kernel.
> 
>         $ grep -r "atomic_t.*= ATOMIC_INIT(0)" * | wc -l
>         104

Only IFF all those cases are for static/global storage. Otherwise you
have to ensure 0-ness by either using __GFP_ZERO or explicit
initialization.

Re: [patch 2/2] lockdep: initialize lockdep debugging statistics

[David Rientjes]

On Fri, 6 Mar 2009, Peter Zijlstra wrote:

> > Peter, this is purely a matter of good software engineering practices. 
> 
> For complex types I would agree, but atomic_t is assumed to be just
> another int - just with special ops. For such things we can hold that
> memset('0') will properly initialize them to their 0 value.
> 

That's the point about good software engineering practices: atomic_t 
should not be assumed to be another int.  If its implementation were ever 
to change even for a single architecture, so would these variable 
declarations.

> > I assume you wouldn't object to removing all such cases in the kernel.
> > 
> >         $ grep -r "atomic_t.*= ATOMIC_INIT(0)" * | wc -l
> >         104
> 
> Only IFF all those cases are for static/global storage.

They are.

Anyway, I've already deferred to your maintainership of lockdep about not 
wanting to initialize global atomics in this part of the kernel, so while 
I disagree with it from a software engineering perspective, I think I'll 
be able to move on.

Re: [patch 2/2] lockdep: initialize lockdep debugging statistics

[Peter Zijlstra]

On Fri, 2009-03-06 at 02:27 -0800, David Rientjes wrote:
> That's the point about good software engineering practices: atomic_t 
> should not be assumed to be another int. 

We do, and its a rather fundamental assumption. Any implementation not
conforming is deemed buggy -- esp on the 0 property.

http://lkml.org/lkml/2008/10/28/216