From patchwork Wed Jul 30 09:17:02 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Guy Martin X-Patchwork-Id: 4646621 Return-Path: X-Original-To: patchwork-linux-parisc@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id B4DFCC0338 for ; Wed, 30 Jul 2014 09:17:25 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 655972012D for ; Wed, 30 Jul 2014 09:17:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D487120120 for ; Wed, 30 Jul 2014 09:17:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751356AbaG3JRW (ORCPT ); Wed, 30 Jul 2014 05:17:22 -0400 Received: from venus.vo.lu ([80.90.45.96]:56186 "EHLO venus.vo.lu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751211AbaG3JRU (ORCPT ); Wed, 30 Jul 2014 05:17:20 -0400 Received: from ibiza.lux.tuxicoman.be (UnknownHost [85.93.195.103]) by venus.vo.lu with SMTP (version=TLS\Tls cipher=Aes128 bits=128); Wed, 30 Jul 2014 11:16:59 +0200 Received: from cartman.lux.tuxicoman.be ([2001:7e8:2221:200:224:8cff:fed3:dda9] helo=webmail.tuxicoman.be) by ibiza.lux.tuxicoman.be with esmtp (Exim 4.80.1) (envelope-from ) id 1XCQ0M-0004lU-Kj; Wed, 30 Jul 2014 11:17:02 +0200 MIME-Version: 1.0 Date: Wed, 30 Jul 2014 11:17:02 +0200 From: Guy Martin To: Helge Deller Cc: linux-parisc@vger.kernel.org Subject: Re: [RFC PATCHv2] 64bit LWS CAS In-Reply-To: <53D810FB.9010406@gmx.de> References: <20140729211334.02d2b5e2@borg.lux.tuxicoman.be> <53D810FB.9010406@gmx.de> Message-ID: X-Sender: gmsoft@tuxicoman.be User-Agent: Roundcube Webmail/0.9.5 Sender: linux-parisc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-parisc@vger.kernel.org X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Helge, On 2014-07-29 23:24, Helge Deller wrote: > Hi Guy, > > Very nice work ! Thanks ! > I compile-tested it...(but didn't runtime tested it yet): I have attached the small test program I use to test the LWS outside gcc for easy testing. Moreover, the gcc patch I attached previously is not the right one. It also contained JDA's changes. I have attached a patch that applies against gcc's trunk. > AS arch/parisc/kernel/syscall.o > /home/cvs/LINUX/git-kernel/linux-2.6/arch/parisc/kernel/syscall.S: > Assembler messages: > /home/cvs/LINUX/git-kernel/linux-2.6/arch/parisc/kernel/syscall.S:866: > Error: Invalid operands > /home/cvs/LINUX/git-kernel/linux-2.6/arch/parisc/kernel/syscall.S:870: > Error: Invalid operands > > Line 866 is the jump label 18: > 18: ldd,ma 0(%sr3,%r26), %r29 > > Line 870 is 19: > 19: ldd,ma 4(%sr3,%r26), %r29 > > I think both should be ldw ? Indeed, fixed in the new patch attached. > An idea: > Maybe the config option CONFIG_PA8X00 (which enables -march=2.0) can be > used in > some places to use the 64bit assembler even on 32bit kernel (instead > of using CONFIG_64BIT) ? Indeed, this is definitely a good idea. But it should probably be a separate patch since we need to touch the existing code to enable wide mode when entering the LWS. > >> My approach for 64bit CAS on 32bit is be the following : >> - Load old into 2 registers >> - Compare low and high part and bail out if different >> - Load new into a FPU register >> - Store the content of the FPU register to the memory >> >> The point here being to do the store in the last step in a single >> instruction. >> I think the same approach can be used for 128bit CAS as well but I >> don't think it's needed at the moment. > > Since the 64bit CAS on 32bit currently uses 9 asms, it can't be added > as is right now anyway. > Maybe it makes sense to pull out the "flddx 0(%sr3,%r24), %fr4" from > this content and to preload > it to where you set up r22/r23-high/low ? Good point, I changed that too. However, having 9 asm isns shouldn't be a problem there because we won't jump past that. >> Regading the GCC counterpart of the implementation, I'm not sure about >> the way to proceed. >> >> Should I try to detect the presence of the new LWS and use it for all >> CAS operations at init time ? > > I leave this up to Dave & Carlos to answer. > >> So far I only used the new LWS for 64bit CAS. > > + /*************************************************** > + New CAS implementation which uses pointers and > variable size information. > + The value pointed by old and new MUST NOT change while > performing CAS. > + The lock only protect the value at %r26. > + > + %r26 - Address to examine > + %r25 - Pointer to the value to check (old) > + %r24 - Pointer to the value to set (new) > + %r23 - Size of the variable (8bit = 0, 16bit = 1, > 32bit = 2, 64bit = 4) > > Since you shift %r23 in your code, I think the comment above is wrong > for 64bit which should be 3 instead of 4 ? Fixed. > You use nop's in your code to align to 32 bytes to be able to jump. > Does it make sense to use .align 32 instead ? I'm not sure myself about > that... I don't know either, nop seemed like the easiest and cleanest way to do so for me. > Should we maybe drop the whole ENABLE_LWS_DEBUG thing? Was it ever > used/enabled? Indeed, I have not tested it and I dropped it in this new patch. >> I guess that using the new LWS unconditionally for all CAS operations >> isn't an option since it will break for newer gcc on old kernels. > > Up to now we only had the 32bit CAS working correctly, so we shouldn't > care much > about the other CAS anyway. > And if we get it backported into all relevant kernels before we change > gcc > I would prefer this hard break... 32bit CAS works but as far as I understand, 8 and 16 bit is broken since the stw in the current LWS will overwrite 3 and 2 bytes of memory respectively with zeros. So IMHO this should definitely be changed. Guy diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S index 8387860..d565e1f 100644 --- a/arch/parisc/kernel/syscall.S +++ b/arch/parisc/kernel/syscall.S @@ -74,7 +74,7 @@ ENTRY(linux_gateway_page) /* ADDRESS 0xb0 to 0xb8, lws uses two insns for entry */ /* Light-weight-syscall entry must always be located at 0xb0 */ /* WARNING: Keep this number updated with table size changes */ -#define __NR_lws_entries (2) +#define __NR_lws_entries (3) lws_entry: gate lws_start, %r0 /* increase privilege */ @@ -502,7 +502,7 @@ lws_exit: /*************************************************** - Implementing CAS as an atomic operation: + Implementing 32bit CAS as an atomic operation: %r26 - Address to examine %r25 - Old value to check (old) @@ -659,6 +659,239 @@ cas_action: ASM_EXCEPTIONTABLE_ENTRY(2b-linux_gateway_page, 3b-linux_gateway_page) + /*************************************************** + New CAS implementation which uses pointers and variable size information. + The value pointed by old and new MUST NOT change while performing CAS. + The lock only protect the value at %r26. + + %r26 - Address to examine + %r25 - Pointer to the value to check (old) + %r24 - Pointer to the value to set (new) + %r23 - Size of the variable (8bit = 0, 16bit = 1, 32bit = 2, 64bit = 3) + %r28 - Return non-zero on failure + %r21 - Kernel error code + + If debugging is DISabled: + + %r21 has the following meanings: + + EAGAIN - CAS is busy, ldcw failed, try again. + EFAULT - Read or write failed. + + If debugging is enabled: + + EDEADLOCK - CAS called recursively. + EAGAIN && r28 == 1 - CAS is busy. Lock contended. + EAGAIN && r28 == 2 - CAS is busy. ldcw failed. + EFAULT - Read or write failed. + + Scratch: r20, r22, r28, r29, r1, fr4 (32bit for 64bit CAS only) + + ****************************************************/ + + /* ELF32 Process entry path */ +lws_compare_and_swap_2: +#ifdef CONFIG_64BIT + /* Clip the input registers */ + depdi 0, 31, 32, %r26 + depdi 0, 31, 32, %r25 + depdi 0, 31, 32, %r24 + depdi 0, 31, 32, %r23 +#endif + + /* Check the validity of the size pointer */ + subi,>>= 4, %r23, %r0 + b,n lws_exit_nosys + + /* Jump to the functions which will load the old and new values into + registers depending on the their size */ + shlw %r23, 2, %r29 + blr %r29, %r0 + nop + + /* 8bit load */ +4: ldb 0(%sr3,%r25), %r25 + b cas2_lock_start +5: ldb 0(%sr3,%r24), %r24 + nop + nop + nop + nop + nop + + /* 16bit load */ +6: ldh 0(%sr3,%r25), %r25 + b cas2_lock_start +7: ldh 0(%sr3,%r24), %r24 + nop + nop + nop + nop + nop + + /* 32bit load */ +8: ldw 0(%sr3,%r25), %r25 + b cas2_lock_start +9: ldw 0(%sr3,%r24), %r24 + nop + nop + nop + nop + nop + + /* 64bit load */ +#ifdef CONFIG_64BIT +10: ldd 0(%sr3,%r25), %r25 +11: ldd 0(%sr3,%r24), %r24 +#else + /* Load new value into r22/r23 - high/low */ +10: ldw 0(%sr3,%r25), %r22 +11: ldw 4(%sr3,%r25), %r23 + /* Load new value into fr4 for atomic store later */ +12: flddx 0(%sr3,%r24), %fr4 +#endif + +cas2_lock_start: + /* Load start of lock table */ + ldil L%lws_lock_start, %r20 + ldo R%lws_lock_start(%r20), %r28 + + /* Extract four bits from r26 and hash lock (Bits 4-7) */ + extru %r26, 27, 4, %r20 + + /* Find lock to use, the hash is either one of 0 to + 15, multiplied by 16 (keep it 16-byte aligned) + and add to the lock table offset. */ + shlw %r20, 4, %r20 + add %r20, %r28, %r20 + + rsm PSW_SM_I, %r0 /* Disable interrupts */ + /* COW breaks can cause contention on UP systems */ + LDCW 0(%sr2,%r20), %r28 /* Try to acquire the lock */ + cmpb,<>,n %r0, %r28, cas2_action /* Did we get it? */ +cas2_wouldblock: + ldo 2(%r0), %r28 /* 2nd case */ + ssm PSW_SM_I, %r0 + b lws_exit /* Contended... */ + ldo -EAGAIN(%r0), %r21 /* Spin in userspace */ + + /* + prev = *addr; + if ( prev == old ) + *addr = new; + return prev; + */ + + /* NOTES: + This all works becuse intr_do_signal + and schedule both check the return iasq + and see that we are on the kernel page + so this process is never scheduled off + or is ever sent any signal of any sort, + thus it is wholly atomic from usrspaces + perspective + */ +cas2_action: + /* Jump to the correct function */ + blr %r29, %r0 + /* Set %r28 as non-zero for now */ + ldo 1(%r0),%r28 + + /* 8bit CAS */ +13: ldb,ma 0(%sr3,%r26), %r29 + sub,= %r29, %r25, %r0 + b,n cas2_end +14: stb,ma %r24, 0(%sr3,%r26) + b cas2_end + copy %r0, %r28 + nop + nop + + /* 16bit CAS */ +15: ldh,ma 0(%sr3,%r26), %r29 + sub,= %r29, %r25, %r0 + b,n cas2_end +16: sth,ma %r24, 0(%sr3,%r26) + b cas2_end + copy %r0, %r28 + nop + nop + + /* 32bit CAS */ +17: ldw,ma 0(%sr3,%r26), %r29 + sub,= %r29, %r25, %r0 + b,n cas2_end +18: stw,ma %r24, 0(%sr3,%r26) + b cas2_end + copy %r0, %r28 + nop + nop + + /* 64bit CAS */ +#ifdef CONFIG_64BIT +19: ldd,ma 0(%sr3,%r26), %r29 + sub,= %r29, %r25, %r0 + b,n cas2_end +20: std,ma %r24, 0(%sr3,%r26) + copy %r0, %r28 +#else + /* Compare first word */ +19: ldw,ma 0(%sr3,%r26), %r29 + sub,= %r29, %r22, %r0 + b,n cas2_end + /* Compare second word */ +20: ldw,ma 4(%sr3,%r26), %r29 + sub,= %r29, %r23, %r0 + b,n cas2_end + /* Perform the store */ +21: fstdx %fr4, 0(%sr3,%r26) + copy %r0, %r28 +#endif + +cas2_end: + /* Free lock */ + stw,ma %r20, 0(%sr2,%r20) + /* Enable interrupts */ + ssm PSW_SM_I, %r0 + /* Return to userspace, set no error */ + b lws_exit + copy %r0, %r21 + +22: + /* Error occurred on load or store */ + /* Free lock */ + stw %r20, 0(%sr2,%r20) + ssm PSW_SM_I, %r0 + ldo 1(%r0),%r28 + b lws_exit + ldo -EFAULT(%r0),%r21 /* set errno */ + nop + nop + nop + + /* Exception table entries, for the load and store, return EFAULT. + Each of the entries must be relocated. */ + ASM_EXCEPTIONTABLE_ENTRY(4b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(5b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(6b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(7b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(8b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(9b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(10b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(11b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(13b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(14b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(15b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(16b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(17b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(18b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(19b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(20b-linux_gateway_page, 22b-linux_gateway_page) +#ifndef CONFIG_64BIT + ASM_EXCEPTIONTABLE_ENTRY(12b-linux_gateway_page, 22b-linux_gateway_page) + ASM_EXCEPTIONTABLE_ENTRY(21b-linux_gateway_page, 22b-linux_gateway_page) +#endif + /* Make sure nothing else is placed on this page */ .align PAGE_SIZE END(linux_gateway_page) @@ -675,8 +908,9 @@ ENTRY(end_linux_gateway_page) /* Light-weight-syscall table */ /* Start of lws table. */ ENTRY(lws_table) - LWS_ENTRY(compare_and_swap32) /* 0 - ELF32 Atomic compare and swap */ - LWS_ENTRY(compare_and_swap64) /* 1 - ELF64 Atomic compare and swap */ + LWS_ENTRY(compare_and_swap32) /* 0 - ELF32 Atomic 32bit compare and swap */ + LWS_ENTRY(compare_and_swap64) /* 1 - ELF64 Atomic 32bit compare and swap */ + LWS_ENTRY(compare_and_swap_2) /* 2 - ELF32 Atomic 64bit compare and swap */ END(lws_table) /* End of lws table */