Page MenuHomePhabricator

Keep the output text sections with prefixes ".text.hot" , ".text.unlikely", ".text.startup", ".text.exit" separate
ClosedPublic

Authored by tmsriram on Apr 19 2018, 3:15 PM.

Details

Summary

Separate output sections for selected text section prefixes to enable TLB optimizations and for readablilty.

I committed a patch to gold linker recently to do this although that was via a flag. Talking to Rui offline, he suggested I make this the default. This is useful for the following reasons:

  • We have tools to map specific code to huge pages and we do not want to do this for unlikely executed code, improves huge page TLB.
  • We also munlock code that is unlikely executed and it is easier to find it with this.
  • This is also useful or code layout verification.

Is this alright?

Diff Detail

Repository
rL LLVM

Event Timeline

tmsriram created this revision.Apr 19 2018, 3:15 PM
ruiu added a comment.Apr 19 2018, 3:40 PM

And this should also slightly improve performance because of less number of page faults and improved locality for hot paths, I guess?

pcc added a subscriber: pcc.Apr 19 2018, 3:46 PM

Note that this ordering isn't necessarily best for every architecture. On ARM for example it is best to put the hot sections in the middle (see also D44969).

I think that for hot sections my preference would be to treat them as if they were listed in --symbol-ordering-file. The behaviour for the other sections seems fine to me.

ruiu added a comment.Apr 19 2018, 3:49 PM

What do you mean by "as if they were listed in --symbol-ordering-file"?

pcc added a comment.Apr 19 2018, 3:53 PM

I mean that on ARM any sections named .text.hot.* would be placed in the middle of the .text section right after the sections named in the --symbol-ordering-file (if present).

In D45841#1072712, @pcc wrote:

Note that this ordering isn't necessarily best for every architecture. On ARM for example it is best to put the hot sections in the middle (see also D44969).

When you say "this ordering" I am confused because currently there is no ordering of the output sections with prefix ".text". Looking at it, seems like the order in which they were seen is the order in the final binary.

I think that for hot sections my preference would be to treat them as if they were listed in --symbol-ordering-file. The behaviour for the other sections seems fine to me.

So, this patch would break any symbol ordering across prefixes. Maybe it is better to disable this feature if symbol-ordering-file is used, what do you think?

pcc added a comment.Apr 19 2018, 3:56 PM
In D45841#1072712, @pcc wrote:

Note that this ordering isn't necessarily best for every architecture. On ARM for example it is best to put the hot sections in the middle (see also D44969).

When you say "this ordering" I am confused because currently there is no ordering of the output sections with prefix ".text". Looking at it, seems like the order in which they were seen is the order in the final binary.

Yes, I meant the ordering imposed by this patch which would be hot followed by unlikely, startup, etc.

I think that for hot sections my preference would be to treat them as if they were listed in --symbol-ordering-file. The behaviour for the other sections seems fine to me.

So, this patch would break any symbol ordering across prefixes. Maybe it is better to disable this feature if symbol-ordering-file is used, what do you think?

That would also work for now.

tmsriram added a comment.EditedApr 20 2018, 10:35 AM
In D45841#1072734, @pcc wrote:
In D45841#1072712, @pcc wrote:

Note that this ordering isn't necessarily best for every architecture. On ARM for example it is best to put the hot sections in the middle (see also D44969).

When you say "this ordering" I am confused because currently there is no ordering of the output sections with prefix ".text". Looking at it, seems like the order in which they were seen is the order in the final binary.

Yes, I meant the ordering imposed by this patch which would be hot followed by unlikely, startup, etc.

Ok, this patch itself does not impose an ordering among output sections. The ordering is arbitrary and follows what is seen first by the linker. Look at the test case shown. If I swap .text.exit and .text.startup for instance, the order would be different.

I think that for hot sections my preference would be to treat them as if they were listed in --symbol-ordering-file. The behaviour for the other sections seems fine to me.

So, this patch would break any symbol ordering across prefixes. Maybe it is better to disable this feature if symbol-ordering-file is used, what do you think?

That would also work for now.

I was thinking a lot more about this and playing with it. Two things:

  • This patch will break symbol-ordering-file for symbols that are in two different output sections.
  • This patch could also affect linker scripts which assume all .text is in the same output section.

Given this, I believe it might be better to not change the default behavior and have this feature under a flag, just like what gold does. What do you think?

pcc added a comment.Apr 20 2018, 11:11 AM
In D45841#1072734, @pcc wrote:
In D45841#1072712, @pcc wrote:

Note that this ordering isn't necessarily best for every architecture. On ARM for example it is best to put the hot sections in the middle (see also D44969).

When you say "this ordering" I am confused because currently there is no ordering of the output sections with prefix ".text". Looking at it, seems like the order in which they were seen is the order in the final binary.

Yes, I meant the ordering imposed by this patch which would be hot followed by unlikely, startup, etc.

Ok, this patch itself does not impose an ordering among output sections. The ordering is arbitrary and follows what is seen first by the linker. Look at the test case shown. If I swap .text.exit and .text.startup for instance, the order would be different.

Got it. Sorry I missed that part.

I think that for hot sections my preference would be to treat them as if they were listed in --symbol-ordering-file. The behaviour for the other sections seems fine to me.

So, this patch would break any symbol ordering across prefixes. Maybe it is better to disable this feature if symbol-ordering-file is used, what do you think?

That would also work for now.

I was thinking a lot more about this and playing with it. Two things:

  • This patch will break symbol-ordering-file for symbols that are in two different output sections.
  • This patch could also affect linker scripts which assume all .text is in the same output section.

Given this, I believe it might be better to not change the default behavior and have this feature under a flag, just like what gold does. What do you think?

Seems reasonable. Even if we made it conditional on presence of --symbol-ordering-file or a linker script I think it would be a little confusing for either of those things to have that sort of effect on prefixed output sections.

grimar added a subscriber: grimar.Apr 23 2018, 3:06 AM

I think this is OK, but since it changes the traditional legacy "GNU" linkers behavior, don't we want a flag to disable that?
Since gold has a flag, we will need to support it anyways I think, so we probably should have -no-<something>/-<something> flags,
matching the gold ones and just a different default value.

ELF/Writer.cpp
107 ↗(On Diff #143170)

This needs a comment I think.

test/ELF/text-section-prefix.s
12 ↗(On Diff #143170)

You do not need symbols, no? Just having different .text.* sections should be enough for this test case.

I was thinking a lot more about this and playing with it. Two things:

  • This patch will break symbol-ordering-file for symbols that are in two different output sections.
  • This patch could also affect linker scripts which assume all .text is in the same output section.

Given this, I believe it might be better to not change the default behavior and have this feature under a flag, just like what gold does. What do you think?

I do not think linker scripts is a problem probably. When you write a script, you usually explicitly say where to put the sections,
so if script wants all .text* to be in a single section it just do .text : { *(.text) } and I think this is what scripts usually do.
If they don't, then there will be multiple .text sections, but I am not sure it is correct for the script to assume that case without listing the output section .text.

For symbol-ordering-file, disabling the feature looks like a solution.

But actually following the gold behaviour (so to have the feature under the flag and do not change the default) looks good to me for start, as it is safe and can
easily be switched.

tmsriram updated this revision to Diff 145097.May 3 2018, 2:53 PM

New option -z keep-text-section-prefix.

  • Changed to have this feature enabled by an option
  • The default is not changed.
tmsriram marked an inline comment as done.May 3 2018, 2:53 PM

Can you add a test case, please?

ELF/Driver.cpp
765 ↗(On Diff #145097)

Since each word is separated in a command line option name, I believe naming
should use uppercases:

ZKeepTextSectionPrefix

ELF/Writer.cpp
94 ↗(On Diff #145097)

This can be just:

return SectionName.startswith(PrefixName) || SectionName == PrefixName.drop_back();

I would probably also use shorter namings for arguments:
PrefixName -> Prefix
SectionName -> Name

114 ↗(On Diff #145097)

I think using commas would be more correct?

".text.hot", ".text.unlikely", ".text.startup" or ".text.exit"

tmsriram updated this revision to Diff 145216.May 4 2018, 9:59 AM

Add back the test that was omitted in the last update.

Make all the other suggested changes.

tmsriram marked 3 inline comments as done.May 4 2018, 10:00 AM
grimar added inline comments.May 4 2018, 10:11 AM
test/ELF/text-section-prefix.s
2 ↗(On Diff #145216)

Please add a check for nokeep-text-section-prefix too.

ruiu added inline comments.May 4 2018, 11:20 AM
ELF/Driver.cpp
762–766 ↗(On Diff #145216)

Sort alphabetically.

ELF/Writer.cpp
113 ↗(On Diff #145216)

Could you also mention why you want to do that in the first place? In this case the intention of doing this is more important than what we are doing for future readers.

119–120 ↗(On Diff #145216)

nit: remove {}

tmsriram updated this revision to Diff 145235.May 4 2018, 11:23 AM

Add check for nokeep-text-section-prefix

tmsriram marked 2 inline comments as done.May 4 2018, 11:24 AM
grimar added inline comments.May 4 2018, 11:30 AM
test/ELF/text-section-prefix.s
5 ↗(On Diff #145235)

I think it is a bit overcomplicated.

Can you just add 2 cases here please:

  1. No any flags.
  2. Case with -z nokeep-text-section-prefix.

Them should be the same. And I think the following should work:

  1. CHECKNO: .text
  2. CHECKNO-NOT: .text
tmsriram updated this revision to Diff 145238.May 4 2018, 11:31 AM

Updated with more comments and nits fixed.

tmsriram updated this revision to Diff 145275.May 4 2018, 1:48 PM

Updated test.

ruiu accepted this revision.May 4 2018, 3:07 PM

LGTM

ELF/Writer.cpp
117 ↗(On Diff #145275)

nit: we remove {} in a case like this.

This revision is now accepted and ready to land.May 4 2018, 3:07 PM
This revision was automatically updated to reflect the committed changes.