lkml.org 
[lkml]   [2018]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V9 11/21] csky: Atomic operations
Thx Peter, 

Your review has been a great help.

On Sun, Oct 21, 2018 at 10:55:08PM +0200, Peter Zijlstra wrote:
> On Tue, Oct 16, 2018 at 10:58:30AM +0800, Guo Ren wrote:
> > + smp_mb();
> > + lock->tickets.owner++;
>
> WRITE_ONCE(lock->tickets.owner, lock->tickets.owner + 1);
Yes, approve! I should use WRITE_ONCE/READ_ONCE as necessary.

> > +/*
> > + * Test-and-set spin-locking.
> > + */
>
> I'm still not entirely sure why you want to have two spinlock
> implementations; to me that is just extra maintenance overhead.
Test and set (spinlock & rwlock) is easier for debug :P, and I don't
know the details of queue-rwlock (maybe I should learn it).

From education's view, we could teach students both of them in
arch/csky :)

Anyway, I just want to keep both of them.

Thx

> > + asm volatile (
> > + " movi %0, 0 \n"
> > + " stw %0, (%1) \n"
> > + : "=&r" (tmp)
> > + : "r"(p)
> > + : "cc");
>
> WRITE_ONCE(lock->lock, 0);
> ?
Cool ... I like WRITE_ONCE style.

> > +
> > +#define arch_spin_is_locked(x) (READ_ONCE((x)->lock) != 0)
> > +
> > +/*
> > + * read lock/unlock/trylock
> > + */
>
> Idem, why do you want a second rwlock_t implementation?
The same as above of spinlock.

> > + asm volatile (
> > + "1: ldex.w %0, (%1) \n"
> > + " movi %0, 0 \n"
> > + " stex.w %0, (%1) \n"
> > + " bez %0, 1b \n"
> > + : "=&r" (tmp)
> > + : "r"(p)
> > + : "cc");
>
> Isn't that:
>
> WRITE_ONCE(lock->lock, 0);
Yes, no need ldex/stex and you've mentioned in spinlock before :P

> > diff --git a/arch/csky/kernel/atomic.S b/arch/csky/kernel/atomic.S
> > new file mode 100644
> > index 0000000..d2357c8
> > --- /dev/null
> > +++ b/arch/csky/kernel/atomic.S
> > @@ -0,0 +1,87 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +// Copyright (C) 2018 Hangzhou C-SKY Microsystems co.,ltd.
> > +
> > +#include <linux/linkage.h>
> > +#include <abi/entry.h>
> > +
> > +.text
> > +
> > +/*
> > + * int csky_cmpxchg(int oldval, int newval, int *ptr)
> > + *
> > + * If *ptr != oldval && return 1,
> > + * else *ptr = newval return 0.
> > + */
> > +#ifdef CONFIG_CPU_HAS_LDSTEX
> > +ENTRY(csky_cmpxchg)
> > + USPTOKSP
> > + mfcr a3, epc
> > + INCTRAP a3
> > +
> > + subi sp, 8
> > + stw a3, (sp, 0)
> > + mfcr a3, epsr
> > + stw a3, (sp, 4)
> > +
> > + psrset ee
> > +1:
> > + ldex a3, (a2)
> > + cmpne a0, a3
> > + bt16 2f
> > + mov a3, a1
> > + stex a3, (a2)
> > + bez a3, 1b
> > +2:
> > + sync.is
> > + mvc a0
> > + ldw a3, (sp, 0)
> > + mtcr a3, epc
> > + ldw a3, (sp, 4)
> > + mtcr a3, epsr
> > + addi sp, 8
> > + KSPTOUSP
> > + rte
> > +END(csky_cmpxchg)
>
> I don't understand why you have this; if the CPU has ll/sc, why do you
> need syscall support?
I've really considered your advice before, but from abi view 610/807/810
all have csky_cmpxchg trap and we want to make them the same. Some apps
use the trap directly and not use libc api. Maybe we could delete the
trap in future version of kernel.

>
> In any case, nothing terminally broken; so I suppose that's good enough
> for starters. I just really don't understand some decisions (like having
> two lock implementations and having that cmpxchg syscall when you have
> hardware ll/sc).
>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Thx peter and the two questions which I've clarified in above.

Best Regards
Guo Ren

\
 
 \ /
  Last update: 2018-10-22 03:54    [W:0.051 / U:11.320 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site