Re: [RFC] Add implicit barriers to irqsave/restore class of functions

From: Christian Ruppert <christian.ruppert@abilis.com>
To: Vineet Gupta <Vineet.Gupta1@synopsys.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Pierrick Hascoet <pierrick.hascoet@abilis.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@kernel.org>
Subject: Re: [RFC] Add implicit barriers to irqsave/restore class of functions
Date: Thu, 4 Apr 2013 10:26:53 +0200	[thread overview]
Message-ID: <20130404082652.GA15261@ab42.lan> (raw)
In-Reply-To: <515C2EC0.4090904@synopsys.com>

Hi Vineet,

Just a short message to inform you that the test campaign on our RFC
patch has run through successfully and we consider the patch stable
enough for release to our customers. The main reason the barriers were
added in this particular place is because other architectures do the
same.

I agree that the patch adding barriers in preempt_* is more in line with
Thomas' comment than our RFC (which predates that comment) and we have
also started a test campaign for that patch yesterday. I'll give an update
directly in reply to the patch when that campaign has run through.

Greetings,
  Christian

On Wed, Apr 03, 2013 at 06:59:36PM +0530, Vineet Gupta wrote:
> Hi Christian,
> 
> On 04/03/2013 06:40 PM, Christian Ruppert wrote:
> > This patch adds implicit memory barriers to irqsave/restore functions of
> > the ARC architecture port in line with what is done in other architectures.
> >
> > It seems to fix several seemingly unrelated issues in our platform but for
> > the moment it is insufficiently tested (and might even be incomplete).
> > Please comment.
> 
> I think this is not needed. Semantically we need barrier for spinlocks, not irq
> save/restore - although spin locks are one of the primary users of irq
> save/restore API.
> 
> So repeating what Thomas already said, the barrier already exists for
> PREEMPT_COUNT, we need to make sure they are present for !PREEMPT_COUNT.
> 
> -Vineet
> 
> > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > ---
> >  arch/arc/include/asm/irqflags.h |    8 ++++----
> >  1 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arc/include/asm/irqflags.h b/arch/arc/include/asm/irqflags.h
> > index ccd8480..c8147d1 100644
> > --- a/arch/arc/include/asm/irqflags.h
> > +++ b/arch/arc/include/asm/irqflags.h
> > @@ -39,7 +39,7 @@ static inline long arch_local_irq_save(void)
> >  	"	flag.nz %0		\n"
> >  	: "=r"(temp), "=r"(flags)
> >  	: "n"((STATUS_E1_MASK | STATUS_E2_MASK))
> > -	: "cc");
> > +	: "memory", "cc");
> >  
> >  	return flags;
> >  }
> > @@ -53,7 +53,7 @@ static inline void arch_local_irq_restore(unsigned long flags)
> >  	__asm__ __volatile__(
> >  	"	flag %0			\n"
> >  	:
> > -	: "r"(flags));
> > +	: "r"(flags) : "memory");
> >  }
> >  
> >  /*
> > @@ -73,7 +73,7 @@ static inline void arch_local_irq_disable(void)
> >  	"	and %0, %0, %1		\n"
> >  	"	flag %0			\n"
> >  	: "=&r"(temp)
> > -	: "n"(~(STATUS_E1_MASK | STATUS_E2_MASK)));
> > +	: "n"(~(STATUS_E1_MASK | STATUS_E2_MASK)) : "memory");
> >  }
> >  
> >  /*
> > @@ -85,7 +85,7 @@ static inline long arch_local_save_flags(void)
> >  
> >  	__asm__ __volatile__(
> >  	"	lr  %0, [status32]	\n"
> > -	: "=&r"(temp));
> > +	: "=&r"(temp) : : "memory");
> >  
> >  	return temp;
> >  }
> 

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates