From patchwork Tue Mar 14 15:55:23 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Liebler X-Patchwork-Id: 738785 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vjK8z5zsjz9s73 for ; Wed, 15 Mar 2017 02:55:52 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="U6Nl4Pre"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:from:date:mime-version :in-reply-to:content-type:message-id; q=dns; s=default; b=ThEVq6 UCvagrIxbjx9lyzvAbOKl+gpZk1CKHw/cjDxMpDs0Urnmg2hU9qwLhQMoi8R09Rb 3wUMwxTejV96zEjMXjV+iM91Ox/R1KZOe1JpKoA5cDj+NKlIYD8YxxMUtSnDQY4j Ec4bilqvLHB66rtvR/ZMmnxNXjOWScjMYeya8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:from:date:mime-version :in-reply-to:content-type:message-id; s=default; bh=0S/pe0EbH4P+ MP7AooZ51pZqPXI=; b=U6Nl4PreybfgKzVRRw0kIvDBcpsWFGN0NxGTi/bc6l5e 1TM9sDx8HQnDJDevhfRFTVdjqKzL5roATMncz8n1f1OEuIvaS6cOftTrRxDWbAuI YObmP55US6RdndxQa4VDGUI+1VSoHCJ2pGUXYc8NZzKLxfVTu3lDeDTdgdDc93U= Received: (qmail 66384 invoked by alias); 14 Mar 2017 15:55:33 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 66180 invoked by uid 89); 14 Mar 2017 15:55:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=lhi X-HELO: mx0a-001b2d01.pphosted.com Subject: Re: [PATCH 2/2] S390: Use generic spinlock code. To: libc-alpha@sourceware.org References: <1481905917-15654-1-git-send-email-stli@linux.vnet.ibm.com> <1481905917-15654-2-git-send-email-stli@linux.vnet.ibm.com> <1487018364.16322.88.camel@redhat.com> <1487437519.20203.74.camel@redhat.com> From: Stefan Liebler Date: Tue, 14 Mar 2017 16:55:23 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <1487437519.20203.74.camel@redhat.com> X-TM-AS-GCONF: 00 x-cbid: 17031415-0016-0000-0000-0000045626FF X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17031415-0017-0000-0000-000026CD4155 Message-Id: <6910c6f2-b763-e311-88c9-91d774f796e0@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-03-14_08:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=4 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1702020001 definitions=main-1703140123 On 02/18/2017 06:05 PM, Torvald Riegel wrote: > On Wed, 2017-02-15 at 17:26 +0100, Stefan Liebler wrote: >> On 02/13/2017 09:39 PM, Torvald Riegel wrote: >>> On Wed, 2017-02-08 at 15:49 +0100, Stefan Liebler wrote: >>>> This is an updated version of the patch, which adjusts the s390 specific >>>> atomic-macros in the same way as in include/atomic.h. >>>> Thus passing a volatile int pointer is fine, too. >>> >>> The general direction of this is okay. >>> Some of my comments for patch 1/2 apply here as well (eg, volatile vs. >>> atomics). >>> >> See answer in patch 1/2. >> >>> What I don't like is the choice of 1000 for >>> SPIN_LOCK_READS_BETWEEN_CMPXCHG. Have you run benchmarks to come up >>> with this value, or is it a guess? Why isn't it documented how you end >>> up with this number? >>> We can keep these with a choice such as this, but then we need to have a >>> FIXME comment in the code, explaining that this is just an arbitrary >>> choice. >>> >>> I would guess that just spinning forever is sufficient, and that we >>> don't need to throw in a CAS every now and then; using randomized >>> exponential back-off might be more important. This is something that we >>> would be in a better position to answer if you'd provide a >>> microbenchmark for this choice too. >>> At the end of 2016, I've posted a draft of a microbenchmark for rwlocks. >>> Maybe you can use this as a start and run a few experiments? >> > >> I've run own benchmarks in the same manner as your mentioned >> microbenchmark for rwlocks below. >> You are right, I can't recognize a real difference between >> #define SPIN_LOCK_READS_BETWEEN_CMPXCHG 1000 >> and >> #define SPIN_LOCK_READS_BETWEEN_CMPXCHG -1 >> >> As it does not hurt, I prefer to use a CAS every 1000 plain reads. >> A CAS is not necessary on current CPUs but from architecture >> perspective, it is more correct if there is such a serialization >> instruction. > > What do you mean by "more correct"? I'm not aware of an architecture > that would not ensure that stores on one CPU will eventually become > visible to other CPUs. > > Thus, as I wrote in my review of patch 1/2, I think we should just > remove the occassional CAS (ie, just do the equivalent of the -1 > setting, always). > >> There is a difference between >> #define SPIN_LOCK_READS_BETWEEN_CMPXCHG 0 >> and one of the others. > > Right. We do want to spin if the lock is acquired by another thread. > What we should do in a future patch is to experiment with the back-off > between loads. tile already has some code for this, which we at least > need to integrate at some point. > >> The same applies to >> #define SPIN_TRYLOCK_LOAD_AND_TEST_BEFORE_XCHG 1 >> It does not hurt if the lock is free, but there is a difference if the >> lock is already acquired and trylock is called often. > > Yes. I've replied to this point in the 1/2 patch thread. > >> I've saw your microbenchmark-post and added some notes. > > Thanks. I'll get to them later. > >> I added a FIXME comment to re-evaluate the choice once we have the >> appropriate microbenchmarks. >> >>> Also, I'm not quite sure whether this number is really >>> spinlock-specific, and I would like to find a better place for these. >>> IMO, they should be in some header that contains default tuning >>> parameters for synchronization code, which is provided by each >>> architecture that uses the generic spinlock; we'd have no #ifdef for the >>> tuning parameters, so we'd catch typos in those headers. >>> >> See pthread_spin_parameters.h in updated patch 1/2. > > I suggest that we'll work towards consensus on patch 1/2 first. I > believe once that is done, patch 2/2 would likely just remove s390 code. > I've attached an updated patch which just removes the s390 code. > Thanks for continuing the work on this. I know we have some back and > forth here in terms of overall direction, but I also think we're making > progress and are continually improving the changes. > From 78888be8fab0f3e988360c77240d8aa08fcc980c Mon Sep 17 00:00:00 2001 From: Stefan Liebler Date: Tue, 14 Mar 2017 14:10:05 +0100 Subject: [PATCH 2/2] S390: Use generic spinlock code. This patch removes the s390 specific implementation of spinlock code and is now using the generic one. ChangeLog: * sysdeps/s390/nptl/pthread_spin_init.c: Delete File. * sysdeps/s390/nptl/pthread_spin_lock.c: Likewise. * sysdeps/s390/nptl/pthread_spin_trylock.c: Likewise. * sysdeps/s390/nptl/pthread_spin_unlock.c: Likewise. --- sysdeps/s390/nptl/pthread_spin_init.c | 19 ------------------- sysdeps/s390/nptl/pthread_spin_lock.c | 32 -------------------------------- sysdeps/s390/nptl/pthread_spin_trylock.c | 32 -------------------------------- sysdeps/s390/nptl/pthread_spin_unlock.c | 32 -------------------------------- 4 files changed, 115 deletions(-) delete mode 100644 sysdeps/s390/nptl/pthread_spin_init.c delete mode 100644 sysdeps/s390/nptl/pthread_spin_lock.c delete mode 100644 sysdeps/s390/nptl/pthread_spin_trylock.c delete mode 100644 sysdeps/s390/nptl/pthread_spin_unlock.c diff --git a/sysdeps/s390/nptl/pthread_spin_init.c b/sysdeps/s390/nptl/pthread_spin_init.c deleted file mode 100644 index d826871..0000000 --- a/sysdeps/s390/nptl/pthread_spin_init.c +++ /dev/null @@ -1,19 +0,0 @@ -/* Copyright (C) 2003-2017 Free Software Foundation, Inc. - This file is part of the GNU C Library. - Contributed by Martin Schwidefsky , 2003. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - . */ - -/* Not needed. pthread_spin_init is an alias for pthread_spin_unlock. */ diff --git a/sysdeps/s390/nptl/pthread_spin_lock.c b/sysdeps/s390/nptl/pthread_spin_lock.c deleted file mode 100644 index 7349940..0000000 --- a/sysdeps/s390/nptl/pthread_spin_lock.c +++ /dev/null @@ -1,32 +0,0 @@ -/* Copyright (C) 2003-2017 Free Software Foundation, Inc. - This file is part of the GNU C Library. - Contributed by Martin Schwidefsky , 2003. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - . */ - -#include "pthreadP.h" - -int -pthread_spin_lock (pthread_spinlock_t *lock) -{ - int oldval; - - __asm__ __volatile__ ("0: lhi %0,0\n" - " cs %0,%2,%1\n" - " jl 0b" - : "=&d" (oldval), "=Q" (*lock) - : "d" (1), "m" (*lock) : "cc" ); - return 0; -} diff --git a/sysdeps/s390/nptl/pthread_spin_trylock.c b/sysdeps/s390/nptl/pthread_spin_trylock.c deleted file mode 100644 index 0e848da..0000000 --- a/sysdeps/s390/nptl/pthread_spin_trylock.c +++ /dev/null @@ -1,32 +0,0 @@ -/* Copyright (C) 2003-2017 Free Software Foundation, Inc. - This file is part of the GNU C Library. - Contributed by Martin Schwidefsky , 2003. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - . */ - -#include -#include "pthreadP.h" - -int -pthread_spin_trylock (pthread_spinlock_t *lock) -{ - int old; - - __asm__ __volatile__ ("cs %0,%3,%1" - : "=d" (old), "=Q" (*lock) - : "0" (0), "d" (1), "m" (*lock) : "cc" ); - - return old != 0 ? EBUSY : 0; -} diff --git a/sysdeps/s390/nptl/pthread_spin_unlock.c b/sysdeps/s390/nptl/pthread_spin_unlock.c deleted file mode 100644 index 54e7378..0000000 --- a/sysdeps/s390/nptl/pthread_spin_unlock.c +++ /dev/null @@ -1,32 +0,0 @@ -/* Copyright (C) 2003-2017 Free Software Foundation, Inc. - This file is part of the GNU C Library. - Contributed by Martin Schwidefsky , 2003. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - . */ - -/* Ugly hack to avoid the declaration of pthread_spin_init. */ -#define pthread_spin_init pthread_spin_init_XXX -#include "pthreadP.h" -#undef pthread_spin_init - -int -pthread_spin_unlock (pthread_spinlock_t *lock) -{ - __asm__ __volatile__ (" xc %O0(4,%R0),%0\n" - " bcr 15,0" - : "=Q" (*lock) : "m" (*lock) : "cc" ); - return 0; -} -strong_alias (pthread_spin_unlock, pthread_spin_init) -- 2.7.4