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

[Jiri Slaby <jslaby@suse.cz>]

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