linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Guo Ren <ren_guo@c-sky.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: akpm@linux-foundation.org, arnd@arndb.de,
	daniel.lezcano@linaro.org, davem@davemloft.net,
	gregkh@linuxfoundation.org, hch@infradead.org,
	marc.zyngier@arm.com, mark.rutland@arm.com, robh@kernel.org,
	tglx@linutronix.de, linux-kernel@vger.kernel.org,
	linux-arch@vger.kernel.org, devicetree@vger.kernel.org,
	robh+dt@kernel.org, c-sky_gcc_upstream@c-sky.com,
	Andrea Parri <andrea.parri@amarulasolutions.com>
Subject: Re: [PATCH V9 11/21] csky: Atomic operations
Date: Mon, 22 Oct 2018 09:52:58 +0800	[thread overview]
Message-ID: <20181022015258.GA3649@guoren-Inspiron-7460> (raw)
In-Reply-To: <20181021205508.GJ4931@worktop.programming.kicks-ass.net>

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

  reply	other threads:[~2018-10-22  1:53 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-16  2:58 [PATCH V9 00/21] C-SKY(csky) Linux Kernel Port Guo Ren
2018-10-16  2:58 ` [PATCH V9 01/21] csky: Build infrastructure Guo Ren
2018-10-23  0:08   ` Guo Ren
2018-10-24 22:53     ` Arnd Bergmann
2018-10-25 17:04       ` Guo Ren
2018-10-16  2:58 ` [PATCH V9 02/21] csky: defconfig Guo Ren
2018-10-17 14:56   ` Arnd Bergmann
2018-10-16  2:58 ` [PATCH V9 03/21] csky: Kernel booting Guo Ren
2018-10-17 14:58   ` Arnd Bergmann
2018-10-16  2:58 ` [PATCH V9 04/21] csky: Exception handling and mm-fault Guo Ren
2018-10-17 14:59   ` Arnd Bergmann
2018-10-16  2:58 ` [PATCH V9 05/21] csky: System Call Guo Ren
2018-10-17 15:02   ` Arnd Bergmann
2018-10-18  2:02     ` Guo Ren
2018-10-16  2:58 ` [PATCH V9 06/21] csky: Cache and TLB routines Guo Ren
2018-10-17 15:08   ` Arnd Bergmann
2018-10-16  2:58 ` [PATCH V9 07/21] csky: MMU and page table management Guo Ren
2018-10-17 15:06   ` Arnd Bergmann
2018-10-18  2:05     ` Guo Ren
2018-10-16  2:58 ` [PATCH V9 08/21] csky: Process management and Signal Guo Ren
2018-10-17 15:11   ` Arnd Bergmann
2018-10-18  2:37     ` Guo Ren
2018-10-16  2:58 ` [PATCH V9 09/21] csky: VDSO and rt_sigreturn Guo Ren
2018-10-17 15:13   ` Arnd Bergmann
2018-10-16  2:58 ` [PATCH V9 10/21] csky: IRQ handling Guo Ren
2018-10-17 15:14   ` Arnd Bergmann
2018-10-18  2:39     ` Guo Ren
2018-10-16  2:58 ` [PATCH V9 11/21] csky: Atomic operations Guo Ren
2018-10-17 15:17   ` Arnd Bergmann
2018-10-18  2:40     ` Guo Ren
2018-10-21 20:55   ` Peter Zijlstra
2018-10-22  1:52     ` Guo Ren [this message]
2018-10-16  2:58 ` [PATCH V9 12/21] csky: ELF and module probe Guo Ren
2018-10-17 15:18   ` Arnd Bergmann
2018-10-18  2:49     ` Guo Ren
2018-10-18  8:31       ` Arnd Bergmann
2018-10-16  2:58 ` [PATCH V9 13/21] csky: Library functions Guo Ren
2018-10-17 15:24   ` Arnd Bergmann
2018-10-18  3:10     ` Guo Ren
2018-10-16  2:58 ` [PATCH V9 14/21] csky: User access Guo Ren
2018-10-17 15:37   ` Arnd Bergmann
2018-10-16  2:58 ` [PATCH V9 15/21] csky: Debug and Ptrace GDB Guo Ren
2018-10-17 15:46   ` Arnd Bergmann
2018-10-18  3:17     ` Guo Ren
2018-10-16  2:58 ` [PATCH V9 16/21] csky: SMP support Guo Ren
2018-10-17 15:47   ` Arnd Bergmann
2018-10-16  2:58 ` [PATCH V9 17/21] csky: Misc headers Guo Ren
2018-10-17 15:49   ` Arnd Bergmann
2018-10-16  2:58 ` [PATCH V9 18/21] dt-bindings: csky CPU Bindings Guo Ren
2018-10-17 15:50   ` Arnd Bergmann
2018-10-18  3:21     ` Guo Ren
2018-10-18  3:45       ` Guo Ren
2018-10-18 14:31   ` Rob Herring
2018-10-19  2:19     ` Guo Ren
2018-10-16  2:58 ` [PATCH V9 19/21] dt-bindings: Add vendor prefix for csky Guo Ren
2018-10-16  2:58 ` [PATCH V9 20/21] MAINTAINERS: Add csky Guo Ren
2018-10-17 15:51   ` Arnd Bergmann
2018-10-16  5:48 ` [PATCH V9 21/21] csky: support dword access for get_user_size() Guo Ren
2018-10-17 15:44   ` Arnd Bergmann
2018-10-18  3:41     ` Guo Ren
2018-10-18  8:34       ` Arnd Bergmann
2018-10-18  8:57         ` Guo Ren
2018-10-24  7:17           ` Arnd Bergmann
2018-10-25 17:08             ` Guo Ren
2018-10-17 15:58 ` [PATCH V9 00/21] C-SKY(csky) Linux Kernel Port Arnd Bergmann
2018-10-18  4:10   ` Guo Ren
2018-10-18  8:36     ` Arnd Bergmann
2018-10-18  9:03       ` Guo Ren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181022015258.GA3649@guoren-Inspiron-7460 \
    --to=ren_guo@c-sky.com \
    --cc=akpm@linux-foundation.org \
    --cc=andrea.parri@amarulasolutions.com \
    --cc=arnd@arndb.de \
    --cc=c-sky_gcc_upstream@c-sky.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marc.zyngier@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=peterz@infradead.org \
    --cc=robh+dt@kernel.org \
    --cc=robh@kernel.org \
    --cc=tglx@linutronix.de \
    --subject='Re: [PATCH V9 11/21] csky: Atomic operations' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).