linux-kernel - Re: [PATCH] x86/traps: rename conditional_{sti,cli} to cond_local_irq

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]

Message-ID: <20160117093543.GA8549@pd.tnic>
Date:	Sun, 17 Jan 2016 10:35:43 +0100
From:	Borislav Petkov <bp@...e.de>
To:	Alexander Kuleshov <kuleshovmail@...il.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	Andy Lutomirski <luto@...nel.org>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Wang Nan <wangnan0@...wei.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/traps: rename conditional_{sti,cli} to
 cond_local_irq_{enable,disable}

On Sun, Jan 17, 2016 at 12:53:41PM +0600, Alexander Kuleshov wrote:
> The arch/x86/kernel/traps.c contains definition of two pair of helpers
> conditional_{sti,cli} to enable/disable interrupts depends on state of
> the interrupt flag and *preempt* versions of these helpers.
> 
> This patch provides two following changes:

Looks better.

> 
> 1. contitional_sti() and conditional_cli() renamed to cond_local_irq_enable()
> and cond_local_irq_disable() respectively as these names are more clear.
> 
> 2. preempt_conditional_sti() and preempt_conditional_cli() are removed, because
> because they differ only in the call of preempt_count_{inc,dec} from their
> *non-preempt* variants. Instead of the preempt_conditional_sti() and
> preempt_conditional_cli() we are using it in place, like:
> 
>         preempt_disable();
>         cond_local_irq_enable(regs);
> 
> or
> 
>         cond_local_irq_disable(regs);
>         preempt_enable_no_resched();

So I gave you the example with preempt_enable_no_resched() but you have
sched_preempt_enable_no_resched() below. Why?

Does the traps.c code look like scheduler code and there you have to use
scheduler primitives? Or was there another reason for it I'm not seeing
right now?

Also, for your next submission, always try to answer to the question
"Why is the change being done" in the commit message instead of
explaining what you're doing. Because "what you're doing" we can see,
*why* you're doing it is the much more interesting question.

For example, I would've written:

"Make the preemption and interrupt flag handling more readable in the
traps.c code. While at it, remove silly helpers and rename others to
more understandable names so that one doesn't have to go and lookup the
function definition when looking at the code flow."

Or something with a similar effect...

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--