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

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