Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

2017-01-14 Thread Ingo Molnar 

* Paul E. McKenney  wrote:

> On Sun, Jan 15, 2017 at 08:11:23AM +0100, Ingo Molnar wrote:
> > 
> > * Paul E. McKenney  wrote:
> > 
> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > index 357b32aaea48..5fdfe874229e 100644
> > > --- a/include/linux/rcupdate.h
> > > +++ b/include/linux/rcupdate.h
> > > @@ -1175,11 +1175,11 @@ do { \
> > >   * if the UNLOCK and LOCK are executed by the same CPU or if the
> > >   * UNLOCK and LOCK operate on the same lock variable.
> > >   */
> > > -#ifdef CONFIG_PPC
> > > +#ifdef CONFIG_ARCH_WEAK_RELACQ
> > >  #define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for 
> > > lock. */
> > > -#else /* #ifdef CONFIG_PPC */
> > > +#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
> > >  #define smp_mb__after_unlock_lock()  do { } while (0)
> > > -#endif /* #else #ifdef CONFIG_PPC */
> > > +#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
> > >  
> > >  
> > 
> > So at the risk of sounding totally pedantic, why not structure it like the 
> > existing smp_mb__before/after*() primitives in barrier.h?
> > 
> > That allows asm-generic/barrier.h to pick up the definition - for example 
> > in the 
> > case of smp_acquire__after_ctrl_dep() we do:
> > 
> >  #ifndef smp_acquire__after_ctrl_dep
> >  #define smp_acquire__after_ctrl_dep()   smp_rmb()
> >  #endif
> > 
> > Which allows Tile to relax it:
> > 
> >   arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep()   
> > barrier()
> > 
> > I.e. I'd move the API definition out of rcupdate.h and into barrier.h - 
> > even 
> > though tree-RCU is the only user of this barrier type.
> 
> I wouldn't have any problem with that, however, some time back it was
> moved into RCU because (you guessed it!) RCU is the only user.  ;-)

Indeed ...

[sounds of rummaging around in the Git tree]

I found this commit of yours from ancient history (more than a year ago!):

  commit 12d560f4ea87030667438a169912380be00cea4b
  Author: Paul E. McKenney 
  Date:   Tue Jul 14 18:35:23 2015 -0700

rcu,locking: Privatize smp_mb__after_unlock_lock()

RCU is the only thing that uses smp_mb__after_unlock_lock(), and is
likely the only thing that ever will use it, so this commit makes this
macro private to RCU.

Signed-off-by: Paul E. McKenney 
Cc: Will Deacon 
Cc: Peter Zijlstra 
Cc: Benjamin Herrenschmidt 
Cc: "linux-a...@vger.kernel.org" 

So I concur and I'm fine with your patch - or with the status quo code as well.

Thanks,

Ingo

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

2017-01-14 Thread Paul E. McKenney 
On Sun, Jan 15, 2017 at 08:11:23AM +0100, Ingo Molnar wrote:
> 
> * Paul E. McKenney  wrote:
> 
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 357b32aaea48..5fdfe874229e 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -1175,11 +1175,11 @@ do { \
> >   * if the UNLOCK and LOCK are executed by the same CPU or if the
> >   * UNLOCK and LOCK operate on the same lock variable.
> >   */
> > -#ifdef CONFIG_PPC
> > +#ifdef CONFIG_ARCH_WEAK_RELACQ
> >  #define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for 
> > lock. */
> > -#else /* #ifdef CONFIG_PPC */
> > +#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
> >  #define smp_mb__after_unlock_lock()do { } while (0)
> > -#endif /* #else #ifdef CONFIG_PPC */
> > +#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
> >  
> >  
> 
> So at the risk of sounding totally pedantic, why not structure it like the 
> existing smp_mb__before/after*() primitives in barrier.h?
> 
> That allows asm-generic/barrier.h to pick up the definition - for example in 
> the 
> case of smp_acquire__after_ctrl_dep() we do:
> 
>  #ifndef smp_acquire__after_ctrl_dep
>  #define smp_acquire__after_ctrl_dep()   smp_rmb()
>  #endif
> 
> Which allows Tile to relax it:
> 
>   arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep()   
> barrier()
> 
> I.e. I'd move the API definition out of rcupdate.h and into barrier.h - even 
> though tree-RCU is the only user of this barrier type.

I wouldn't have any problem with that, however, some time back it was
moved into RCU because (you guessed it!) RCU is the only user.  ;-)

Thanx, Paul

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

2017-01-14 Thread Ingo Molnar 

* Paul E. McKenney  wrote:

> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 357b32aaea48..5fdfe874229e 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -1175,11 +1175,11 @@ do { \
>   * if the UNLOCK and LOCK are executed by the same CPU or if the
>   * UNLOCK and LOCK operate on the same lock variable.
>   */
> -#ifdef CONFIG_PPC
> +#ifdef CONFIG_ARCH_WEAK_RELACQ
>  #define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for lock. */
> -#else /* #ifdef CONFIG_PPC */
> +#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
>  #define smp_mb__after_unlock_lock()  do { } while (0)
> -#endif /* #else #ifdef CONFIG_PPC */
> +#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
>  
>  

So at the risk of sounding totally pedantic, why not structure it like the 
existing smp_mb__before/after*() primitives in barrier.h?

That allows asm-generic/barrier.h to pick up the definition - for example in 
the 
case of smp_acquire__after_ctrl_dep() we do:

 #ifndef smp_acquire__after_ctrl_dep
 #define smp_acquire__after_ctrl_dep()   smp_rmb()
 #endif

Which allows Tile to relax it:

  arch/tile/include/asm/barrier.h:#define smp_acquire__after_ctrl_dep()   
barrier()

I.e. I'd move the API definition out of rcupdate.h and into barrier.h - even 
though tree-RCU is the only user of this barrier type.

Thanks,

Ingo

[PATCH] scsi: ibmvscsi: constify dev_pm_ops structures

2017-01-14 Thread Bhumika Goyal 
Declare dev_pm_ops structures as const as they are only stored in the pm
field of a vio_driver structure. This field is of type const, so
dev_pm_ops structures having similar properties can be declared const.
Done using Coccinelle:

@r1 disable optional_qualifier@
identifier i;
position p;
@@
static struct dev_pm_ops i@p={...};

@ok@
identifier r1.i;
position p;
struct vio_driver x;
@@
x.pm=@p;

@bad@
position p!={r1.p,ok.p};
identifier r1.i;
@@
i@p

@depends on !bad disable optional_qualifier@
identifier r1.i;
@@
+const
struct dev_pm_ops i;

File sizes after cross compiling the .o files for powerpc architecture.

File size before:
  text data bss dec hex filename
  578935665  20   63578f85a drivers/scsi/ibmvscsi/ibmvfc.o

File size after:
  text data bss dec hex filename
  580775481  20   63578f85a drivers/scsi/ibmvscsi/ibmvfc.o

File size before:
   textdata bss dec hex filenam
  209902816  80   238865d4e drivers/scsi/ibmvscsi/ibmvscsi.o

File size after:
  text data bss dec hex filename
  211742632  80   238865d4e drivers/scsi/ibmvscsi/ibmvscsi.o

Signed-off-by: Bhumika Goyal 
---
 drivers/scsi/ibmvscsi/ibmvfc.c   | 2 +-
 drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index 78b72c2..1e1b343 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -4940,7 +4940,7 @@ static unsigned long ibmvfc_get_desired_dma(struct 
vio_dev *vdev)
 };
 MODULE_DEVICE_TABLE(vio, ibmvfc_device_table);
 
-static struct dev_pm_ops ibmvfc_pm_ops = {
+static const struct dev_pm_ops ibmvfc_pm_ops = {
.resume = ibmvfc_resume
 };
 
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 50cd011..b94e7b1 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -2335,7 +2335,7 @@ static int ibmvscsi_resume(struct device *dev)
 };
 MODULE_DEVICE_TABLE(vio, ibmvscsi_device_table);
 
-static struct dev_pm_ops ibmvscsi_pm_ops = {
+static const struct dev_pm_ops ibmvscsi_pm_ops = {
.resume = ibmvscsi_resume
 };
 
-- 
1.9.1

Re: [PATCH tip/core/rcu 2/3] srcu: Force full grace-period ordering

2017-01-14 Thread Paul E. McKenney 
On Sat, Jan 14, 2017 at 11:54:17AM -0800, Paul E. McKenney wrote:
> On Sat, Jan 14, 2017 at 10:35:50AM +0100, Ingo Molnar wrote:
> > * Paul E. McKenney  wrote:

[ . . . ]

> > > + */
> > > +#ifdef CONFIG_PPC
> > > +#define smp_mb__after_unlock_lock()  smp_mb()  /* Full ordering for 
> > > lock. */
> > > +#else /* #ifdef CONFIG_PPC */
> > > +#define smp_mb__after_unlock_lock()  do { } while (0)
> > > +#endif /* #else #ifdef CONFIG_PPC */
> > 
> > Yeah, so I realize that this was pre-existing code, but putting CONFIG_$ARCH
> > #ifdefs into generic headers is generally frowned upon.
> > 
> > The canonical approach would be either to define a helper Kconfig variable 
> > that 
> > can be set by PPC (but other architectures don't need to set it), or to 
> > expose a 
> > suitable macro (function) for architectures to define in their barrier.h 
> > arch 
> > header file.
> 
> Very well, I will add a separate commit for this.  4.11 OK?

Does the patch below seem reasonable?

Thanx, Paul



commit 271c0601237c41a279f975563e13837bace0df03
Author: Paul E. McKenney 
Date:   Sat Jan 14 13:32:50 2017 -0800

rcu: Make arch select smp_mb__after_unlock_lock() strength

The definition of smp_mb__after_unlock_lock() is currently smp_mb()
for CONFIG_PPC and a no-op otherwise.  It would be better to instead
provide an architecture-selectable Kconfig option, and select the
strength of smp_mb__after_unlock_lock() based on that option.  This
commit therefore creates CONFIG_ARCH_WEAK_RELACQ, has PPC select it,
and bases the definition of smp_mb__after_unlock_lock() on this new
CONFIG_ARCH_WEAK_RELACQ Kconfig option.

Reported-by: Ingo Molnar 
Signed-off-by: Paul E. McKenney 
Cc: Peter Zijlstra 
Cc: Will Deacon 
Cc: Boqun Feng 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Michael Ellerman 
Cc: 

diff --git a/arch/Kconfig b/arch/Kconfig
index 99839c23d453..94dd90d33f95 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -316,6 +316,9 @@ config HAVE_CMPXCHG_LOCAL
 config HAVE_CMPXCHG_DOUBLE
bool
 
+config ARCH_WEAK_RELACQ
+   bool
+
 config ARCH_WANT_IPC_PARSE_VERSION
bool
 
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index a8ee573fe610..e7083d27271e 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -165,6 +165,7 @@ config PPC
select HAVE_ARCH_HARDENED_USERCOPY
select HAVE_KERNEL_GZIP
select HAVE_CC_STACKPROTECTOR
+   select ARCH_WEAK_RELACQ
 
 config GENERIC_CSUM
def_bool CPU_LITTLE_ENDIAN
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 357b32aaea48..5fdfe874229e 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -1175,11 +1175,11 @@ do { \
  * if the UNLOCK and LOCK are executed by the same CPU or if the
  * UNLOCK and LOCK operate on the same lock variable.
  */
-#ifdef CONFIG_PPC
+#ifdef CONFIG_ARCH_WEAK_RELACQ
 #define smp_mb__after_unlock_lock()smp_mb()  /* Full ordering for lock. */
-#else /* #ifdef CONFIG_PPC */
+#else /* #ifdef CONFIG_ARCH_WEAK_RELACQ */
 #define smp_mb__after_unlock_lock()do { } while (0)
-#endif /* #else #ifdef CONFIG_PPC */
+#endif /* #else #ifdef CONFIG_ARCH_WEAK_RELACQ */
 
 
 #endif /* __LINUX_RCUPDATE_H */