+ softirq-cleanup-preempt-check.patch added to -mm tree

+ softirq-cleanup-preempt-check.patch added to -mm tree

[akpm]

The patch titled
     softirq: cleanup preempt check
has been added to the -mm tree.  Its filename is
     softirq-cleanup-preempt-check.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: softirq: cleanup preempt check
From: Jiri Slaby <jslaby@suse.cz>

The preempt debugging check in the softirq code is ugly.  The print is
lenghty and spread over several lines which makes the code difficult to
read.  This was also the reason for the wrong spacing in the message.

So move the check into a separate inlined function and make the print text
a one-liner for easier grepping.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/softirq.c |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff -puN kernel/softirq.c~softirq-cleanup-preempt-check kernel/softirq.c
--- a/kernel/softirq.c~softirq-cleanup-preempt-check
+++ a/kernel/softirq.c
@@ -177,6 +177,17 @@ void local_bh_enable_ip(unsigned long ip
 }
 EXPORT_SYMBOL(local_bh_enable_ip);
 
+static inline void softirq_preempt_check(struct softirq_action *h,
+		int prev_count)
+{
+	if (unlikely(prev_count != preempt_count())) {
+		printk(KERN_ERR "huh, entered softirq %td %s %pf with preempt_count %08x, exited with %08x?\n",
+			h - softirq_vec, softirq_to_name[h - softirq_vec],
+			h->action, prev_count, preempt_count());
+		preempt_count() = prev_count;
+	}
+}
+
 /*
  * We restart softirq processing MAX_SOFTIRQ_RESTART times,
  * and we fall back to softirqd after that.
@@ -218,14 +229,8 @@ restart:
 			trace_softirq_entry(h, softirq_vec);
 			h->action(h);
 			trace_softirq_exit(h, softirq_vec);
-			if (unlikely(prev_count != preempt_count())) {
-				printk(KERN_ERR "huh, entered softirq %td %s "
-				       "%pf with preempt_count %08x,"
-				       " exited with %08x?\n", h - softirq_vec,
-				       softirq_to_name[h - softirq_vec],
-				       h->action, prev_count, preempt_count());
-				preempt_count() = prev_count;
-			}
+
+			softirq_preempt_check(h, prev_count);
 
 			rcu_bh_qs(cpu);
 		}
_

Patches currently in -mm which might be from jslaby@suse.cz are

linux-next.patch
drivers-media-ir-ene_irc-fix-null-dereference.patch
softirq-improve-preempt-count-error-message.patch
softirq-cleanup-preempt-check.patch
leds-route-kbd-leds-through-the-generic-leds-layer.patch
hpet-unmap-unused-i-o-space.patch

Re: + softirq-cleanup-preempt-check.patch added to -mm tree

[Ingo Molnar]

* akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:

> +static inline void softirq_preempt_check(struct softirq_action *h,
> +		int prev_count)

unnecessary linebreak.

> +{
> +	if (unlikely(prev_count != preempt_count())) {

should be something like:

	if (prev_count == preempt_count())
		return;

then the rest will look cleaner as well.

> +		printk(KERN_ERR "huh, entered softirq %td %s %pf with preempt_count %08x, exited with %08x?\n",

Could be pr_err().

> +			softirq_preempt_check(h, prev_count);

Please put 'debug' in the function name as i suggested - that way people 
will only read it if they are interested in debug checks.

softirq_debug_check() would be perfect. (which might even grow new 
checks in the future)

Thanks,

	Ingo

Re: + softirq-cleanup-preempt-check.patch added to -mm tree

[Jiri Slaby]

On 10/05/2010 09:38 PM, Ingo Molnar wrote:
> 
> * akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:
> 
>> +static inline void softirq_preempt_check(struct softirq_action *h,
>> +		int prev_count)
> 
> unnecessary linebreak.

How unnecessary is this linebreak:
$ wc -c
static inline void softirq_preempt_check(struct softirq_action *h, int
prev_count)
83

People, including me, still work with 80-col terminals. What I can
tolerate are undivided strings, because it sucks if one cannot grep for
anything from the log.

>> +	if (unlikely(prev_count != preempt_count())) {
> 
> should be something like:
> 
> 	if (prev_count == preempt_count())
> 		return;
> 
> then the rest will look cleaner as well.

Yeah, I thought about that, but it doesn't make sense. Sometime later if
someone would want to add another check there, they would have to put
all that stuff back. And also it doesn't help readability in any way.

>> +		printk(KERN_ERR "huh, entered softirq %td %s %pf with preempt_count %08x, exited with %08x?\n",
> 
> Could be pr_err().

It could, but I dislike those just because 'pr' doesn't mean 'print'
anymore and beginners are getting lost like never before. If it only was
print_err.

>> +			softirq_preempt_check(h, prev_count);
> 
> Please put 'debug' in the function name as i suggested - that way people 
> will only read it if they are interested in debug checks.
> 
> softirq_debug_check() would be perfect. (which might even grow new 
> checks in the future)

Actually, not that perfect. Before I renamed the function to
softirq_preempt_check, it looked like to me: Hmm, there is a debug
check. What does it check?

With softirq_preempt_check, it does exactly what the name says. And
'check' says, it's just a some kind of sanity test. At least to me.

thanks,
-- 
js
suse labs

Re: + softirq-cleanup-preempt-check.patch added to -mm tree

[Peter Zijlstra]

On Tue, 2010-10-05 at 23:04 +0200, Jiri Slaby wrote:
> >> +static inline void softirq_preempt_check(struct softirq_action *h,
> >> +            int prev_count)
> > 
> > unnecessary linebreak.
> 
> How unnecessary is this linebreak:
> $ wc -c
> static inline void softirq_preempt_check(struct softirq_action *h, int
> prev_count)
> 83
> 
> People, including me, still work with 80-col terminals. What I can
> tolerate are undivided strings, because it sucks if one cannot grep for
> anything from the log 

I actually work on ~350 character wide terminals, but then, I vert-split
it 4-ways so I end up with ~85 chars per column.. :-)

But I agree the line should be split, I however much prefer the form:

static inline
void softirq_preempt_check(struct softirq_action *h, int prev_count)

Re: + softirq-cleanup-preempt-check.patch added to -mm tree

[Jiri Slaby]

On 10/09/2010 06:15 PM, Peter Zijlstra wrote:
> But I agree the line should be split, I however much prefer the form:
> 
> static inline
> void softirq_preempt_check(struct softirq_action *h, int prev_count)

Ok I can do that while I'm ratelimiting the message. When this error
occurs, it floods the kmsg quite a lot for works like netif_rx.

thanks,
-- 
js
suse labs