| Message ID | 94E2034B-9DE4-4ADF-8B2F-197D38A363CF@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
> On Jul 31, 2015, at 15:52, Peter Zijlstra <peterz@infradead.org> 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. 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 . Thanks
On Fri, Jul 31, 2015 at 05:25:02PM +0800, yalin wang wrote: > > > On Jul 31, 2015, at 15:52, Peter Zijlstra <peterz@infradead.org> 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.
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 <peterz@infradead.org> 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? Even then, I think there are technical issues with the proposal, since we could get spurious execution of the old code without explicit synchronisation (see the kick_all_cpus_sync() call in aarch64_insn_patch_text). Will
diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h index c0e5165..25b1668 100644 --- a/arch/arm64/include/asm/jump_label.h +++ b/arch/arm64/include/asm/jump_label.h @@ -28,7 +28,7 @@ static __always_inline bool arch_static_branch(struct static_key *key) { - asm goto("1: nop\n\t" + asm goto("1: b . + " __stringify(AARCH64_INSN_SIZE) "\n\t" ".pushsection __jump_table, \"aw\"\n\t" ".align 3\n\t" ".quad 1b, %l[l_yes], %c0\n\t" diff --git a/arch/arm64/kernel/jump_label.c b/arch/arm64/kernel/jump_label.c index 4f1fec7..eb09868 100644 --- a/arch/arm64/kernel/jump_label.c +++ b/arch/arm64/kernel/jump_label.c @@ -28,13 +28,15 @@ void arch_jump_label_transform(struct jump_entry *entry, void *addr = (void *)entry->code; u32 insn; - if (type == JUMP_LABEL_ENABLE) { + if (type == JUMP_LABEL_ENABLE) insn = aarch64_insn_gen_branch_imm(entry->code, - entry->target, - AARCH64_INSN_BRANCH_NOLINK); - } else { - insn = aarch64_insn_gen_nop(); - } + entry->target, + AARCH64_INSN_BRANCH_NOLINK); + else + insn = aarch64_insn_gen_branch_imm(entry->code, + (unsigned long)addr + AARCH64_INSN_SIZE, + AARCH64_INSN_BRANCH_NOLINK); + aarch64_insn_patch_text(&addr, &insn, 1); }
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. Signed-off-by: yalin wang <yalin.wang2010@gmail.com> --- arch/arm64/include/asm/jump_label.h | 2 +- arch/arm64/kernel/jump_label.c | 14 ++++++++------ 2 files changed, 9 insertions(+), 7 deletions(-)