All of lore.kernel.org
 help / color / mirror / Atom feed
From: will.deacon@arm.com (Will Deacon)
To: linux-arm-kernel@lists.infradead.org
Subject: [RFC] arm64:change jump_label to use branch instruction, not use NOP instr
Date: Fri, 31 Jul 2015 11:14:32 +0100	[thread overview]
Message-ID: <20150731101432.GA29497@arm.com> (raw)
In-Reply-To: <20150731093355.GU25159@twins.programming.kicks-ass.net>

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

WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will.deacon@arm.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: yalin wang <yalin.wang2010@gmail.com>,
	Catalin Marinas <Catalin.Marinas@arm.com>,
	Ingo Molnar <mingo@kernel.org>,
	"anton@samba.org" <anton@samba.org>,
	Mark Rutland <Mark.Rutland@arm.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	open list <linux-kernel@vger.kernel.org>,
	"jbaron@akamai.com" <jbaron@akamai.com>
Subject: Re: [RFC] arm64:change jump_label to use branch instruction, not use NOP instr
Date: Fri, 31 Jul 2015 11:14:32 +0100	[thread overview]
Message-ID: <20150731101432.GA29497@arm.com> (raw)
In-Reply-To: <20150731093355.GU25159@twins.programming.kicks-ass.net>

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

  reply	other threads:[~2015-07-31 10:14 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-31  7:41 [RFC] arm64:change jump_label to use branch instruction, not use NOP instr yalin wang
2015-07-31  7:41 ` yalin wang
2015-07-31  7:52 ` Peter Zijlstra
2015-07-31  7:52   ` Peter Zijlstra
2015-07-31  9:25   ` yalin wang
2015-07-31  9:25     ` yalin wang
2015-07-31  9:33     ` Peter Zijlstra
2015-07-31  9:33       ` Peter Zijlstra
2015-07-31 10:14       ` Will Deacon [this message]
2015-07-31 10:14         ` Will Deacon
2015-08-01 10:00         ` yalin wang
2015-08-01 10:00           ` yalin wang

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=20150731101432.GA29497@arm.com \
    --to=will.deacon@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.