Re: [PATCH 2/2] lockdep reentrancy

[Mathieu Desnoyers]

Hi Andrew,

* Andrew Morton ([EMAIL PROTECTED]) wrote:
> On Tue, 16 Jan 2007 12:56:31 -0500
> Mathieu Desnoyers <[EMAIL PROTECTED]> wrote:
> 
> > Here is a patch to lockdep.c so it behaves correctly when a kprobe 
> > breakpoint is
> > put on a marker within hardirq tracing functions as long as the marker is 
> > within
> > the lockdep_recursion incremented boundaries. It should apply on 
> > 2.6.20-rc4-git3.
> > 
> > Mathieu
> > 
> > Signed-off-by: Mathieu Desnoyers <[EMAIL PROTECTED]>
> > 
> > 
> > @@ -1841,33 +1843,36 @@ void trace_hardirqs_on(void)
> 
> You lost the patch headers.
> 
> > struct task_struct *curr = current;
> > unsigned long ip;
> >  
> > if (unlikely(!debug_locks || current->lockdep_recursion))
> > return;
> >  
> > +   current->lockdep_recursion++;
> > +   barrier();
> 
> Why can't we use lockdep_off() here?
> 

Because I thought that changing lockdep_off() for the whole system might be a
little different than adding a lockdep recursion protection. See my other email
for the lockdep_on/off() discussion.

And please drop this patch : It sometimes make lockdep trigger false positives
under heavy IRQ load because of a dropped lockdep event.

Mathieu


> > if (DEBUG_LOCKS_WARN_ON(unlikely(!early_boot_irqs_enabled)))
> > -   return;
> > +   goto end;
> >  
> > if (unlikely(curr->hardirqs_enabled)) {
> > debug_atomic_inc(_hardirqs_on);
> > -   return;
> > +   goto end;
> > }
> > /* we'll do an OFF -> ON transition: */
> > curr->hardirqs_enabled = 1;
> > ip = (unsigned long) __builtin_return_address(0);
> >  
> > if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
> > -   return;
> > +   goto end;
> > if (DEBUG_LOCKS_WARN_ON(current->hardirq_context))
> > -   return;
> > +   goto end;
> > /*
> >  * We are going to turn hardirqs on, so set the
> >  * usage bit for all held locks:
> >  */
> > if (!mark_held_locks(curr, 1, ip))
> > -   return;
> > +   goto end;
> > /*
> >  * If we have softirqs enabled, then set the usage
> >  * bit for all held locks. (disabled hardirqs prevented
> > @@ -1875,11 +1880,14 @@ void trace_hardirqs_on(void)
> >  */
> > if (curr->softirqs_enabled)
> > if (!mark_held_locks(curr, 0, ip))
> > -   return;
> > +   goto end;
> >  
> > curr->hardirq_enable_ip = ip;
> > curr->hardirq_enable_event = ++curr->irq_events;
> > debug_atomic_inc(_on_events);
> > +end:
> > +   barrier();
> > +   current->lockdep_recursion--;
> 
> lockdep_on()?
> 
> >  }
> >  
> >  EXPORT_SYMBOL(trace_hardirqs_on);
> > @@ -1888,14 +1896,17 @@ void trace_hardirqs_off(void)
> >  {
> > struct task_struct *curr = current;
> >  
> > if (unlikely(!debug_locks || current->lockdep_recursion))
> > return;
> >  
> > +   current->lockdep_recursion++;
> > +   barrier();
> 
> lockdep_off()?
> 
> > if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
> > -   return;
> > +   goto end;
> >  
> > if (curr->hardirqs_enabled) {
> > /*
> > @@ -1910,6 +1921,9 @@ void trace_hardirqs_off(void)
> > debug_atomic_inc(_off_events);
> > } else
> > debug_atomic_inc(_hardirqs_off);
> > +end:
> > +   barrier();
> > +   current->lockdep_recursion--;
> 
> lockdep_on()?

-- 
OpenPGP public key:  http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] lockdep reentrancy

[Mathieu Desnoyers]

Hi Andrew,

* Andrew Morton ([EMAIL PROTECTED]) wrote:
 On Tue, 16 Jan 2007 12:56:31 -0500
 Mathieu Desnoyers [EMAIL PROTECTED] wrote:
 
  Here is a patch to lockdep.c so it behaves correctly when a kprobe 
  breakpoint is
  put on a marker within hardirq tracing functions as long as the marker is 
  within
  the lockdep_recursion incremented boundaries. It should apply on 
  2.6.20-rc4-git3.
  
  Mathieu
  
  Signed-off-by: Mathieu Desnoyers [EMAIL PROTECTED]
  
  
  @@ -1841,33 +1843,36 @@ void trace_hardirqs_on(void)
 
 You lost the patch headers.
 
  struct task_struct *curr = current;
  unsigned long ip;
   
  if (unlikely(!debug_locks || current-lockdep_recursion))
  return;
   
  +   current-lockdep_recursion++;
  +   barrier();
 
 Why can't we use lockdep_off() here?
 

Because I thought that changing lockdep_off() for the whole system might be a
little different than adding a lockdep recursion protection. See my other email
for the lockdep_on/off() discussion.

And please drop this patch : It sometimes make lockdep trigger false positives
under heavy IRQ load because of a dropped lockdep event.

Mathieu


  if (DEBUG_LOCKS_WARN_ON(unlikely(!early_boot_irqs_enabled)))
  -   return;
  +   goto end;
   
  if (unlikely(curr-hardirqs_enabled)) {
  debug_atomic_inc(redundant_hardirqs_on);
  -   return;
  +   goto end;
  }
  /* we'll do an OFF - ON transition: */
  curr-hardirqs_enabled = 1;
  ip = (unsigned long) __builtin_return_address(0);
   
  if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
  -   return;
  +   goto end;
  if (DEBUG_LOCKS_WARN_ON(current-hardirq_context))
  -   return;
  +   goto end;
  /*
   * We are going to turn hardirqs on, so set the
   * usage bit for all held locks:
   */
  if (!mark_held_locks(curr, 1, ip))
  -   return;
  +   goto end;
  /*
   * If we have softirqs enabled, then set the usage
   * bit for all held locks. (disabled hardirqs prevented
  @@ -1875,11 +1880,14 @@ void trace_hardirqs_on(void)
   */
  if (curr-softirqs_enabled)
  if (!mark_held_locks(curr, 0, ip))
  -   return;
  +   goto end;
   
  curr-hardirq_enable_ip = ip;
  curr-hardirq_enable_event = ++curr-irq_events;
  debug_atomic_inc(hardirqs_on_events);
  +end:
  +   barrier();
  +   current-lockdep_recursion--;
 
 lockdep_on()?
 
   }
   
   EXPORT_SYMBOL(trace_hardirqs_on);
  @@ -1888,14 +1896,17 @@ void trace_hardirqs_off(void)
   {
  struct task_struct *curr = current;
   
  if (unlikely(!debug_locks || current-lockdep_recursion))
  return;
   
  +   current-lockdep_recursion++;
  +   barrier();
 
 lockdep_off()?
 
  if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
  -   return;
  +   goto end;
   
  if (curr-hardirqs_enabled) {
  /*
  @@ -1910,6 +1921,9 @@ void trace_hardirqs_off(void)
  debug_atomic_inc(hardirqs_off_events);
  } else
  debug_atomic_inc(redundant_hardirqs_off);
  +end:
  +   barrier();
  +   current-lockdep_recursion--;
 
 lockdep_on()?

-- 
OpenPGP public key:  http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] lockdep reentrancy

[Andrew Morton]

On Tue, 16 Jan 2007 12:56:31 -0500
Mathieu Desnoyers <[EMAIL PROTECTED]> wrote:

> Here is a patch to lockdep.c so it behaves correctly when a kprobe breakpoint 
> is
> put on a marker within hardirq tracing functions as long as the marker is 
> within
> the lockdep_recursion incremented boundaries. It should apply on 
> 2.6.20-rc4-git3.
> 
> Mathieu
> 
> Signed-off-by: Mathieu Desnoyers <[EMAIL PROTECTED]>
> 
> 
> @@ -1841,33 +1843,36 @@ void trace_hardirqs_on(void)

You lost the patch headers.

>   struct task_struct *curr = current;
>   unsigned long ip;
>  
>   if (unlikely(!debug_locks || current->lockdep_recursion))
>   return;
>  
> + current->lockdep_recursion++;
> + barrier();

Why can't we use lockdep_off() here?

>   if (DEBUG_LOCKS_WARN_ON(unlikely(!early_boot_irqs_enabled)))
> - return;
> + goto end;
>  
>   if (unlikely(curr->hardirqs_enabled)) {
>   debug_atomic_inc(_hardirqs_on);
> - return;
> + goto end;
>   }
>   /* we'll do an OFF -> ON transition: */
>   curr->hardirqs_enabled = 1;
>   ip = (unsigned long) __builtin_return_address(0);
>  
>   if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
> - return;
> + goto end;
>   if (DEBUG_LOCKS_WARN_ON(current->hardirq_context))
> - return;
> + goto end;
>   /*
>* We are going to turn hardirqs on, so set the
>* usage bit for all held locks:
>*/
>   if (!mark_held_locks(curr, 1, ip))
> - return;
> + goto end;
>   /*
>* If we have softirqs enabled, then set the usage
>* bit for all held locks. (disabled hardirqs prevented
> @@ -1875,11 +1880,14 @@ void trace_hardirqs_on(void)
>*/
>   if (curr->softirqs_enabled)
>   if (!mark_held_locks(curr, 0, ip))
> - return;
> + goto end;
>  
>   curr->hardirq_enable_ip = ip;
>   curr->hardirq_enable_event = ++curr->irq_events;
>   debug_atomic_inc(_on_events);
> +end:
> + barrier();
> + current->lockdep_recursion--;

lockdep_on()?

>  }
>  
>  EXPORT_SYMBOL(trace_hardirqs_on);
> @@ -1888,14 +1896,17 @@ void trace_hardirqs_off(void)
>  {
>   struct task_struct *curr = current;
>  
>   if (unlikely(!debug_locks || current->lockdep_recursion))
>   return;
>  
> + current->lockdep_recursion++;
> + barrier();

lockdep_off()?

>   if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
> - return;
> + goto end;
>  
>   if (curr->hardirqs_enabled) {
>   /*
> @@ -1910,6 +1921,9 @@ void trace_hardirqs_off(void)
>   debug_atomic_inc(_off_events);
>   } else
>   debug_atomic_inc(_hardirqs_off);
> +end:
> + barrier();
> + current->lockdep_recursion--;

lockdep_on()?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] lockdep reentrancy

[Andrew Morton]

On Tue, 16 Jan 2007 12:56:31 -0500
Mathieu Desnoyers [EMAIL PROTECTED] wrote:

 Here is a patch to lockdep.c so it behaves correctly when a kprobe breakpoint 
 is
 put on a marker within hardirq tracing functions as long as the marker is 
 within
 the lockdep_recursion incremented boundaries. It should apply on 
 2.6.20-rc4-git3.
 
 Mathieu
 
 Signed-off-by: Mathieu Desnoyers [EMAIL PROTECTED]
 
 
 @@ -1841,33 +1843,36 @@ void trace_hardirqs_on(void)

You lost the patch headers.

   struct task_struct *curr = current;
   unsigned long ip;
  
   if (unlikely(!debug_locks || current-lockdep_recursion))
   return;
  
 + current-lockdep_recursion++;
 + barrier();

Why can't we use lockdep_off() here?

   if (DEBUG_LOCKS_WARN_ON(unlikely(!early_boot_irqs_enabled)))
 - return;
 + goto end;
  
   if (unlikely(curr-hardirqs_enabled)) {
   debug_atomic_inc(redundant_hardirqs_on);
 - return;
 + goto end;
   }
   /* we'll do an OFF - ON transition: */
   curr-hardirqs_enabled = 1;
   ip = (unsigned long) __builtin_return_address(0);
  
   if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
 - return;
 + goto end;
   if (DEBUG_LOCKS_WARN_ON(current-hardirq_context))
 - return;
 + goto end;
   /*
* We are going to turn hardirqs on, so set the
* usage bit for all held locks:
*/
   if (!mark_held_locks(curr, 1, ip))
 - return;
 + goto end;
   /*
* If we have softirqs enabled, then set the usage
* bit for all held locks. (disabled hardirqs prevented
 @@ -1875,11 +1880,14 @@ void trace_hardirqs_on(void)
*/
   if (curr-softirqs_enabled)
   if (!mark_held_locks(curr, 0, ip))
 - return;
 + goto end;
  
   curr-hardirq_enable_ip = ip;
   curr-hardirq_enable_event = ++curr-irq_events;
   debug_atomic_inc(hardirqs_on_events);
 +end:
 + barrier();
 + current-lockdep_recursion--;

lockdep_on()?

  }
  
  EXPORT_SYMBOL(trace_hardirqs_on);
 @@ -1888,14 +1896,17 @@ void trace_hardirqs_off(void)
  {
   struct task_struct *curr = current;
  
   if (unlikely(!debug_locks || current-lockdep_recursion))
   return;
  
 + current-lockdep_recursion++;
 + barrier();

lockdep_off()?

   if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
 - return;
 + goto end;
  
   if (curr-hardirqs_enabled) {
   /*
 @@ -1910,6 +1921,9 @@ void trace_hardirqs_off(void)
   debug_atomic_inc(hardirqs_off_events);
   } else
   debug_atomic_inc(redundant_hardirqs_off);
 +end:
 + barrier();
 + current-lockdep_recursion--;

lockdep_on()?
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] lockdep reentrancy

[Mathieu Desnoyers]

Here is a patch to lockdep.c so it behaves correctly when a kprobe breakpoint is
put on a marker within hardirq tracing functions as long as the marker is within
the lockdep_recursion incremented boundaries. It should apply on 
2.6.20-rc4-git3.

Mathieu

Signed-off-by: Mathieu Desnoyers <[EMAIL PROTECTED]>


@@ -1841,33 +1843,36 @@ void trace_hardirqs_on(void)
struct task_struct *curr = current;
unsigned long ip;
 
if (unlikely(!debug_locks || current->lockdep_recursion))
return;
 
+   current->lockdep_recursion++;
+   barrier();
+
if (DEBUG_LOCKS_WARN_ON(unlikely(!early_boot_irqs_enabled)))
-   return;
+   goto end;
 
if (unlikely(curr->hardirqs_enabled)) {
debug_atomic_inc(_hardirqs_on);
-   return;
+   goto end;
}
/* we'll do an OFF -> ON transition: */
curr->hardirqs_enabled = 1;
ip = (unsigned long) __builtin_return_address(0);
 
if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
-   return;
+   goto end;
if (DEBUG_LOCKS_WARN_ON(current->hardirq_context))
-   return;
+   goto end;
/*
 * We are going to turn hardirqs on, so set the
 * usage bit for all held locks:
 */
if (!mark_held_locks(curr, 1, ip))
-   return;
+   goto end;
/*
 * If we have softirqs enabled, then set the usage
 * bit for all held locks. (disabled hardirqs prevented
@@ -1875,11 +1880,14 @@ void trace_hardirqs_on(void)
 */
if (curr->softirqs_enabled)
if (!mark_held_locks(curr, 0, ip))
-   return;
+   goto end;
 
curr->hardirq_enable_ip = ip;
curr->hardirq_enable_event = ++curr->irq_events;
debug_atomic_inc(_on_events);
+end:
+   barrier();
+   current->lockdep_recursion--;
 }
 
 EXPORT_SYMBOL(trace_hardirqs_on);
@@ -1888,14 +1896,17 @@ void trace_hardirqs_off(void)
 {
struct task_struct *curr = current;
 
if (unlikely(!debug_locks || current->lockdep_recursion))
return;
 
+   current->lockdep_recursion++;
+   barrier();
+   
if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
-   return;
+   goto end;
 
if (curr->hardirqs_enabled) {
/*
@@ -1910,6 +1921,9 @@ void trace_hardirqs_off(void)
debug_atomic_inc(_off_events);
} else
debug_atomic_inc(_hardirqs_off);
+end:
+   barrier();
+   current->lockdep_recursion--;
 }
 
 EXPORT_SYMBOL(trace_hardirqs_off);

-- 
OpenPGP public key:  http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] lockdep reentrancy

[Mathieu Desnoyers]

Here is a patch to lockdep.c so it behaves correctly when a kprobe breakpoint is
put on a marker within hardirq tracing functions as long as the marker is within
the lockdep_recursion incremented boundaries. It should apply on 
2.6.20-rc4-git3.

Mathieu

Signed-off-by: Mathieu Desnoyers [EMAIL PROTECTED]


@@ -1841,33 +1843,36 @@ void trace_hardirqs_on(void)
struct task_struct *curr = current;
unsigned long ip;
 
if (unlikely(!debug_locks || current-lockdep_recursion))
return;
 
+   current-lockdep_recursion++;
+   barrier();
+
if (DEBUG_LOCKS_WARN_ON(unlikely(!early_boot_irqs_enabled)))
-   return;
+   goto end;
 
if (unlikely(curr-hardirqs_enabled)) {
debug_atomic_inc(redundant_hardirqs_on);
-   return;
+   goto end;
}
/* we'll do an OFF - ON transition: */
curr-hardirqs_enabled = 1;
ip = (unsigned long) __builtin_return_address(0);
 
if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
-   return;
+   goto end;
if (DEBUG_LOCKS_WARN_ON(current-hardirq_context))
-   return;
+   goto end;
/*
 * We are going to turn hardirqs on, so set the
 * usage bit for all held locks:
 */
if (!mark_held_locks(curr, 1, ip))
-   return;
+   goto end;
/*
 * If we have softirqs enabled, then set the usage
 * bit for all held locks. (disabled hardirqs prevented
@@ -1875,11 +1880,14 @@ void trace_hardirqs_on(void)
 */
if (curr-softirqs_enabled)
if (!mark_held_locks(curr, 0, ip))
-   return;
+   goto end;
 
curr-hardirq_enable_ip = ip;
curr-hardirq_enable_event = ++curr-irq_events;
debug_atomic_inc(hardirqs_on_events);
+end:
+   barrier();
+   current-lockdep_recursion--;
 }
 
 EXPORT_SYMBOL(trace_hardirqs_on);
@@ -1888,14 +1896,17 @@ void trace_hardirqs_off(void)
 {
struct task_struct *curr = current;
 
if (unlikely(!debug_locks || current-lockdep_recursion))
return;
 
+   current-lockdep_recursion++;
+   barrier();
+   
if (DEBUG_LOCKS_WARN_ON(!irqs_disabled()))
-   return;
+   goto end;
 
if (curr-hardirqs_enabled) {
/*
@@ -1910,6 +1921,9 @@ void trace_hardirqs_off(void)
debug_atomic_inc(hardirqs_off_events);
} else
debug_atomic_inc(redundant_hardirqs_off);
+end:
+   barrier();
+   current-lockdep_recursion--;
 }
 
 EXPORT_SYMBOL(trace_hardirqs_off);

-- 
OpenPGP public key:  http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/