From patchwork Sat Aug 1 10:00:23 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: yalin wang X-Patchwork-Id: 6922131 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 258C49F39B for ; Sat, 1 Aug 2015 10:04:45 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0CA5A2053C for ; Sat, 1 Aug 2015 10:04:44 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5607820532 for ; Sat, 1 Aug 2015 10:04:42 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZLTbd-0007QM-QJ; Sat, 01 Aug 2015 10:01:29 +0000 Received: from casper.infradead.org ([2001:770:15f::2]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZLTbB-0007Ps-AE for linux-arm-kernel@bombadil.infradead.org; Sat, 01 Aug 2015 10:01:01 +0000 Received: from mail-pa0-x22f.google.com ([2607:f8b0:400e:c03::22f]) by casper.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZLTb7-0006zl-Q2 for linux-arm-kernel@lists.infradead.org; Sat, 01 Aug 2015 10:00:59 +0000 Received: by pachj5 with SMTP id hj5so54662698pac.3 for ; Sat, 01 Aug 2015 03:00:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=content-type:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=2/71cEQnCxzhLsvuBeiwDC1Bg74cM6FKFcuYnbhIoJk=; b=CLpWT6wtSzPK6zjwhg6wezbPjV7eM25ZYi3o+qVh2zHfWhImBOI6jTlsE/Om2axLy9 Goako7dfJu+JAx4W7je8fla11RUWUwf/Nh1G38zJr6shatpabfyAoE9lIgMICScNlMix krBASCrMd90L7ZE3h4oOEcFlKkTzwnpf9QKo93+RKWXCDonbvga+lcF98tBLLTtJddYF lgbgV7ql6oHE558VffRH+qvBSljEIWNf7Uq8v3gBLsWOAINjlRHbEL37F7bAmFoK4Bn6 /x28TkRaGNk64NYs200x0iDLmvVao+bLsidtnFwA4DdSsJPfpUai9HS5BGwIob7gJ3Rs dQwQ== X-Received: by 10.66.139.193 with SMTP id ra1mr16409209pab.131.1438423231715; Sat, 01 Aug 2015 03:00:31 -0700 (PDT) Received: from [17.87.20.100] ([17.87.20.100]) by smtp.gmail.com with ESMTPSA id kn8sm11115952pdb.67.2015.08.01.03.00.28 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Sat, 01 Aug 2015 03:00:31 -0700 (PDT) Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2102\)) Subject: Re: [RFC] arm64:change jump_label to use branch instruction, not use NOP instr From: yalin wang In-Reply-To: <20150731101432.GA29497@arm.com> Date: Sat, 1 Aug 2015 18:00:23 +0800 Message-Id: <33D3C3BF-C8E7-4423-8062-DA83EF826872@gmail.com> References: <94E2034B-9DE4-4ADF-8B2F-197D38A363CF@gmail.com> <20150731075259.GR25159@twins.programming.kicks-ass.net> <8F6A71B0-C7DB-4505-BEE2-79967FC91A8F@gmail.com> <20150731093355.GU25159@twins.programming.kicks-ass.net> <20150731101432.GA29497@arm.com> To: Will Deacon X-Mailer: Apple Mail (2.2102) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150801_110058_031896_901D29DE X-CRM114-Status: GOOD ( 32.17 ) X-Spam-Score: -2.5 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Peter Zijlstra , Catalin Marinas , open list , "jbaron@akamai.com" , "anton@samba.org" , Ingo Molnar , "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable 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 > ? 2015?7?31??18:14?Will Deacon ??? > > On Fri, Jul 31, 2015 at 10:33:55AM +0100, Peter Zijlstra wrote: >> On Fri, Jul 31, 2015 at 05:25:02PM +0800, yalin wang wrote: >>>> On Jul 31, 2015, at 15:52, Peter Zijlstra wrote: >>>> On Fri, Jul 31, 2015 at 03:41:37PM +0800, yalin wang wrote: >>>>> This change a little arch_static_branch(), use b . + 4 for false >>>>> return, why? According to aarch64 TRM, if both source and dest >>>>> instr are branch instr, can patch the instr directly, don't need >>>>> all cpu to do ISB for sync, this means we can call >>>>> aarch64_insn_patch_text_nosync() during patch_text(), >>>>> will improve the performance when change a static_key. >>>> >>>> This doesn't parse.. What? >>>> >>>> Also, this conflicts with the jump label patches I've got. >>> >>> this is arch depend , you can see aarch64_insn_patch_text( ) for more info, >>> if aarch64_insn_hotpatch_safe() is true, will patch the text directly. >> >> So I patched all arches, including aargh64. >> >>> what is your git branch base? i make the patch based on linux-next branch, >>> maybe a little delay than yours , could you share your branch git address? >>> i can make a new based on yours . >> >> https://git.kernel.org/cgit/linux/kernel/git/peterz/queue.git/log/?h=locking/jump_label >> >> Don't actually use that branch for anything permanent, this is throw-away >> git stuff. >> >> But you're replacing a NOP with an unconditional branch to the next >> instruction? I suppose I'll leave that to Will and co.. I just had >> trouble understanding your Changelog -- also I was very much not awake >> yet. > > Optimising the (hopefully rare) patching operation but having a potential > impact on the runtime code (assumedly a hotpath) seems completely backwards > to me. > > Yalin, do you have a reason for this change or did you just notice that > paragraph in the architecture and decide to apply it here? > in fact, i don’t have any special reason to must change like this, just found we can do this when i read AARCH64 TRM, then i make this patch :) > Even then, I think there are technical issues with the proposal, since > we could get spurious execution of the old code without explicitsynchronisation (see the kick_all_cpus_sync() call in > aarch64_insn_patch_text). i think jump_label code don’t have responsibility to make sure the sync with other cores, if it is not safe to execute old and new patch code on different cores, the caller should do this sync, he can cancel a work_struct / cu_sync() or some thing like this, i have a look at the software implementation (!HAVE_JUMP_LABEL) , it also doesn’t do any sync, just atomic_inc() and return directly . if the ARCH technology concern is not a matter, i think we can apply it :) in fact , i have another solution for jump_label, i see we calculate the jump instruction every time , why not let the compiler do this during compile time , during run time , we just need swap it with NOP instruction, and by this method, we don’t need store target address in struct jump_entry , it can save some space , this is my patch for this method : — yalin@ubuntu:~/linux-next$ git diff i just store a offset relative *key address and store a NOP in jump_entry, when need change a static_key, we just need swap the jump insert and NOP instr . the jump_entry is shrieked to u64[2] , save some space . Thanks diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h index 1b5e0e8..c040cd3 100644 --- a/arch/arm64/include/asm/jump_label.h +++ b/arch/arm64/include/asm/jump_label.h @@ -28,16 +28,17 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool branch) { - asm goto("1: nop\n\t" + asm goto("1: b %l[l_no]\n\t" ".pushsection __jump_table, \"aw\"\n\t" ".align 3\n\t" - ".quad 1b, %l[l_yes], %c0\n\t" + ".word %c0 - 1b\n\t" + "nop\n\t" + ".quad %c0\n\t" ".popsection\n\t" - : : "i"(&((char *)key)[branch]) : : l_yes); - - return false; -l_yes: + : : "i"(&((char *)key)[branch]) : : l_no); return true; +l_no: + return false; } static __always_inline bool arch_static_branch_jump(struct static_key *key, bool branch) @@ -45,10 +46,11 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool asm goto("1: b %l[l_yes]\n\t" ".pushsection __jump_table, \"aw\"\n\t" ".align 3\n\t" - ".quad 1b, %l[l_yes], %c0\n\t" + ".word %c0 - 1b\n\t" + "nop\n\t" + ".quad %c0\n\t" ".popsection\n\t" : : "i"(&((char *)key)[branch]) : : l_yes); - return false; l_yes: return true; @@ -57,8 +59,8 @@ l_yes: typedef u64 jump_label_t; struct jump_entry { - jump_label_t code; - jump_label_t target; + s32 offset; + u32 insn; jump_label_t key; ...skipping... @@ -45,10 +46,11 @@ static __always_inline bool arch_static_branch_jump(struct static_key *key, bool asm goto("1: b %l[l_yes]\n\t" ".pushsection __jump_table, \"aw\"\n\t" ".align 3\n\t" - ".quad 1b, %l[l_yes], %c0\n\t" + ".word %c0 - 1b\n\t" + "nop\n\t" + ".quad %c0\n\t" ".popsection\n\t" : : "i"(&((char *)key)[branch]) : : l_yes); - return false; l_yes: return true; @@ -57,8 +59,8 @@ l_yes: typedef u64 jump_label_t; struct jump_entry { - jump_label_t code; - jump_label_t target; + s32 offset; + u32 insn; jump_label_t key; }; diff --git a/arch/arm64/kernel/jump_label.c b/arch/arm64/kernel/jump_label.c index c2dd1ad..2e0e7bc 100644 --- a/arch/arm64/kernel/jump_label.c +++ b/arch/arm64/kernel/jump_label.c @@ -25,17 +25,10 @@ void arch_jump_label_transform(struct jump_entry *entry, enum jump_label_type type) { - void *addr = (void *)entry->code; - u32 insn; - - if (type == JUMP_LABEL_JMP) { - insn = aarch64_insn_gen_branch_imm(entry->code, - entry->target, - AARCH64_INSN_BRANCH_NOLINK); - } else { - insn = aarch64_insn_gen_nop(); - } - + void *addr = (void *)entry->key - entry->offset; + u32 old = *(u32*)addr; + u32 insn = entry->insn; + entry->insn = old; aarch64_insn_patch_text(&addr, &insn, 1); } ---