From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752688AbcAPTHL (ORCPT ); Sat, 16 Jan 2016 14:07:11 -0500 Received: from mx2.suse.de ([195.135.220.15]:58457 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752529AbcAPTHJ (ORCPT ); Sat, 16 Jan 2016 14:07:09 -0500 Date: Sat, 16 Jan 2016 20:06:47 +0100 From: Borislav Petkov To: Alexander Kuleshov Cc: Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , x86@kernel.org, Andy Lutomirski , Dave Hansen , Wang Nan , linux-kernel@vger.kernel.org Subject: Re: [PATCH] x86/traps: use conditional_{cli,sti} in preempt_conditinal_{cli_sti} Message-ID: <20160116190647.GB32085@pd.tnic> References: <1452949087-14855-1-git-send-email-kuleshovmail@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1452949087-14855-1-git-send-email-kuleshovmail@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Jan 16, 2016 at 06:58:07PM +0600, Alexander Kuleshov wrote: > The 3d2a71a596bd9 commit (x86, traps: converge do_debug handlers by > Alexander van Heukelum ) introduces two functions: > preempt_conditional_sti/cli() which are enables/disables interrupts > depends on state of the interrupt enable flag and increments/decrements > the preempt counter. > > In the same time arch/x86/kernel/traps.c defines two similar inline > functions: conditional_{sti,cli} which are do the same, but without > touch of the preempt counter. Let's use these functions in the > preempt_conditional_{sti,cli} instead of duplication of 'if' statemets. > > Signed-off-by: Alexander Kuleshov > --- > arch/x86/kernel/traps.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c > index ade185a..30ec8fa 100644 > --- a/arch/x86/kernel/traps.c > +++ b/arch/x86/kernel/traps.c > @@ -92,8 +92,7 @@ static inline void conditional_sti(struct pt_regs *regs) > static inline void preempt_conditional_sti(struct pt_regs *regs) > { > preempt_count_inc(); > - if (regs->flags & X86_EFLAGS_IF) > - local_irq_enable(); > + conditional_sti(regs); What I would do is kill both preempt_conditional_sti() and preempt_conditional_cli() instead. Why? Because call sites become more readable: preempt_disable(); if (regs->flags & X86_EFLAGS_IF) local_irq_enable(); and if (regs->flags & X86_EFLAGS_IF) local_irq_disable(); preempt_enable_no_resched(); Those preempt_* variants are just silly. Yes, I'd delete conditional_cli() because nothing uses it. And since I'm deleting crap, I'd delete conditional_sti() too because it's naming is ugly. You have to know that "STI" means Set Interrupt Flag. Now, if it were called cond_local_irq_enable() that would be better. So yeah, IMO, the most understandable variant would be: preempt_disable(); cond_local_irq_enable(); and the opposing pair: cond_local_irq_disable(); preempt_enable_no_resched(); And *that* is actually understandable at a quick glance. But that's just me. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --