RE: [PATCH v9 14/14] powerpc: rewrite local_t using soft_irq

2017-08-04 Thread David Laight 
From: Nicholas Piggin
> Sent: 04 August 2017 10:04
> On Fri, 04 Aug 2017 11:40:43 +1000
> Benjamin Herrenschmidt  wrote:
> 
> > On Fri, 2017-08-04 at 03:50 +1000, Nicholas Piggin wrote:
> > > Hey, so... why are any of these implemented in asm? We should
> > > just do them all in C, right? I looked a bit harder at code gen
> > > and a couple of them are still emitting larx/stcx.
> >
> > As long as we can guarantee that the C compiler won't play games
> > moving stuff around. But yes, I tend to agree.
> 
> 
> I believe so. I mean we already depend on the same pattern for any
> other sequence of local_irq_disable(); c code; local_irq_enable();
> so we'd have other problems if we couldn't.

I'd guess that a "memory" clobber on the irq_disable/enable would be enough.
It could be restricted to the memory area being updated.

David

Re: [PATCH v9 14/14] powerpc: rewrite local_t using soft_irq

2017-08-04 Thread Nicholas Piggin 
On Fri, 04 Aug 2017 11:40:43 +1000
Benjamin Herrenschmidt  wrote:

> On Fri, 2017-08-04 at 03:50 +1000, Nicholas Piggin wrote:
> > Hey, so... why are any of these implemented in asm? We should
> > just do them all in C, right? I looked a bit harder at code gen
> > and a couple of them are still emitting larx/stcx.  
> 
> As long as we can guarantee that the C compiler won't play games
> moving stuff around. But yes, I tend to agree.


I believe so. I mean we already depend on the same pattern for any
other sequence of local_irq_disable(); c code; local_irq_enable();
so we'd have other problems if we couldn't.

I can easily believe there have been bugs with the fixed r13
handling in gcc in the past, but it looks like it does the right
thing now AFAIKS.

Thanks,
Nick

Re: [PATCH v9 14/14] powerpc: rewrite local_t using soft_irq

2017-08-03 Thread Benjamin Herrenschmidt 
On Fri, 2017-08-04 at 03:50 +1000, Nicholas Piggin wrote:
> Hey, so... why are any of these implemented in asm? We should
> just do them all in C, right? I looked a bit harder at code gen
> and a couple of them are still emitting larx/stcx.

As long as we can guarantee that the C compiler won't play games
moving stuff around. But yes, I tend to agree.

Cheers,
Ben.

Re: [PATCH v9 14/14] powerpc: rewrite local_t using soft_irq

2017-08-03 Thread Nicholas Piggin 
On Thu,  3 Aug 2017 09:19:18 +0530
Madhavan Srinivasan  wrote:

> @@ -14,6 +17,202 @@ typedef struct
>  #define local_read(l)atomic_long_read(&(l)->a)
>  #define local_set(l,i)   atomic_long_set(&(l)->a, (i))
>  
> +#ifdef CONFIG_PPC64
> +
> +static __inline__ void local_add(long i, local_t *l)
> +{
> + long t;
> + unsigned long flags;
> +
> + powerpc_local_irq_pmu_save(flags);
> + __asm__ __volatile__(
> + PPC_LL" %0,0(%2)\n\
> + add %0,%1,%0\n"
> + PPC_STL" %0,0(%2)\n"
> + : "=" (t)
> + : "r" (i), "r" (&(l->a.counter)));
> + powerpc_local_irq_pmu_restore(flags);
> +}

Hey, so... why are any of these implemented in asm? We should
just do them all in C, right? I looked a bit harder at code gen
and a couple of them are still emitting larx/stcx.

> +
> +#define local_cmpxchg(l, o, n) \
> + (cmpxchg_local(&((l)->a.counter), (o), (n)))
> +#define local_xchg(l, n) (xchg_local(&((l)->a.counter), (n)))

e.g., these.

Thanks,
Nick