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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 05 Oct 2010 23:04:41 +0200
From:	Jiri Slaby <jslaby@...e.cz>
To:	Ingo Molnar <mingo@...e.hu>
CC:	akpm@...ux-foundation.org, mm-commits@...r.kernel.org,
	a.p.zijlstra@...llo.nl, tglx@...utronix.de,
	linux-kernel@...r.kernel.org
Subject: Re: + softirq-cleanup-preempt-check.patch added to -mm tree

On 10/05/2010 09:38 PM, Ingo Molnar wrote:
> 
> * akpm@...ux-foundation.org <akpm@...ux-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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/