Re: Gold linker patch to split unlikely text into a separate segment

["Sriraman Tallam via binutils" <binutils at sourceware dot org>]

On Thu, Oct 19, 2017 at 2:45 PM, Cary Coutant <ccoutant@gmail.com> wrote:
> +      bool text_unlikely_segment
> +          = (parameters->options().text_unlikely_segment()
> +             && is_prefix_of(".text.unlikely",
> +                             object->section_name(shndx).c_str()));
> +      if (it == this->section_segment_map_.end()
> +         && !text_unlikely_segment)
>
> To make the code easier to follow, I'd prefer to see this section of
> code rewritten into three forks:
>
>       if (text_unlikely_segment)
>         {
>           // new code for .text.unlikely ...
>         }
>       else
>         {
>           Section_segment_map::iterator it = ...
>           if (it == this->section_segment_map_.end())
>             {
>               os = choose_output_section(...);
>             }
>           else
>             {
>               // We know the name of the output section...
>               ...
>               os = get_output_section(...);
>               ...
>           }
>        }
>
> -         const char* os_name = it->second->name;
> +         const char* os_name = text_unlikely_segment ?
> +                               ".text.unlikely" : it->second->name;
>

Done.

> The ?: expression needs parens, and the ? should be at the beginning
> of the next line, like so:
>
>          const char* os_name = (text_unlikely_segment
>                                 ? ".text.unlikely" : it->second->name);
>
> (This is moot if you follow my suggestion above.)
>
>           if (!os->is_unique_segment())
>             {
>               os->set_is_unique_segment();
> -             os->set_extra_segment_flags(it->second->flags);
> -             os->set_segment_alignment(it->second->align);
> +              if (!text_unlikely_segment)
> +               {
> +                 os->set_extra_segment_flags(it->second->flags);
> +                 os->set_segment_alignment(it->second->align);
> +               }
>
> Do you want to call set_is_unique_segment() for .text.unlikely
> sections? That sounds wrong to me. If that's the right thing to do,
> please explain in a comment.

Yes, I want the final binary to be like this:

Elf file type is EXEC (Executable file)
Entry point 0x400560
There are 10 program headers, starting at offset 64

Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align

....

  LOAD           0x0000000000000000 0x0000000000400000 0x0000000000400000
                 0x0000000000000970 0x0000000000000970  R E    1000
  LOAD           0x0000000000000970 0x0000000000401970 0x0000000000401970
                 0x000000000000000b 0x000000000000000b  R E    1000
  LOAD           0x0000000000000de8 0x0000000000402de8 0x0000000000402de8
...


 Section to Segment mapping:
  Segment Sections...
...
   02     .interp .note.ABI-tag .note.gnu.build-id .dynsym .dynstr
.gnu.hash .hash .gnu.version .gnu.version_r .rela.dyn .rela.plt .init
.plt .text .fini .rodata .eh_frame .eh_frame_hdr
   03     .text.unlikely
   04     .ctors .dtors .jcr .dynamic .got .got.plt .data .bss


One more PT_LOAD segment for .text.unlikely which can only be done
with set_is_unique_segment.



>
> +  DEFINE_bool(text_unlikely_segment, options::DASH_Z, '\0', false,
> +             N_("Move .text.unlikely sections to a separate ELF segment."),
> +             N_("Do not move .text.unlikely sections to a separate "
> +                "ELF segment."));
>
> Just call it a "segment" (not "ELF segment").

Done.

PTAL.

Thanks
Sri


>
> -cary
>
>
> On Thu, Oct 19, 2017 at 11:30 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>> Ping. Cary, is this alright to commit?
>>
>> * options.h (-z,text_unlikely_segment): New option.
>> * layout.cc (Layout::layout): Create new output section
>> for .text.unlikely sections with the new option.
>>  (Layout::segment_precedes): Check for the new option
>>  when segment flags match.
>> * testsuite/text_unlikely_segment.cc: New test source.
>> * testsuite/text_unlikely_segment.sh: New test script.
>> * testsuite/Makefile.am (text_unlikely_segment): New test.
>>
>> On Tue, Oct 10, 2017 at 2:52 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>> Ping. Cary, is this alright to commit?
>>>
>>> * options.h (-z,text_unlikely_segment): New option.
>>> * layout.cc (Layout::layout): Create new output section
>>> for .text.unlikely sections with the new option.
>>>  (Layout::segment_precedes): Check for the new option
>>>  when segment flags match.
>>> * testsuite/text_unlikely_segment.cc: New test source.
>>> * testsuite/text_unlikely_segment.sh: New test script.
>>> * testsuite/Makefile.am (text_unlikely_segment): New test.
>>>
>>>
>>>
>>>
>>> On Thu, Oct 5, 2017 at 4:41 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>> On Thu, Oct 5, 2017 at 4:41 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On 10/5/17, Sriraman Tallam <tmsriram@google.com> wrote:
>>>>>> On Wed, Oct 4, 2017 at 5:12 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>> On 10/4/17, Sriraman Tallam <tmsriram@google.com> wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>>      This patch adds an option to gold to create a new ELF segment for
>>>>>>>> code determined unlikely by the compiler.  Currently, this can be done
>>>>>>>> with a linker plugin but I was wondering if it is fine to have this
>>>>>>>> support in gold with an option for ease of use.
>>>>>>>>
>>>>>>>>      The advantages of doing this are:
>>>>>>>>
>>>>>>>> * We could only map the hot segment to huge pages to keep iTLB misses
>>>>>>>> low
>>>>>>>> * We could munlock the unlikely segment
>>>>>>>>
>>>>>>>> Is this alright?
>>>>>>>>
>>>>>>>> ChangeLog entry:
>>>>>>>>
>>>>>>>> * options.h (text_unlikely_segment): New option.
>>>>>>>> * layout.cc (Layout::layout): Create new output section
>>>>>>>> for .text.unlikely sections with the new option.
>>>>>>>> (Layout::segment_precedes): Check for the new option
>>>>>>>> when segment flags match.
>>>>>>>>
>>>>>>>> Patch attached.
>>>>>>>>
>>>>>>>
>>>>>>> This is an interesting approach.  Do you have some performace
>>>>>>> numbers?
>>>>>>
>>>>>> With function re-ordering of hot code, segment splitting and mapping
>>>>>> only hot code to huge pages, we see a reduction in iTLB misses
>>>>>> translating to performance improvements of 0.5 to 1% on some of our
>>>>>> benchmarks.
>>>>>
>>>>> Please include this info in your commit log.
>>>>>
>>>>>>>
>>>>>>> 2 Comments:
>>>>>>>
>>>>>>> 1.  I'd prefer "-z text-unlikely-segment" with '-', instead of '_'.
>>>>>>> 2.  Need a testcase.
>>>>>>
>>>>>> Done and patch attached.
>>>>>>
>>>>>
>>>>> LGTM.  But I can't approve it.
>>>>
>>>> Np, thanks!
>>>>
>>>> Sri
>>>>
>>>>>
>>>>> Thanks.
>>>>>
>>>>> --
>>>>> H.J.

Attachment: segment_split.txt
Description: Text document