ARM: Use udiv/sdiv for __aeabi_{u}idiv library functions
Subsystems (3)
v2 cover ARM: Use udiv/sdiv for __aeabi_{u}idiv library functions
This is a respin of a patch series from about a year ago[1]. I realized that we already had most of the code in recordmcount to figure out where we make calls to particular functions, so recording where we make calls to the integer division functions should be easy enough to add support for using the same design. Looking back on the thread it seems like Mans was thinking along the same lines, although it wasn't obvious to me back then or even over the last few days when I wrote this.
This series copies and reworks recordmcount to record the locations of the calls to the library integer division functions on ARM builds, and puts those locations into a table that we use to patch instructions at boot. The first patch adds the recorduidiv program, and the second patch implements the runtime patching for modules and kernel code. The module part hooks into the relocation patching code we already have.
The discussion surrounding adding ARMv7VE so that this patchset isn't as necessary is still on going, so I'm just sending out these patches for now. Assuming we can come to a conclusion on how to implement ARMv7VE I'll finish up those patches.
Comments/feedback appreciated.
Changes from v1:
* Add suport for PJ4 udiv/sdiv mrc opcodes
* Create a new program for recording uidiv instead of modifying recordmcount
[1] http://lkml.kernel.org/r/1383951632-6090-1-git-send-email-sboyd at codeaurora.org
Stephen Boyd (2):
scripts: Add a recorduidiv program
ARM: Replace calls to __aeabi_{u}idiv with udiv/sdiv instructions
arch/arm/Kconfig | 14 + arch/arm/include/asm/cputype.h | 8 +- arch/arm/include/asm/setup.h | 3 + arch/arm/kernel/module.c | 40 +++ arch/arm/kernel/setup.c | 68 ++++ arch/arm/kernel/vmlinux.lds.S | 13 + scripts/Makefile | 1 + scripts/Makefile.build | 17 +- scripts/recorduidiv.c | 745 +++++++++++++++++++++++++++++++++++++++++ 9 files changed, 906 insertions(+), 3 deletions(-) create mode 100644 scripts/recorduidiv.c
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v2 1/2 scripts: Add a recorduidiv program
The ARM compiler inserts calls to __aeabi_uidiv() and __aeabi_idiv() when it needs to perform division on signed and unsigned integers. If a processor has support for the udiv and sdiv division instructions the calls to these support routines can be replaced with those instructions. Therefore, record the location of calls to these library functions into two sections (one for udiv and one for sdiv) similar to how we trace calls to mcount. When the kernel boots up it will check to see if the processor supports the instructions and then patch the call sites with the instruction.
--- scripts/Makefile | 1 + scripts/Makefile.build | 17 +- scripts/recorduidiv.c | 745 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 761 insertions(+), 2 deletions(-) create mode 100644 scripts/recorduidiv.c
scripts/Makefile +1 -0
@@ -15,6 +15,7 @@ hostprogs-$(CONFIG_KALLSYMS) += kallsyms hostprogs-$(CONFIG_LOGO) += pnmtologo hostprogs-$(CONFIG_VT) += conmakehash hostprogs-$(BUILD_C_RECORDMCOUNT) += recordmcount +hostprogs-$(CONFIG_ARM_PATCH_UIDIV) += recorduidiv hostprogs-$(CONFIG_BUILDTIME_EXTABLE_SORT) += sortextable hostprogs-$(CONFIG_ASN1) += asn1_compiler hostprogs-$(CONFIG_MODULE_SIG) += sign-file
scripts/Makefile.build +15 -2
@@ -241,12 +241,25 @@ cmd_record_mcount = \ fi; endif +ifdef CONFIG_ARM_PATCH_UIDIV +# Due to recursion, we must skip empty.o. +# The empty.o file is created in the make process in order to determine +# the target endianness and word size. It is made before all other C +# files, including recorduidiv. +cmd_record_uidiv = \ + if [ $(@) != "scripts/mod/empty.o" ]; then \ + $(objtree)/scripts/recorduidiv "$(@)"; \ + fi; +recorduidiv_source := $(srctree)/scripts/recorduidiv.c +endif + define rule_cc_o_c $(call echo-cmd,checksrc) $(cmd_checksrc) \ $(call echo-cmd,cc_o_c) $(cmd_cc_o_c); \ $(cmd_modversions) \ $(call echo-cmd,record_mcount) \ $(cmd_record_mcount) \ + $(call echo-cmd,record_uidiv) $(cmd_record_uidiv) \ scripts/basic/fixdep $(depfile) $@ '$(call make-cmd,cc_o_c)' > \ $(dot-target).tmp; \ rm -f $(depfile); \
@@ -254,13 +267,13 @@ define rule_cc_o_c endef # Built-in and composite module parts -$(obj)/%.o: $(src)/%.c $(recordmcount_source) FORCE +$(obj)/%.o: $(src)/%.c $(recordmcount_source) $(recorduidiv_source) FORCE $(call cmd,force_checksrc) $(call if_changed_rule,cc_o_c) # Single-part modules are special since we need to mark them in $(MODVERDIR) -$(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) FORCE +$(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(recorduidiv_source) FORCE $(call cmd,force_checksrc) $(call if_changed_rule,cc_o_c) @{ echo $(@:.o=.ko); echo $@; } > $(MODVERDIR)/$(@F:.o=.mod)
scripts/recorduidiv.c new file +745 -0
745 lines not on this page · raw message
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
Quoted 10 lines · Stephen Boyd
The ARM compiler inserts calls to
__aeabi_uidiv()and__aeabi_idiv()when it needs to perform division on signed and unsigned integers. If a processor has support for the udiv and sdiv division instructions the calls to these support routines can be replaced with those instructions. Therefore, record the location of calls to these library functions into two sections (one for udiv and one for sdiv) similar to how we trace calls to mcount. When the kernel boots up it will check to see if the processor supports the instructions and then patch the call sites with the instruction.
Do we have any resolution on these programs which modify the object files in-place, rather than breaking any hard-links which may be present (eg, as a result of using ccache in hard-link mode) ?
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
Quoted 12 lines · Russell King - ARM Linux
Quoted 10 lines · Stephen Boyd
The ARM compiler inserts calls to
__aeabi_uidiv()and__aeabi_idiv()when it needs to perform division on signed and unsigned integers. If a processor has support for the udiv and sdiv division instructions the calls to these support routines can be replaced with those instructions. Therefore, record the location of calls to these library functions into two sections (one for udiv and one for sdiv) similar to how we trace calls to mcount. When the kernel boots up it will check to see if the processor supports the instructions and then patch the call sites with the instruction.
Quoted 3 lines
Do we have any resolution on these programs which modify the object files in-place, rather than breaking any hard-links which may be present (eg, as a result of using ccache in hard-link mode) ?
Good point, but I do not think anybody is using CCACHE_HARDLINK with the kernel. As the manpage says, it is going to confuse make, so the time saved by ccache would be offset by make trying to recompile all *.c files each time.
Michal
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
Quoted 13 lines · Michal Marek
Quoted 12 lines · Russell King - ARM Linux
Quoted 10 lines · Stephen Boyd
The ARM compiler inserts calls to
__aeabi_uidiv()and__aeabi_idiv()when it needs to perform division on signed and unsigned integers. If a processor has support for the udiv and sdiv division instructions the calls to these support routines can be replaced with those instructions. Therefore, record the location of calls to these library functions into two sections (one for udiv and one for sdiv) similar to how we trace calls to mcount. When the kernel boots up it will check to see if the processor supports the instructions and then patch the call sites with the instruction.
Quoted 6 lines
Quoted 3 lines
Do we have any resolution on these programs which modify the object files in-place, rather than breaking any hard-links which may be present (eg, as a result of using ccache in hard-link mode) ?
Good point, but I do not think anybody is using
CCACHE_HARDLINKwith the kernel.
That's wrong then, because I've been using it for a very long time with my nightly builds. :) Therefore, there is somebody!
Quoted 5 lines
As the manpage says, it is going to confuse make, so the time saved by ccache would be offset by make trying to recompile all *.c files each time.
From what I've noticed, it makes a big difference when running nightly
builds. My nightly builds use O= and always build into an empty target tree, so there are no old objects back-dated to confuse make.
Even if there were, make would spot that the object is older than the source, and try to re-make the target again, at which point ccache would re-hardlink the object after looking up the hashed preprocessed source.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
Quoted 18 lines · Russell King - ARM Linux
Quoted 17 lines · Michal Marek
Quoted 15 lines · Russell King - ARM Linux
Quoted 10 lines · Stephen Boyd
The ARM compiler inserts calls to
__aeabi_uidiv()and__aeabi_idiv()when it needs to perform division on signed and unsigned integers. If a processor has support for the udiv and sdiv division instructions the calls to these support routines can be replaced with those instructions. Therefore, record the location of calls to these library functions into two sections (one for udiv and one for sdiv) similar to how we trace calls to mcount. When the kernel boots up it will check to see if the processor supports the instructions and then patch the call sites with the instruction.Do we have any resolution on these programs which modify the object files in-place, rather than breaking any hard-links which may be present (eg, as a result of using ccache in hard-link mode) ?
Quoted 5 lines
Quoted 2 lines
Good point, but I do not think anybody is using
CCACHE_HARDLINKwith the kernel.That's wrong then, because I've been using it for a very long time with my nightly builds. :) Therefore, there is somebody!
OK.
Quoted 4 lines
Quoted 3 lines
As the manpage says, it is going to confuse make, so the time saved by ccache would be offset by make trying to recompile all *.c files each time.
Quoted 8 lines
From what I've noticed, it makes a big difference when running nightly builds. My nightly builds use O= and always build into an empty target tree, so there are no old objects back-dated to confuse make.
Even if there were, make would spot that the object is older than the source, and try to re-make the target again, at which point ccache would re-hardlink the object after looking up the hashed preprocessed source.
That's what I meant. If you do a second make in a freshly built tree, it will recompile all the files again. It will all be cache hits, but each of the will be lot slower than comparing two timestamps. But in throwaway build trees you do not care, I haven't considered that...
Michal
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
Quoted 7 lines · Michal Marek
Quoted 6 lines · Russell King - ARM Linux
Quoted 5 lines · Michal Marek
Quoted 3 lines · Russell King - ARM Linux
Do we have any resolution on these programs which modify the object files in-place, rather than breaking any hard-links which may be present (eg, as a result of using ccache in hard-link mode) ?
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Good point, but I do not think anybody is using
CCACHE_HARDLINKwith the kernel.That's wrong then, because I've been using it for a very long time with my nightly builds. :) Therefore, there is somebody!
OK.
So, both recordmcount and the new recordudiv program are idempotent. They check if the to-be-added section is already present and do nothing. So the result is correct even with CCACHE_HARDLINK, just the intermediate file might be incorrect. If this still is considered an issue, I suggest clearing CCACHE_HARDLINK when using any of these postprocessors, so as not to penalize other use cases.
The perl recordmcount does not modify the file in place.
Michal
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
Quoted 11 lines · Michal Marek
Quoted 10 lines · Michal Marek
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Michal Marek
Quoted 3 lines · Russell King - ARM Linux
Do we have any resolution on these programs which modify the object files in-place, rather than breaking any hard-links which may be present (eg, as a result of using ccache in hard-link mode) ?
Good point, but I do not think anybody is using
CCACHE_HARDLINKwith the kernel.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
That's wrong then, because I've been using it for a very long time with my nightly builds. :) Therefore, there is somebody!
OK.
So, both recordmcount and the new recordudiv program are idempotent. They check if the to-be-added section is already present and do nothing.
They hardly "do nothing", as the (eg) recordmcount plasters the build log with warnings. A solution to that would be to make recordmcount silent if the section is already present.
Quoted 4 lines
So the result is correct even with
CCACHE_HARDLINK, just the intermediate file might be incorrect. If this still is considered an issue, I suggest clearingCCACHE_HARDLINKwhen using any of these postprocessors, so as not to penalize other use cases.
Another solution would be to have the top level make file unset the CCACHE_HARDLINK environment variable if any of the options which enable in-place editing of object files is enabled. Looking at the ccache code, the environment variable has to be deleted from the environment to turn off the option - and I'm not sure whether make can delete environment variables. It certainly can override them, but I see nothing in the info pages which suggests that environment variables can be deleted by a makefile.
However, doing it outside of the kernel build system is likely error prone especially as the kernel configuration options change and/or their effect changes.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
Quoted 7 lines · Russell King - ARM Linux
Quoted 2 lines · Michal Marek
So, both recordmcount and the new recordudiv program are idempotent. They check if the to-be-added section is already present and do nothing.
They hardly "do nothing", as the (eg) recordmcount plasters the build log with warnings. A solution to that would be to make recordmcount silent if the section is already present.
Right, there is a warning. The recorduidiv program exits silently.
Quoted 8 lines
Quoted 4 lines
So the result is correct even with
CCACHE_HARDLINK, just the intermediate file might be incorrect. If this still is considered an issue, I suggest clearingCCACHE_HARDLINKwhen using any of these postprocessors, so as not to penalize other use cases.Another solution would be to have the top level make file unset the
CCACHE_HARDLINKenvironment variable if any of the options which enable in-place editing of object files is enabled.
This is what I meant, sorry for not being clear.
Quoted 6 lines
Looking at the ccache
code, the environment variable has to be deleted from the environment to turn off the option - and I'm not sure whether make can delete environment variables. It certainly can override them, but I see nothing in the info pages which suggests that environment variables can be deleted by a makefile.
unexport CCACHE_HARDLINK
will do the trick.
Michal
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
In reply to Russell King - ARM LinuxQuoted 3 lines · On Tue, 1 Dec 2015 16:19:44 +0000 Russell King - ARM Linux…
They hardly "do nothing", as the (eg) recordmcount plasters the build log with warnings. A solution to that would be to make recordmcount silent if the section is already present.
Note, that warning found plenty of bugs when modifications of the build system was being done and broke recordmcount.c. I really don't want to silent it.
But for some reason, your build is causing lots of warnings and not for others. Perhaps we can add a "SILENT_RECORDMCOUNT" environment variable and have it set when something like CCACHE_HARDLINK or whatever is causing it to trigger when we don't care.
-- Steve
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
Quoted 7 lines · Steven Rostedt
Quoted 3 lines · On Tue, 1 Dec 2015 16:19:44 +0000 Russell King - ARM Linux…
They hardly "do nothing", as the (eg) recordmcount plasters the build log with warnings. A solution to that would be to make recordmcount silent if the section is already present.
Quoted 8 lines
Note, that warning found plenty of bugs when modifications of the build system was being done and broke
recordmcount.c. I really don't want to silent it.But for some reason, your build is causing lots of warnings and not for others. Perhaps we can add a "
SILENT_RECORDMCOUNT" environment variable and have it set when something likeCCACHE_HARDLINKor whatever is causing it to trigger when we don't care.
The case is:
Build 1 runs with CCACHE_HARDLINK enabled.
- Each object ccache creates will be stored in ccache, and hard linked into the throw-away object tree.
- recordmcount modifies in-place the object in the object tree, which also modifies the object in the ccache repository.
The throw-away object tree is thrown away, and a new tree is created, and the build re-run. It doesn't matter what CCACHE options are used, the effect will now be the same:
- Each "hit" ccache object from the previous build will be linked or copied to the new throw-away object tree.
- recordmcount will be re-run on the object, which now contains the results of the previous recordmcount in-place modification. This causes recordmcount to issue a warning.
There's two solutions to this: one is to disable CCACHE_HARDLINK for all kernel builds which use in-place object modification. The other solution is to avoid in-place object modification, instead doing a read-write-rename.
I think I ought to ask another question though, before we decide what to do. With recordmcount doing in-place object modification, what happens if a SIGINT or similar is received half way through the modification of an object? I would hope that make would delete the object and not leave it around.
Another suggestion - maybe recordmcount, which fstat()s the file, should check the st_nlink before modifying the file, and error out with a helpful error message telling people not to use hardlinks, which would stop nasty surprises (and make it a rule that this should be implemented as a general principle for good build behaviour) - iow, something like this (untested):
scripts/recordmcount.c | 4 ++++ 1 file changed, 4 insertions(+)
scripts/recordmcount.c +4 -0
@@ -203,6 +203,10 @@ static void *mmap_file(char const *fname)fprintf (stderr,"not a regular file: %s\n" , fname);fail_file (); } +if (sb.st_nlink !=1 ) { +fprintf (stderr,"file is hard linked: %s\n" , fname); +fail_file (); + } addr =mmap (0 , sb.st_size,PROT_READ |PROT_WRITE ,MAP_PRIVATE , fd_map,0 ); mmap_failed =0 ;
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
Quoted 5 lines · On Tue, 1 Dec 2015 17:10:14 +0000 Russell King - ARM Linux…
Another suggestion - maybe recordmcount, which fstat()s the file, should check the
st_nlinkbefore modifying the file, and error out with a helpful error message telling people not to use hardlinks, which would stop nasty surprises (and make it a rule that this should be implemented as a general principle for good build behaviour) - iow,
Actually I like this solution the best.
Quoted 1 line
something like this (untested):
Can you test it to see if it gives you the error, otherwise I need to set up a CCACHE_HARDLINK environment :-)
I guess another solution is to do a copy instead of modifying in place if it detects the multiple hard link?
-- Steve
Quoted 21 lines
scripts/recordmcount.c | 4 ++++ 1 file changed, 4 insertions(+)scripts/recordmcount.c +4
-0@@ -203,6 +203,10 @@ static void *mmap_file(char const *fname)fprintf (stderr,"not a regular file: %s\n" , fname);fail_file (); } +if (sb.st_nlink !=1 ) { +fprintf (stderr,"file is hard linked: %s\n" , fname); +fail_file (); + } addr =mmap (0 , sb.st_size,PROT_READ |PROT_WRITE ,MAP_PRIVATE , fd_map,0 ); mmap_failed =0 ;
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
Quoted 9 lines · Steven Rostedt
Quoted 5 lines · On Tue, 1 Dec 2015 17:10:14 +0000 Russell King - ARM Linux…
Another suggestion - maybe recordmcount, which fstat()s the file, should check the
st_nlinkbefore modifying the file, and error out with a helpful error message telling people not to use hardlinks, which would stop nasty surprises (and make it a rule that this should be implemented as a general principle for good build behaviour) - iow,
Quoted 6 lines
Actually I like this solution the best.
Quoted 1 line
something like this (untested):
Can you test it to see if it gives you the error, otherwise I need to set up a
CCACHE_HARDLINKenvironment :-)
It does indeed:
CC init/do_mounts_initrd.o
file is hard linked: init/do_mounts_initrd.o /home/rmk/git/linux-rmk/scripts/Makefile.build:258: recipe for target 'init/do_mounts_initrd.o' failed make[2]: *** [init/do_mounts_initrd.o] Error 1
Quoted 2 lines
I guess another solution is to do a copy instead of modifying in place if it detects the multiple hard link?
That would be the "transparent" solution. If you think it's worth persuing, I'll have a go at fixing recordmcount to do that.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
Dne 1.12.2015 v 19:16 Russell King - ARM Linux napsal(a):
Quoted 6 lines · Steven Rostedt
Quoted 2 lines
I guess another solution is to do a copy instead of modifying in place if it detects the multiple hard link?
That would be the "transparent" solution. If you think it's worth persuing, I'll have a go at fixing recordmcount to do that.
But in terms of number of file copies, it would on par with disabling CCACHE_HARDLINK from within the Makefile.
Michal
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
In reply to Russell King - ARM LinuxQuoted 6 lines · Russell King - ARM Linux
Quoted 2 lines · Steven Rostedt
I guess another solution is to do a copy instead of modifying in place if it detects the multiple hard link?
That would be the "transparent" solution. If you think it's worth persuing, I'll have a go at fixing recordmcount to do that.
Well, copying the file is easy - I've tested this and the linker appears happy with the result:
scripts/recordmcount.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
scripts/recordmcount.c +14 -0
@@ -211,6 +211,20 @@ static void *mmap_file(char const *fname) addr =umalloc (sb.st_size);uread (fd_map, addr, sb.st_size); } +if (sb.st_nlink !=1 ) { +/* file is hard-linked, break the hard link */ +close (fd_map); +if (unlink (fname) <0 ) { +perror (fname); +fail_file (); + } + fd_map =open (fname,O_RDWR |O_CREAT , sb.st_mode); +if (fd_map <0 ) { +perror (fname); +fail_file (); + } +uwrite (fd_map, addr, sb.st_size); + }return addr; }
Signature · 2 lines
FTTC broadband for 0.8mile line: [email protected] down 400kbps up according to speedtest.net.
Re: [PATCH v2 1/2] scripts: Add a recorduidiv program
Quoted 10 lines · On Wed, 2 Dec 2015 10:23:39 +0000 Russell King - ARM Linux…
Quoted 6 lines · Russell King - ARM Linux
Quoted 2 lines · Steven Rostedt
I guess another solution is to do a copy instead of modifying in place if it detects the multiple hard link?
That would be the "transparent" solution. If you think it's worth persuing, I'll have a go at fixing recordmcount to do that.
Well, copying the file is easy - I've tested this and the linker appears happy with the result:
Want to make this into a proper patch and I'll start running it through my tests?
-- Steve
Quoted 30 lines
scripts/recordmcount.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)scripts/recordmcount.c +14
-0@@ -211,6 +211,20 @@ static void *mmap_file(char const *fname) addr =umalloc (sb.st_size);uread (fd_map, addr, sb.st_size); } +if (sb.st_nlink !=1 ) { +/* file is hard-linked, break the hard link */ +close (fd_map); +if (unlink (fname) <0 ) { +perror (fname); +fail_file (); + } + fd_map =open (fname,O_RDWR |O_CREAT , sb.st_mode); +if (fd_map <0 ) { +perror (fname); +fail_file (); + } +uwrite (fd_map, addr, sb.st_size); + }return addr; }
PATCH 1/1 scripts: recordmcount: break hardlinks
recordmcount edits the file in-place, which can cause problems when using ccache in hardlink mode. Arrange for recordmcount to break a hardlinked object.
---
Steven, sorry it took a while to get this out...
scripts/recordmcount.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
scripts/recordmcount.c +14 -0
@@ -211,6 +211,20 @@ static void *mmap_file(char const *fname) addr =umalloc (sb.st_size);uread (fd_map, addr, sb.st_size); } +if (sb.st_nlink !=1 ) { +/* file is hard-linked, break the hard link */ +close (fd_map); +if (unlink (fname) <0 ) { +perror (fname); +fail_file (); + } + fd_map =open (fname,O_RDWR |O_CREAT , sb.st_mode); +if (fd_map <0 ) { +perror (fname); +fail_file (); + } +uwrite (fd_map, addr, sb.st_size); + }return addr; }
Signature · 1 line
2.1.0
Re: [PATCH] scripts: recordmcount: break hardlinks
Quoted 7 lines · On Fri, 11 Dec 2015 12:09:03 +0000 Russell King <[email protected]>…
recordmcount edits the file in-place, which can cause problems when using ccache in hardlink mode. Arrange for recordmcount to break a hardlinked object.
Signed-off-by Russell King <[email protected]>---
Steven, sorry it took a while to get this out...
Should this be for stable, or is it fine to just add this to my 4.5 queue?
-- Steve
Quoted 29 lines
scripts/recordmcount.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)scripts/recordmcount.c +14
-0@@ -211,6 +211,20 @@ static void *mmap_file(char const *fname) addr =umalloc (sb.st_size);uread (fd_map, addr, sb.st_size); } +if (sb.st_nlink !=1 ) { +/* file is hard-linked, break the hard link */ +close (fd_map); +if (unlink (fname) <0 ) { +perror (fname); +fail_file (); + } + fd_map =open (fname,O_RDWR |O_CREAT , sb.st_mode); +if (fd_map <0 ) { +perror (fname); +fail_file (); + } +uwrite (fd_map, addr, sb.st_size); + }return addr; }
Re: [PATCH] scripts: recordmcount: break hardlinks
Quoted 7 lines · Steven Rostedt
Quoted 4 lines · On Fri, 11 Dec 2015 12:09:03 +0000 Russell King <[email protected]>…
recordmcount edits the file in-place, which can cause problems when using ccache in hardlink mode. Arrange for recordmcount to break a hardlinked object.
Quoted 6 lines
Quoted 3 lines
Signed-off-by Russell King <[email protected]>---
Steven, sorry it took a while to get this out...Should this be for stable, or is it fine to just add this to my 4.5 queue?
I thought you wanted to test it first - although I've been running with this for a while now, my nightly builds have masked out the mcount warning, and I suspect it'll take a while for ccache to purge itself of the modified objects.
If you're happy to add a stable tag to it, then please do so.
Signature · 3 lines
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH] scripts: recordmcount: break hardlinks
Quoted 8 lines · On Fri, 11 Dec 2015 14:45:41 +0000 Russell King - ARM Linux…
Quoted 7 lines · Steven Rostedt
Quoted 4 lines · On Fri, 11 Dec 2015 12:09:03 +0000 Russell King <[email protected]>…
recordmcount edits the file in-place, which can cause problems when using ccache in hardlink mode. Arrange for recordmcount to break a hardlinked object.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Signed-off-by Russell King <[email protected]>---
Steven, sorry it took a while to get this out...Should this be for stable, or is it fine to just add this to my 4.5 queue?
I thought you wanted to test it first - although I've been running with
You're right. I forgot I said that ;-)
Quoted 6 lines
this for a while now, my nightly builds have masked out the mcount warning, and I suspect it'll take a while for ccache to purge itself of the modified objects.
If you're happy to add a stable tag to it, then please do so.
I'm fine with you taking it too, but let me go ahead and run it through my tests now. I'll let you know the results. Takes several hours.
Thanks,
-- Steve
Steven Rostedt
Reviewed-byRe: [PATCH] scripts: recordmcount: break hardlinks
In reply to Russell King - ARM LinuxQuoted 15 lines · On Fri, 11 Dec 2015 14:45:41 +0000 Russell King - ARM Linux…
Quoted 7 lines · Steven Rostedt
Quoted 4 lines · On Fri, 11 Dec 2015 12:09:03 +0000 Russell King <[email protected]>…
recordmcount edits the file in-place, which can cause problems when using ccache in hardlink mode. Arrange for recordmcount to break a hardlinked object.
Quoted 6 lines
Quoted 3 lines
Signed-off-by Russell King <[email protected]>---
Steven, sorry it took a while to get this out...Should this be for stable, or is it fine to just add this to my 4.5 queue?
Quoted 7 lines
I thought you wanted to test it first - although I've been running with this for a while now, my nightly builds have masked out the mcount warning, and I suspect it'll take a while for ccache to purge itself of the modified objects.
If you're happy to add a stable tag to it, then please do so.
I ran it through most my tests (it's still running and is at 20 of 33 tests). If there was anything wrong with this patch, I'm sure one of my tests would have crashed by now.
Do you want to take it, or shall I?
If you want to take it, you can add my:
-- Steve
Re: [PATCH] scripts: recordmcount: break hardlinks
Quoted 3 lines · Steven Rostedt
I ran it through most my tests (it's still running and is at 20 of 33 tests). If there was anything wrong with this patch, I'm sure one of my tests would have crashed by now.
Thanks for testing.
Quoted 1 line
Do you want to take it, or shall I?
I'm easy - it probably makes more sense if take it.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH] scripts: recordmcount: break hardlinks
Quoted 11 lines · On Fri, 11 Dec 2015 18:33:27 +0000 Russell King - ARM Linux…
Quoted 3 lines · Steven Rostedt
I ran it through most my tests (it's still running and is at 20 of 33 tests). If there was anything wrong with this patch, I'm sure one of my tests would have crashed by now.
Thanks for testing.
Quoted 1 line
Do you want to take it, or shall I?
I'm easy - it probably makes more sense if take it.
Heh, you got me hanging in suspense. You missed a word. Is it:
"makes more sense if *you* take it"
or
"makes more sense if *I* take it"
??
Or you could be replying to my sentence of "take it" or "shall I" in which it would be *you* take it.
/me confused
-- Steve
Re: [PATCH] scripts: recordmcount: break hardlinks
Quoted 17 lines · Steven Rostedt
Quoted 11 lines · On Fri, 11 Dec 2015 18:33:27 +0000 Russell King - ARM Linux…
Quoted 3 lines · Steven Rostedt
I ran it through most my tests (it's still running and is at 20 of 33 tests). If there was anything wrong with this patch, I'm sure one of my tests would have crashed by now.
Thanks for testing.
Quoted 1 line
Do you want to take it, or shall I?
I'm easy - it probably makes more sense if take it.
Heh, you got me hanging in suspense. You missed a word. Is it:
Quoted 7 lines
"makes more sense if *you* take it"
or
"makes more sense if *I* take it"
??
Oops, sorry. "makes more sense if *you* take it" was what I thought I typed!
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH] scripts: recordmcount: break hardlinks
Quoted 3 lines · On Fri, 11 Dec 2015 18:58:09 +0000 Russell King - ARM Linux…
Oops, sorry. "makes more sense if *you* take it" was what I thought I typed!
OK, will do.
Thanks!
-- Steve
PATCH v2 2/2 ARM: Replace calls to __aeabi_{u}idiv with udiv/sdiv instructions
In reply to Stephen BoydThe ARM compiler inserts calls to __aeabi_uidiv() and __aeabi_idiv() when it needs to perform division on signed and unsigned integers. If a processor has support for the udiv and sdiv division instructions the calls to these support routines can be replaced with those instructions. Now that recordmcount records the locations of calls to these library functions in two sections (one for udiv and one for sdiv), iterate over these sections early at boot and patch the call sites with the appropriate division instruction when we determine that the processor supports the division instructions. Using the division instructions should be faster and less power intensive than running the support code.
--- arch/arm/Kconfig | 14 +++++++++ arch/arm/include/asm/cputype.h | 8 ++++- arch/arm/include/asm/setup.h | 3 ++ arch/arm/kernel/module.c | 40 +++++++++++++++++++++++++ arch/arm/kernel/setup.c | 68 ++++++++++++++++++++++++++++++++++++++++++ arch/arm/kernel/vmlinux.lds.S | 13 ++++++++ 6 files changed, 145 insertions(+), 1 deletion(-)
arch/arm/Kconfig +14 -0
@@ -1617,6 +1617,20 @@ config AEABI To use this you need GCC version 4.0.0 or later. +config ARM_PATCH_UIDIV + bool "Runtime patch calls to __aeabi_{u}idiv() with udiv/sdiv" + depends on CPU_32v7 && !XIP_KERNEL && AEABI + help + Some v7 CPUs have support for the udiv and sdiv instructions + that can be used in place of calls to __aeabi_uidiv and __aeabi_idiv + functions provided by the ARM runtime ABI. + + Enabling this option allows the kernel to modify itself to replace + branches to these library functions with the udiv and sdiv + instructions themselves. Typically this will be faster and less + power intensive than running the library support code to do + integer division. + config OABI_COMPAT bool "Allow old ABI binaries to run with this kernel (EXPERIMENTAL)" depends on AEABI && !THUMB2_KERNEL
arch/arm/include/asm/cputype.h +7 -1
@@ -250,8 +250,14 @@ static inline int cpu_is_pj4(void)return 0 ; } + +static inline bool __attribute_const__ cpu_is_pj4_nomp(void) +{ +return read_cpuid_part () ==0x56005810 ; +}#else -#define cpu_is_pj4 () 0 +#define cpu_is_pj4 () 0 +#define cpu_is_pj4_nomp () 0#endif static inline int __attribute_const__ cpuid_feature_extract_field(u32 features,
arch/arm/include/asm/setup.h +3 -0
@@ -25,4 +25,7 @@ extern int arm_add_memory(u64 start, u64 size);extern void early_print (const char *str, ...);extern void dump_machine_table (void ); +extern void patch_udiv (void *addr,size_t size); +extern void patch_sdiv (void *addr,size_t size); +#endif
arch/arm/kernel/module.c +40 -0
@@ -20,7 +20,9 @@#include <linux/string.h> #include <linux/gfp.h> +#include <asm/hwcap.h> #include <asm/pgtable.h> +#include <asm/setup.h> #include <asm/sections.h> #include <asm/smp_plat.h> #include <asm/unwind.h>
@@ -51,6 +53,38 @@ void *module_alloc(unsigned long size) }#endif +#ifdef CONFIG_ARM_PATCH_UIDIV +static int module_patch_aeabi_uidiv (unsigned long loc,const Elf32_Sym *sym) +{ +extern char __aeabi_uidiv[], __aeabi_idiv[]; +unsigned long udiv_addr = (unsigned long )__aeabi_uidiv; +unsigned long sdiv_addr = (unsigned long )__aeabi_idiv; +unsigned int mask; + +if (IS_ENABLED (CONFIG_THUMB2_KERNEL )) + mask =HWCAP_IDIVT ; +else + mask =HWCAP_IDIVA ; + +if (elf_hwcap & mask) { +if (sym->st_value == udiv_addr) { +patch_udiv (&loc,sizeof (loc)); +return 1 ; + }else if (sym->st_value == sdiv_addr) { +patch_sdiv (&loc,sizeof (loc)); +return 1 ; + } + } + +return 0 ; +} +#else +static int module_patch_aeabi_uidiv (unsigned long loc,const Elf32_Sym *sym) +{ +return 0 ; +} +#endif +int apply_relocate (Elf32_Shdr *sechdrs,const char *strtab,unsigned int symindex,unsigned int relindex,struct module *module)
@@ -109,6 +143,9 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,return -ENOEXEC ; } +if (module_patch_aeabi_uidiv (loc, sym)) +break ; + offset =__mem_to_opcode_arm (*(u32 *)loc); offset = (offset &0x00ffffff ) <<2 ;if (offset &0x02000000 )
@@ -195,6 +232,9 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,return -ENOEXEC ; } +if (module_patch_aeabi_uidiv (loc, sym)) +break ; + upper =__mem_to_opcode_thumb16 (*(u16 *)loc); lower =__mem_to_opcode_thumb16 (*(u16 *)(loc +2 ));
arch/arm/kernel/setup.c +68 -0
86 lines not on this page · raw message
arch/arm/kernel/vmlinux.lds.S +13 -0
@@ -28,6 +28,18 @@ *(.hyp.idmap.text) \ VMLINUX_SYMBOL(__hyp_idmap_text_end) = .; +#ifdef CONFIG_ARM_PATCH_UIDIV +#define UIDIV_REC . = ALIGN(8); \ + VMLINUX_SYMBOL(__start_udiv_loc) = .; \ + *(__udiv_loc) \ + VMLINUX_SYMBOL(__stop_udiv_loc) = .; \ + VMLINUX_SYMBOL(__start_idiv_loc) = .; \ + *(__idiv_loc) \ + VMLINUX_SYMBOL(__stop_idiv_loc) = .; +#else +#define UIDIV_REC +#endif + #ifdef CONFIG_HOTPLUG_CPU #define ARM_CPU_DISCARD(x) #define ARM_CPU_KEEP(x) x
@@ -210,6 +222,7 @@ SECTIONS .init.data : { #ifndef CONFIG_XIP_KERNEL INIT_DATA + UIDIV_REC #endif INIT_SETUP(16) INIT_CALLS
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v2 2/2] ARM: Replace calls to __aeabi_{u}idiv with udiv/sdiv instructions
Quoted 4 lines · Stephen Boyd
The ARM compiler inserts calls to
__aeabi_uidiv()and__aeabi_idiv()when it needs to perform division on signed and unsigned integers. If a processor has support for the udiv and sdiv division instructions the calls to these support routines
Quoted 8 lines
can be replaced with those instructions. Now that recordmcount records the locations of calls to these library functions in two sections (one for udiv and one for sdiv), iterate over these sections early at boot and patch the call sites with the appropriate division instruction when we determine that the processor supports the division instructions. Using the division instructions should be faster and less power intensive than running the support code.
A few remarks:
1) The code assumes unconditional branches to __aeabi_idiv and __aeabi_uidiv. What if there are conditional branches? Also, tail call optimizations will generate a straight b opcode rather than a bl and patching those will obviously have catastrophic results. I think you should validate the presence of a bl before patching over it.
2) For those cases where a call to __aeabi_uidiv and __aeabi_idiv is not patched due to (1), you could patch a uidiv/idiv plus "bx lr" at those function entry points too.
3) In fact I was wondering if the overhead of the branch and back is really significant compared to the non trivial cost of a idiv instruction and all the complex infrastructure required to patch those branches directly, and consequently if the performance difference is actually worth it versus simply doing (2) alone.
4) Please add some printing to the boot log (debug level should be fine) about the fact that you did modify n branch instances with a div insn. That _could_ turn out to be a useful clue when debugging kernel "strangeties".
Quoted 274 lines
Cc Nicolas Pitre <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Steven Rostedt <[email protected]>Cc M?ns Rullg?rd <[email protected]>Cc Thomas Petazzoni <[email protected]>Signed-off-by Stephen Boyd <[email protected]>--- arch/arm/Kconfig | 14 +++++++++ arch/arm/include/asm/cputype.h | 8 ++++- arch/arm/include/asm/setup.h | 3 ++ arch/arm/kernel/module.c | 40 +++++++++++++++++++++++++ arch/arm/kernel/setup.c | 68 ++++++++++++++++++++++++++++++++++++++++++ arch/arm/kernel/vmlinux.lds.S | 13 ++++++++ 6 files changed, 145 insertions(+), 1 deletion(-)arch/arm/Kconfig +14
-0@@ -1617,6 +1617,20 @@ config AEABI To use this you need GCC version 4.0.0 or later. +config ARM_PATCH_UIDIV + bool "Runtime patch calls to __aeabi_{u}idiv() with udiv/sdiv" + depends on CPU_32v7 && !XIP_KERNEL && AEABI + help + Some v7 CPUs have support for the udiv and sdiv instructions + that can be used in place of calls to __aeabi_uidiv and __aeabi_idiv + functions provided by the ARM runtime ABI. + + Enabling this option allows the kernel to modify itself to replace + branches to these library functions with the udiv and sdiv + instructions themselves. Typically this will be faster and less + power intensive than running the library support code to do + integer division. + config OABI_COMPAT bool "Allow old ABI binaries to run with this kernel (EXPERIMENTAL)" depends on AEABI && !THUMB2_KERNELarch/arm/include/asm/cputype.h +7
-1@@ -250,8 +250,14 @@ static inline int cpu_is_pj4(void)return 0 ; } + +static inline bool __attribute_const__ cpu_is_pj4_nomp(void) +{ +return read_cpuid_part () ==0x56005810 ; +}#else -#define cpu_is_pj4 () 0 +#define cpu_is_pj4 () 0 +#define cpu_is_pj4_nomp () 0#endif static inline int __attribute_const__ cpuid_feature_extract_field(u32 features,arch/arm/include/asm/setup.h +3
-0@@ -25,4 +25,7 @@ extern int arm_add_memory(u64 start, u64 size);extern void early_print (const char *str, ...);extern void dump_machine_table (void ); +extern void patch_udiv (void *addr,size_t size); +extern void patch_sdiv (void *addr,size_t size); +#endif arch/arm/kernel/module.c +40
-0@@ -20,7 +20,9 @@#include <linux/string.h> #include <linux/gfp.h> +#include <asm/hwcap.h> #include <asm/pgtable.h> +#include <asm/setup.h> #include <asm/sections.h> #include <asm/smp_plat.h> #include <asm/unwind.h> @@ -51,6 +53,38 @@ void *module_alloc(unsigned long size) }#endif +#ifdef CONFIG_ARM_PATCH_UIDIV +static int module_patch_aeabi_uidiv (unsigned long loc,const Elf32_Sym *sym) +{ +extern char __aeabi_uidiv[], __aeabi_idiv[]; +unsigned long udiv_addr = (unsigned long )__aeabi_uidiv; +unsigned long sdiv_addr = (unsigned long )__aeabi_idiv; +unsigned int mask; + +if (IS_ENABLED (CONFIG_THUMB2_KERNEL )) + mask =HWCAP_IDIVT ; +else + mask =HWCAP_IDIVA ; + +if (elf_hwcap & mask) { +if (sym->st_value == udiv_addr) { +patch_udiv (&loc,sizeof (loc)); +return 1 ; + }else if (sym->st_value == sdiv_addr) { +patch_sdiv (&loc,sizeof (loc)); +return 1 ; + } + } + +return 0 ; +} +#else +static int module_patch_aeabi_uidiv (unsigned long loc,const Elf32_Sym *sym) +{ +return 0 ; +} +#endif +int apply_relocate (Elf32_Shdr *sechdrs,const char *strtab,unsigned int symindex,unsigned int relindex,struct module *module)@@ -109,6 +143,9 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,return -ENOEXEC ; } +if (module_patch_aeabi_uidiv (loc, sym)) +break ; + offset =__mem_to_opcode_arm (*(u32 *)loc); offset = (offset &0x00ffffff ) <<2 ;if (offset &0x02000000 )@@ -195,6 +232,9 @@ apply_relocate(Elf32_Shdr *sechdrs, const char *strtab, unsigned int symindex,return -ENOEXEC ; } +if (module_patch_aeabi_uidiv (loc, sym)) +break ; + upper =__mem_to_opcode_thumb16 (*(u16 *)loc); lower =__mem_to_opcode_thumb16 (*(u16 *)(loc +2 ));arch/arm/kernel/setup.c +68
-0@@ -32,6 +32,7 @@#include <linux/compiler.h> #include <linux/sort.h> #include <linux/psci.h> +#include <linux/module.h> #include <asm/unified.h> #include <asm/cp15.h> @@ -375,6 +376,72 @@ void __init early_print(const char *str, ...)printk ("%s" , buf); } +#ifdef CONFIG_ARM_PATCH_UIDIV +/* "sdiv r0, r0, r1" or "mrc p6, 1, r0, CR0, CR1, 4" if we're on pj4 w/o MP */ +static u32 __attribute_const__ sdiv_instruction(void) +{ +if (IS_ENABLED (CONFIG_THUMB2_KERNEL )) { +if (cpu_is_pj4_nomp ()) +return __opcode_to_mem_thumb32 (0xee300691 ); +return __opcode_to_mem_thumb32 (0xfb90f0f1 ); + } + +if (cpu_is_pj4_nomp ()) +return __opcode_to_mem_arm (0xee300691 ); +return __opcode_to_mem_arm (0xe710f110 ); +} + +/* "udiv r0, r0, r1" or "mrc p6, 1, r0, CR0, CR1, 0" if we're on pj4 w/o MP */ +static u32 __attribute_const__ udiv_instruction(void) +{ +if (IS_ENABLED (CONFIG_THUMB2_KERNEL )) { +if (cpu_is_pj4_nomp ()) +return __opcode_to_mem_thumb32 (0xee300611 ); +return __opcode_to_mem_thumb32 (0xfbb0f0f1 ); + } + +if (cpu_is_pj4_nomp ()) +return __opcode_to_mem_arm (0xee300611 ); +return __opcode_to_mem_arm (0xe730f110 ); +} + +static void __init_or_module patch(u32 **addr, size_t count, u32 insn) +{ +for (; count !=0 ; count -=4 ) + **addr++ = insn; +} + +void __init_or_module patch_udiv(void *addr, size_t size) +{ +patch (addr, size,udiv_instruction ()); +} + +void __init_or_module patch_sdiv(void *addr, size_t size) +{ +return patch (addr, size,sdiv_instruction ()); +} + +static void __init patch_aeabi_uidiv (void ) +{ +extern char __start_udiv_loc[], __stop_udiv_loc[]; +extern char __start_idiv_loc[], __stop_idiv_loc[]; +unsigned int mask; + +if (IS_ENABLED (CONFIG_THUMB2_KERNEL )) + mask =HWCAP_IDIVT ; +else + mask =HWCAP_IDIVA ; + +if (!(elf_hwcap & mask)) +return ; + +patch_udiv (__start_udiv_loc, __stop_udiv_loc - __start_udiv_loc); +patch_sdiv (__start_idiv_loc, __stop_idiv_loc - __start_idiv_loc); +} +#else +static void __init patch_aeabi_uidiv (void ) { } +#endif +static void __init cpuid_init_hwcaps (void ) {int block;@@ -642,6 +709,7 @@ static void __init setup_processor(void) elf_hwcap = list->elf_hwcap;cpuid_init_hwcaps (); +patch_aeabi_uidiv ();#ifndef CONFIG_ARM_THUMB elf_hwcap &= ~(HWCAP_THUMB |HWCAP_IDIVT );arch/arm/kernel/vmlinux.lds.S +13
-0@@ -28,6 +28,18 @@ *(.hyp.idmap.text) \ VMLINUX_SYMBOL(__hyp_idmap_text_end) = .; +#ifdef CONFIG_ARM_PATCH_UIDIV +#define UIDIV_REC . = ALIGN(8); \ + VMLINUX_SYMBOL(__start_udiv_loc) = .; \ + *(__udiv_loc) \ + VMLINUX_SYMBOL(__stop_udiv_loc) = .; \ + VMLINUX_SYMBOL(__start_idiv_loc) = .; \ + *(__idiv_loc) \ + VMLINUX_SYMBOL(__stop_idiv_loc) = .; +#else +#define UIDIV_REC +#endif + #ifdef CONFIG_HOTPLUG_CPU #define ARM_CPU_DISCARD(x) #define ARM_CPU_KEEP(x) x@@ -210,6 +222,7 @@ SECTIONS .init.data : { #ifndef CONFIG_XIP_KERNEL INIT_DATA + UIDIV_REC #endif INIT_SETUP(16) INIT_CALLSSignature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v2 2/2] ARM: Replace calls to __aeabi_{u}idiv with udiv/sdiv instructions
Quoted 5 lines · Nicolas Pitre
3) In fact I was wondering if the overhead of the branch and back is really significant compared to the non trivial cost of a idiv instruction and all the complex infrastructure required to patch those branches directly, and consequently if the performance difference is actually worth it versus simply doing (2) alone.
I definitely agree with you on this, given that modern CPUs which are going to be benefitting from idiv are modern CPUs with a branch predictor (and if it's not predicting such unconditional calls and returns it's not much use as a branch predictor!)
I think what we need to see is the performance of existing kernels, vs patching the idiv instructions at every callsite, vs patching the called function itself.
Quoted 20 lines
Quoted 20 lines
arch/arm/kernel/setup.c +20
-0+#ifdef CONFIG_ARM_PATCH_UIDIV +/* "sdiv r0, r0, r1" or "mrc p6, 1, r0, CR0, CR1, 4" if we're on pj4 w/o MP */ +static u32 __attribute_const__ sdiv_instruction(void) +{ +if (IS_ENABLED (CONFIG_THUMB2_KERNEL )) { +if (cpu_is_pj4_nomp ()) +return __opcode_to_mem_thumb32 (0xee300691 ); +return __opcode_to_mem_thumb32 (0xfb90f0f1 ); + } + +if (cpu_is_pj4_nomp ()) +return __opcode_to_mem_arm (0xee300691 ); +return __opcode_to_mem_arm (0xe710f110 ); +} + +/* "udiv r0, r0, r1" or "mrc p6, 1, r0, CR0, CR1, 0" if we're on pj4 w/o MP */ +static u32 __attribute_const__ udiv_instruction(void) +{ +if (IS_ENABLED (CONFIG_THUMB2_KERNEL )) { +if (cpu_is_pj4_nomp ())
Quoted 8 lines
arch/arm/kernel/setup.c void __init early_print(const char *str, ...) · line 406
Quoted 8 lines
arch/arm/kernel/setup.c +8
-0+return __opcode_to_mem_thumb32 (0xee300611 ); +return __opcode_to_mem_thumb32 (0xfbb0f0f1 ); + } + +if (cpu_is_pj4_nomp ()) +return __opcode_to_mem_arm (0xee300611 ); +return __opcode_to_mem_arm (0xe730f110 ); +}
Any reason the above aren't marked with __init_or_module as well, as the compiler can choose not to inline them?
Quoted 25 lines
Quoted 25 lines
arch/arm/kernel/setup.c +25
-0+ +static void __init_or_module patch(u32 **addr, size_t count, u32 insn) +{ +for (; count !=0 ; count -=4 ) + **addr++ = insn; +} + +void __init_or_module patch_udiv(void *addr, size_t size) +{ +patch (addr, size,udiv_instruction ()); +} + +void __init_or_module patch_sdiv(void *addr, size_t size) +{ +return patch (addr, size,sdiv_instruction ()); +} + +static void __init patch_aeabi_uidiv (void ) +{ +extern char __start_udiv_loc[], __stop_udiv_loc[]; +extern char __start_idiv_loc[], __stop_idiv_loc[]; +unsigned int mask; + +if (IS_ENABLED (CONFIG_THUMB2_KERNEL )) + mask =HWCAP_IDIVT ;
Quoted 8 lines
arch/arm/kernel/setup.c void __init early_print(const char *str, ...) · line 439
Quoted 8 lines
arch/arm/kernel/setup.c +8
-0+else + mask =HWCAP_IDIVA ; + +if (!(elf_hwcap & mask)) +return ; + +patch_udiv (__start_udiv_loc, __stop_udiv_loc - __start_udiv_loc); +patch_sdiv (__start_idiv_loc, __stop_idiv_loc - __start_idiv_loc);
I'm left really concerned about this. We're modifying code with all the caches on, and the above is not only missing any coherency of the I/D paths, it's also missing any branch predictor maintanence. So, if we've executed any divisions at this point, the predictor could already predicted one of these branches that's being modified.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 2/2] ARM: Replace calls to __aeabi_{u}idiv with udiv/sdiv instructions
In reply to Nicolas PitreQuoted 15 lines · Nicolas Pitre
Quoted 4 lines · Stephen Boyd
The ARM compiler inserts calls to
__aeabi_uidiv()and__aeabi_idiv()when it needs to perform division on signed and unsigned integers. If a processor has support for the udiv and sdiv division instructions the calls to these support routinesQuoted 8 lines
can be replaced with those instructions. Now that recordmcount records the locations of calls to these library functions in two sections (one for udiv and one for sdiv), iterate over these sections early at boot and patch the call sites with the appropriate division instruction when we determine that the processor supports the division instructions. Using the division instructions should be faster and less power intensive than running the support code.
Quoted 7 lines
A few remarks:
1) The code assumes unconditional branches to
__aeabi_idivand__aeabi_uidiv. What if there are conditional branches? Also, tail call optimizations will generate a straight b opcode rather than a bl and patching those will obviously have catastrophic results. I think you should validate the presence of a bl before patching over it.
I did a quick check on a compiled kernel I had nearby, and there are no conditional or tail calls to those functions, so although they should obviously be checked for correctness, performance is unlikely to matter for those.
However, there are almost half as many calls to __aeabi_{u}idivmod as to the plain div functions, 129 vs 228 for signed and unsigned combined. For best results, these functions should also be patched with the hardware instructions. Obviously the call sites for these can't be patched.
Quoted 9 lines
2) For those cases where a call to
__aeabi_uidivand__aeabi_idivis not patched due to (1), you could patch a uidiv/idiv plus "bx lr" at those function entry points too.3) In fact I was wondering if the overhead of the branch and back is really significant compared to the non trivial cost of a idiv instruction and all the complex infrastructure required to patch those branches directly, and consequently if the performance difference is actually worth it versus simply doing (2) alone.
Depending on the operands, the div instruction can take as few as 3 cycles on a Cortex-A7.
Signature · 2 lines
M?ns Rullg?rd mans at mansr.com
Re: [PATCH v2 2/2] ARM: Replace calls to __aeabi_{u}idiv with udiv/sdiv instructions
Quoted 25 lines · M?ns Rullg?rd
Quoted 15 lines · Nicolas Pitre
Quoted 4 lines · Stephen Boyd
The ARM compiler inserts calls to
__aeabi_uidiv()and__aeabi_idiv()when it needs to perform division on signed and unsigned integers. If a processor has support for the udiv and sdiv division instructions the calls to these support routinesQuoted 8 lines
can be replaced with those instructions. Now that recordmcount records the locations of calls to these library functions in two sections (one for udiv and one for sdiv), iterate over these sections early at boot and patch the call sites with the appropriate division instruction when we determine that the processor supports the division instructions. Using the division instructions should be faster and less power intensive than running the support code.
Quoted 7 lines
A few remarks:
1) The code assumes unconditional branches to
__aeabi_idivand__aeabi_uidiv. What if there are conditional branches? Also, tail call optimizations will generate a straight b opcode rather than a bl and patching those will obviously have catastrophic results. I think you should validate the presence of a bl before patching over it.
Quoted 4 lines
I did a quick check on a compiled kernel I had nearby, and there are no conditional or tail calls to those functions, so although they should obviously be checked for correctness, performance is unlikely to matter for those.
I'm more worried about correctness than performance. This kind of patching should be done defensively.
Quoted 5 lines
However, there are almost half as many calls to __aeabi_{u}idivmod as to the plain div functions, 129 vs 228 for signed and unsigned combined. For best results, these functions should also be patched with the hardware instructions. Obviously the call sites for these can't be patched.
Current __aeabi_{u}idivmod implementations are simple wrappers around a call to __aeabi_{u}idiv so they'd get the benefit automatically regardless of the chosen approach.
Quoted 4 lines
Quoted 4 lines
2) For those cases where a call to
__aeabi_uidivand__aeabi_idivis not patched due to (1), you could patch a uidiv/idiv plus "bx lr" at those function entry points too.
Quoted 8 lines
Quoted 5 lines
3) In fact I was wondering if the overhead of the branch and back is really significant compared to the non trivial cost of a idiv instruction and all the complex infrastructure required to patch those branches directly, and consequently if the performance difference is actually worth it versus simply doing (2) alone.
Depending on the operands, the div instruction can take as few as 3 cycles on a Cortex-A7.
Even the current software based implementation can produce a result with about 5 simple ALU instructions depending on the operands.
The average cycle count is more important than the easy-way-out case. And then how significant the two branches around it are compared to idiv alone from direct patching of every call to it.
Nicolas
Re: [PATCH v2 2/2] ARM: Replace calls to __aeabi_{u}idiv with udiv/sdiv instructions
Quoted 13 lines · Nicolas Pitre
Quoted 10 lines · M?ns Rullg?rd
Quoted 5 lines · Nicolas Pitre
3) In fact I was wondering if the overhead of the branch and back is really significant compared to the non trivial cost of a idiv instruction and all the complex infrastructure required to patch those branches directly, and consequently if the performance difference is actually worth it versus simply doing (2) alone.
Depending on the operands, the div instruction can take as few as 3 cycles on a Cortex-A7.
Quoted 6 lines
Even the current software based implementation can produce a result with about 5 simple ALU instructions depending on the operands.
The average cycle count is more important than the easy-way-out case. And then how significant the two branches around it are compared to idiv alone from direct patching of every call to it.
If not calling the function saves an I-cache miss, the benefit can be substantial. No, I have no proof of this being a problem, but it's something that could happen.
Of course, none of this is going to be as good as letting the compiler generate div instructions directly.
Signature · 2 lines
M?ns Rullg?rd mans at mansr.com
Re: [PATCH v2 2/2] ARM: Replace calls to __aeabi_{u}idiv with udiv/sdiv instructions
Quoted 3 lines · M?ns Rullg?rd
If not calling the function saves an I-cache miss, the benefit can be substantial. No, I have no proof of this being a problem, but it's something that could happen.
That's a simplistic view of modern CPUs.
As I've already said, modern CPUs which have branch prediction, but they also have speculative instruction fetching and speculative data prefetching - which the CPUs which have idiv support will have.
With such features, the branch predictor is able to learn that the branch will be taken, and because of the speculative instruction fetching, it can bring the cache line in so that it has the instructions it needs with minimal or, if working correctly, without stalling the CPU pipeline.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Re: [PATCH v2 2/2] ARM: Replace calls to __aeabi_{u}idiv with udiv/sdiv instructions
Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · M?ns Rullg?rd
If not calling the function saves an I-cache miss, the benefit can be substantial. No, I have no proof of this being a problem, but it's something that could happen.
That's a simplistic view of modern CPUs.
As I've already said, modern CPUs which have branch prediction, but they also have speculative instruction fetching and speculative data prefetching - which the CPUs which have idiv support will have.
Quoted 5 lines
With such features, the branch predictor is able to learn that the branch will be taken, and because of the speculative instruction fetching, it can bring the cache line in so that it has the instructions it needs with minimal or, if working correctly, without stalling the CPU pipeline.
It doesn't matter how many fancy features the CPU has. Executing more branches and using more cache lines puts additional pressure on those resources, reducing overall performance. Besides, the performance counters readily show that the prediction is nothing near as perfect as you seem to believe.
Signature · 2 lines
M?ns Rullg?rd mans at mansr.com
Re: [PATCH v2 2/2] ARM: Replace calls to __aeabi_{u}idiv with udiv/sdiv instructions
Quoted 19 lines · M?ns Rullg?rd
Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · M?ns Rullg?rd
If not calling the function saves an I-cache miss, the benefit can be substantial. No, I have no proof of this being a problem, but it's something that could happen.
That's a simplistic view of modern CPUs.
As I've already said, modern CPUs which have branch prediction, but they also have speculative instruction fetching and speculative data prefetching - which the CPUs which have idiv support will have.
Quoted 5 lines
With such features, the branch predictor is able to learn that the branch will be taken, and because of the speculative instruction fetching, it can bring the cache line in so that it has the instructions it needs with minimal or, if working correctly, without stalling the CPU pipeline.
Quoted 5 lines
It doesn't matter how many fancy features the CPU has. Executing more branches and using more cache lines puts additional pressure on those resources, reducing overall performance. Besides, the performance counters readily show that the prediction is nothing near as perfect as you seem to believe.
OK... Let's try to come up with actual numbers.
We know that letting gcc emit idiv by itself is the ultimate solution. And it is free of maintenance on our side besides passing the appropriate argument to gcc of course. So this is worth doing.
For the case where you have a set of target machines in your kernel that may or may not have idiv, then the first step should be to patch __aeabi_uidiv and __aeabi_idiv. This is a pretty small and simple change that might turn out to be more than good enough. It is necessary anyway as the full patching solution does not cover all cases.
Then, IMHO, it would be a good idea to get performance numbers to compare that first step and the full patching solution. Of course the full patching will yield better performance. It has to. But if the difference is not significant enough, then it might not be worth introducing the implied complexity into mainline. And it is not because the approach is bad. In fact I think this is a very cool hack. But it comes with a cost in maintenance and that cost has to be justified.
Just to have an idea, I produced the attached micro benchmark. I tested on a TC2 forced to a single Cortex-A15 core and I got those results:
Testing INLINE_DIV ...
real 0m7.182s user 0m7.170s sys 0m0.000s
Testing PATCHED_DIV ...
real 0m7.181s user 0m7.170s sys 0m0.000s
Testing OUTOFLINE_DIV ...
real 0m7.181s user 0m7.170s sys 0m0.005s
Testing LIBGCC_DIV ...
real 0m18.659s user 0m18.635s sys 0m0.000s
As you can see, whether the div is inline or out-of-line, whether arguments are moved into r0-r1 or not, makes no difference at all on a Cortex-A15.
Now forcing it onto a Cortex-A7 core:
Testing INLINE_DIV ...
real 0m8.917s user 0m8.895s sys 0m0.005s
Testing PATCHED_DIV ...
real 0m11.666s user 0m11.645s sys 0m0.000s
Testing OUTOFLINE_DIV ...
real 0m13.065s user 0m13.025s sys 0m0.000s
Testing LIBGCC_DIV ...
real 0m51.815s user 0m51.750s sys 0m0.005s
So on A cortex-A7 the various overheads become visible. How significant is it in practice with normal kernel usage? I don't know.
Nicolas
-------------- next part --------------
#!/bin/sh
set -e
for test in INLINE_DIV PATCHED_DIV OUTOFLINE_DIV LIBGCC_DIV; do
gcc -o divtest_$test divtest.S -D$test
echo "Testing $test ..."
time ./divtest_$test
echo
rm -f divtest_$test
done
-------------- next part --------------
.arm
.arch_extension idiv
.globl main
main:
stmfd sp!, {r4, r5, lr}
mov r4, #171: mov r5, #1
2:
#if defined(INLINE_DIV)
udiv r0, r4, r5
#elif defined(OUTOFLINE_DIV)
mov r0, r4 mov r1, r5 bl my_div
#elif defined(PATCHED_DIV)
mov r0, r4 mov r1, r5 udiv r0, r0, r1
#elif defined(LIBGCC_DIV)
mov r0, r4 mov r1, r5 bl __aeabi_uidiv
#else
#error "define INLINE_DIV, OUTOFLINE_DIV or LIBGCC_DIV" #endif
add r5, r5, #1
cmp r4, r5
bhs 2b
adds r4, r4, r4, lsl #1
bpl 1b
mov r0, #0
ldmfd sp!, {r4, r5, pc}
.space 1024my_div:
udiv r0, r0, r1 bx lr
Re: [PATCH v2 2/2] ARM: Replace calls to __aeabi_{u}idiv with udiv/sdiv instructions
Quoted 98 lines · Nicolas Pitre
Quoted 19 lines · M?ns Rullg?rd
Quoted 11 lines · Russell King - ARM Linux
Quoted 3 lines · M?ns Rullg?rd
If not calling the function saves an I-cache miss, the benefit can be substantial. No, I have no proof of this being a problem, but it's something that could happen.
That's a simplistic view of modern CPUs.
As I've already said, modern CPUs which have branch prediction, but they also have speculative instruction fetching and speculative data prefetching - which the CPUs which have idiv support will have.
Quoted 5 lines
With such features, the branch predictor is able to learn that the branch will be taken, and because of the speculative instruction fetching, it can bring the cache line in so that it has the instructions it needs with minimal or, if working correctly, without stalling the CPU pipeline.
Quoted 5 lines
It doesn't matter how many fancy features the CPU has. Executing more branches and using more cache lines puts additional pressure on those resources, reducing overall performance. Besides, the performance counters readily show that the prediction is nothing near as perfect as you seem to believe.
OK... Let's try to come up with actual numbers.
We know that letting gcc emit idiv by itself is the ultimate solution. And it is free of maintenance on our side besides passing the appropriate argument to gcc of course. So this is worth doing.
For the case where you have a set of target machines in your kernel that may or may not have idiv, then the first step should be to patch
__aeabi_uidivand__aeabi_idiv. This is a pretty small and simple change that might turn out to be more than good enough. It is necessary anyway as the full patching solution does not cover all cases.Then, IMHO, it would be a good idea to get performance numbers to compare that first step and the full patching solution. Of course the full patching will yield better performance. It has to. But if the difference is not significant enough, then it might not be worth introducing the implied complexity into mainline. And it is not because the approach is bad. In fact I think this is a very cool hack. But it comes with a cost in maintenance and that cost has to be justified.
Just to have an idea, I produced the attached micro benchmark. I tested on a TC2 forced to a single Cortex-A15 core and I got those results:
Testing
INLINE_DIV...real 0m7.182s user 0m7.170s sys 0m0.000sTesting
PATCHED_DIV...real 0m7.181s user 0m7.170s sys 0m0.000sTesting
OUTOFLINE_DIV...real 0m7.181s user 0m7.170s sys 0m0.005sTesting
LIBGCC_DIV...real 0m18.659s user 0m18.635s sys 0m0.000sAs you can see, whether the div is inline or out-of-line, whether arguments are moved into r0-r1 or not, makes no difference at all on a Cortex-A15.
Now forcing it onto a Cortex-A7 core:
Testing
INLINE_DIV...real 0m8.917s user 0m8.895s sys 0m0.005sTesting
PATCHED_DIV...real 0m11.666s user 0m11.645s sys 0m0.000sTesting
OUTOFLINE_DIV...real 0m13.065s user 0m13.025s sys 0m0.000s
Quoted 8 lines
Testing
LIBGCC_DIV...real 0m51.815s user 0m51.750s sys 0m0.005sSo on A cortex-A7 the various overheads become visible. How significant is it in practice with normal kernel usage? I don't know.
Bear in mind that in a trivial test like this, everything fits in L1 caches and branch prediction works perfectly. It would be more informative to measure the effect on a load that already has some cache and branch prediction misses.
Signature · 2 lines
M?ns Rullg?rd mans at mansr.com
Re: [PATCH v2 2/2] ARM: Replace calls to __aeabi_{u}idiv with udiv/sdiv instructions
In reply to Stephen Boydarch/arm/include/asm/cputype.h static inline int cpu_is_pj4(void) · line 256
Quoted 12 lines · Stephen Boyd
arch/arm/include/asm/cputype.h +4
-0@@ -250,8 +250,14 @@ static inline int cpu_is_pj4(void)return 0 ; } + +static inline bool __attribute_const__ cpu_is_pj4_nomp(void) +{ +return read_cpuid_part () ==0x56005810 ;
One other thing here, we really ought to avoid adding more magic numbers to this file. We have the ARM processors nicely #defined, but not a lot else. Maybe its time that we continued with the nice definitions for these part numbers?
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.