Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1619713007:
Make text section 2MB aligned for hugepages. (Closed)
DescriptionMake text section 2MB aligned for hugepages.
If we do not use the option '-Wl,--section-start=.text=2001000' at
link time, the performance gain on hugepages is gone. This is probably
because the original text section is not 2MB aligned and the after
mremap, the hugepage is not preserved.
BUG=chromium:569963
TEST=The performance gain is back on micro benchmarks.
Committed: https://crrev.com/1384534a94e6324d74906f0cbc1e707606c063f2
Cr-Commit-Position: refs/heads/master@{#373673}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Messages
Total messages: 53 (21 generated)
Description was changed from ========== Make text section 2MB aligned for hugepages. If we do not use the option '-Wl,--section-start=.text=2001000' at link time, the performance gain on hugepages is gone. This is probably because the original text section is not 2MB aligned and the after mremap, the hugepage is not preserved. BUG=chromium:569963 TEST=The performance gain is back on micro benchmarks. ========== to ========== Make text section 2MB aligned for hugepages. If we do not use the option '-Wl,--section-start=.text=2001000' at link time, the performance gain on hugepages is gone. This is probably because the original text section is not 2MB aligned and the after mremap, the hugepage is not preserved. BUG=chromium:569963 TEST=The performance gain is back on micro benchmarks. ==========
yunlian@chromium.org changed reviewers: + llozano@chromium.org, shenhan@chromium.org
kenchen@google.com changed reviewers: + kenchen@google.com
https://codereview.chromium.org/1619713007/diff/1/chromeos/hugepage_text/huge... File chromeos/hugepage_text/hugepage_text.cc (right): https://codereview.chromium.org/1619713007/diff/1/chromeos/hugepage_text/huge... chromeos/hugepage_text/hugepage_text.cc:176: size_t head_gap = (kHpageSize - (size_t)addr % kHpageSize) % kHpageSize; This should vaddr and the last mod operation is not necessary.
https://codereview.chromium.org/1619713007/diff/1/chromeos/hugepage_text/huge... File chromeos/hugepage_text/hugepage_text.cc (right): https://codereview.chromium.org/1619713007/diff/1/chromeos/hugepage_text/huge... chromeos/hugepage_text/hugepage_text.cc:176: size_t head_gap = (kHpageSize - (size_t)addr % kHpageSize) % kHpageSize; On 2016/01/22 22:19:42, kenchen wrote: > This should vaddr and the last mod operation is not necessary. Yes, the vaddr is part is done. The last mod is useful when the vaddr is 2MB aligned. In this case, the head_gap is 0 instead of kHpageSize
lgtm
lgtm
The CQ bit was checked by yunlian@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1619713007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1619713007/20001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm
The CQ bit was checked by yunlian@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1619713007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1619713007/20001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
yunlian@chromium.org changed reviewers: + rickyz@chromium.org
yunlian@chromium.org changed reviewers: + derat@chromium.org, satorux@google.com
Can some one approve this for me? I got lgtm from the owner of the library already. Thanks
derat@chromium.org changed reviewers: + cmtice@chromium.org, rickyz@google.com, yunlian@chromium.org - derat@chromium.org, rickyz@chromium.org, satorux@google.com
please get approval from someone in the chromeos/hugepage_text/OWNERS file rather than chromeos/OWNERS: yunlian@chromium.org llozano@chromium.org shenhan@chromium.org cmtice@chromium.org https://codereview.chromium.org/1619713007/diff/20001/chromeos/hugepage_text/... File chromeos/hugepage_text/hugepage_text.cc (right): https://codereview.chromium.org/1619713007/diff/20001/chromeos/hugepage_text/... chromeos/hugepage_text/hugepage_text.cc:176: size_t head_gap = (kHpageSize - (size_t)vaddr % kHpageSize) % kHpageSize; use c++-style casts (e.g. static_cast<size_t>()) here and elsewhere instead of c-style casts
On 2016/02/01 16:46:16, Daniel Erat wrote: > please get approval from someone in the chromeos/hugepage_text/OWNERS file > rather than chromeos/OWNERS: > > mailto:yunlian@chromium.org > mailto:llozano@chromium.org > mailto:shenhan@chromium.org > mailto:cmtice@chromium.org > > https://codereview.chromium.org/1619713007/diff/20001/chromeos/hugepage_text/... > File chromeos/hugepage_text/hugepage_text.cc (right): > > https://codereview.chromium.org/1619713007/diff/20001/chromeos/hugepage_text/... > chromeos/hugepage_text/hugepage_text.cc:176: size_t head_gap = (kHpageSize - > (size_t)vaddr % kHpageSize) % kHpageSize; > use c++-style casts (e.g. static_cast<size_t>()) here and elsewhere instead of > c-style casts (ah, i see you already have that, but maybe they aren't a committer)
https://codereview.chromium.org/1619713007/diff/20001/chromeos/hugepage_text/... File chromeos/hugepage_text/hugepage_text.cc (right): https://codereview.chromium.org/1619713007/diff/20001/chromeos/hugepage_text/... chromeos/hugepage_text/hugepage_text.cc:176: size_t head_gap = (kHpageSize - (size_t)vaddr % kHpageSize) % kHpageSize; On 2016/02/01 16:46:15, Daniel Erat wrote: > use c++-style casts (e.g. static_cast<size_t>()) here and elsewhere instead of > c-style casts Done.
yunlian@chromium.org changed reviewers: + rickyz@chromium.org - rickyz@google.com
On 2016/02/01 17:31:56, yunlian wrote: > https://codereview.chromium.org/1619713007/diff/20001/chromeos/hugepage_text/... > File chromeos/hugepage_text/hugepage_text.cc (right): > > https://codereview.chromium.org/1619713007/diff/20001/chromeos/hugepage_text/... > chromeos/hugepage_text/hugepage_text.cc:176: size_t head_gap = (kHpageSize - > (size_t)vaddr % kHpageSize) % kHpageSize; > On 2016/02/01 16:46:15, Daniel Erat wrote: > > use c++-style casts (e.g. static_cast<size_t>()) here and elsewhere instead of > > c-style casts > > Done. ping?
lgtm https://codereview.chromium.org/1619713007/diff/40001/chromeos/hugepage_text/... File chromeos/hugepage_text/hugepage_text.cc (right): https://codereview.chromium.org/1619713007/diff/40001/chromeos/hugepage_text/... chromeos/hugepage_text/hugepage_text.cc:129: if ((reinterpret_cast<intptr_t>(vaddr) & ~kHpageMask) == 0 && You can probably just DCHECK_EQ(0, reinterpret_cast<uintptr_t>(vaddr) & ~kHpageMask) in this function now
https://codereview.chromium.org/1619713007/diff/40001/chromeos/hugepage_text/... File chromeos/hugepage_text/hugepage_text.cc (right): https://codereview.chromium.org/1619713007/diff/40001/chromeos/hugepage_text/... chromeos/hugepage_text/hugepage_text.cc:129: if ((reinterpret_cast<intptr_t>(vaddr) & ~kHpageMask) == 0 && On 2016/02/03 21:01:31, rickyz wrote: > You can probably just DCHECK_EQ(0, reinterpret_cast<uintptr_t>(vaddr) & > ~kHpageMask) in this function now What is the difference between the expression and the DCHECK_EQ? My understanding is DCHECK_EQ can store the result in the log, it that right?
https://codereview.chromium.org/1619713007/diff/40001/chromeos/hugepage_text/... File chromeos/hugepage_text/hugepage_text.cc (right): https://codereview.chromium.org/1619713007/diff/40001/chromeos/hugepage_text/... chromeos/hugepage_text/hugepage_text.cc:129: if ((reinterpret_cast<intptr_t>(vaddr) & ~kHpageMask) == 0 && On 2016/02/03 at 21:21:51, yunlian wrote: > On 2016/02/03 21:01:31, rickyz wrote: > > You can probably just DCHECK_EQ(0, reinterpret_cast<uintptr_t>(vaddr) & > > ~kHpageMask) in this function now > > What is the difference between the expression and the DCHECK_EQ? > My understanding is DCHECK_EQ can store the result in the log, it that > right? On debug builds, DCHECK would cause a crash if the condition isn't true. With this change, it looks like MremapHugetlbText should only ever be called with a hugepage aligned address, so I was suggesting that instead of checking for a condition that should be always true, you could just make it a requirement of the function (and assert that the requirement is satisfied with a DCHECK).
https://codereview.chromium.org/1619713007/diff/40001/chromeos/hugepage_text/... File chromeos/hugepage_text/hugepage_text.cc (right): https://codereview.chromium.org/1619713007/diff/40001/chromeos/hugepage_text/... chromeos/hugepage_text/hugepage_text.cc:129: if ((reinterpret_cast<intptr_t>(vaddr) & ~kHpageMask) == 0 && On 2016/02/03 21:28:24, rickyz wrote: > On 2016/02/03 at 21:21:51, yunlian wrote: > > On 2016/02/03 21:01:31, rickyz wrote: > > > You can probably just DCHECK_EQ(0, reinterpret_cast<uintptr_t>(vaddr) & > > > ~kHpageMask) in this function now > > > > What is the difference between the expression and the DCHECK_EQ? > > My understanding is DCHECK_EQ can store the result in the log, it that > > right? > > On debug builds, DCHECK would cause a crash if the condition isn't true. With > this change, it looks like MremapHugetlbText should only ever be called with a > hugepage aligned address, so I was suggesting that instead of checking for a > condition that should be always true, you could just make it a requirement of > the function (and assert that the requirement is satisfied with a DCHECK). Done.
https://codereview.chromium.org/1619713007/diff/60001/chromeos/hugepage_text/... File chromeos/hugepage_text/hugepage_text.cc (right): https://codereview.chromium.org/1619713007/diff/60001/chromeos/hugepage_text/... chromeos/hugepage_text/hugepage_text.cc:129: if (DCHECK_EQ(0, reinterpret_cast<uintptr_t>(vaddr) & ~kHpageMask) && Er sorry, what I meant was something like: // Inputs: vaddr, the starting virtual address to remap to hugepage, // must be aligned to kHpageSize. ... static void MremapHugetlbText(void* vaddr, const size_t hsize) { DCHECK_EQ(0, reinterpret_cast<uintptr_t>(vaddr) & ~kHpageMask); ... if (HugetlbMremapSupported()) { ...
https://codereview.chromium.org/1619713007/diff/60001/chromeos/hugepage_text/... File chromeos/hugepage_text/hugepage_text.cc (right): https://codereview.chromium.org/1619713007/diff/60001/chromeos/hugepage_text/... chromeos/hugepage_text/hugepage_text.cc:129: if (DCHECK_EQ(0, reinterpret_cast<uintptr_t>(vaddr) & ~kHpageMask) && On 2016/02/03 23:10:09, rickyz wrote: > Er sorry, what I meant was something like: > > // Inputs: vaddr, the starting virtual address to remap to hugepage, > // must be aligned to kHpageSize. > ... > static void MremapHugetlbText(void* vaddr, const size_t hsize) { > DCHECK_EQ(0, reinterpret_cast<uintptr_t>(vaddr) & ~kHpageMask); > ... > if (HugetlbMremapSupported()) { > ... Done.
On 2016/02/03 23:14:45, yunlian wrote: > https://codereview.chromium.org/1619713007/diff/60001/chromeos/hugepage_text/... > File chromeos/hugepage_text/hugepage_text.cc (right): > > https://codereview.chromium.org/1619713007/diff/60001/chromeos/hugepage_text/... > chromeos/hugepage_text/hugepage_text.cc:129: if (DCHECK_EQ(0, > reinterpret_cast<uintptr_t>(vaddr) & ~kHpageMask) && > On 2016/02/03 23:10:09, rickyz wrote: > > Er sorry, what I meant was something like: > > > > // Inputs: vaddr, the starting virtual address to remap to hugepage, > > // must be aligned to kHpageSize. > > ... > > static void MremapHugetlbText(void* vaddr, const size_t hsize) { > > DCHECK_EQ(0, reinterpret_cast<uintptr_t>(vaddr) & ~kHpageMask); > > ... > > if (HugetlbMremapSupported()) { > > ... > > Done. ping?
lgtm
The CQ bit was checked by yunlian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shenhan@chromium.org, kenchen@google.com Link to the patchset: https://codereview.chromium.org/1619713007/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1619713007/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1619713007/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yunlian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shenhan@chromium.org, rickyz@chromium.org, kenchen@google.com Link to the patchset: https://codereview.chromium.org/1619713007/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1619713007/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1619713007/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yunlian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shenhan@chromium.org, rickyz@chromium.org, kenchen@google.com Link to the patchset: https://codereview.chromium.org/1619713007/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1619713007/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1619713007/120001
Message was sent while issue was closed.
Description was changed from ========== Make text section 2MB aligned for hugepages. If we do not use the option '-Wl,--section-start=.text=2001000' at link time, the performance gain on hugepages is gone. This is probably because the original text section is not 2MB aligned and the after mremap, the hugepage is not preserved. BUG=chromium:569963 TEST=The performance gain is back on micro benchmarks. ========== to ========== Make text section 2MB aligned for hugepages. If we do not use the option '-Wl,--section-start=.text=2001000' at link time, the performance gain on hugepages is gone. This is probably because the original text section is not 2MB aligned and the after mremap, the hugepage is not preserved. BUG=chromium:569963 TEST=The performance gain is back on micro benchmarks. ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Make text section 2MB aligned for hugepages. If we do not use the option '-Wl,--section-start=.text=2001000' at link time, the performance gain on hugepages is gone. This is probably because the original text section is not 2MB aligned and the after mremap, the hugepage is not preserved. BUG=chromium:569963 TEST=The performance gain is back on micro benchmarks. ========== to ========== Make text section 2MB aligned for hugepages. If we do not use the option '-Wl,--section-start=.text=2001000' at link time, the performance gain on hugepages is gone. This is probably because the original text section is not 2MB aligned and the after mremap, the hugepage is not preserved. BUG=chromium:569963 TEST=The performance gain is back on micro benchmarks. Committed: https://crrev.com/1384534a94e6324d74906f0cbc1e707606c063f2 Cr-Commit-Position: refs/heads/master@{#373673} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1384534a94e6324d74906f0cbc1e707606c063f2 Cr-Commit-Position: refs/heads/master@{#373673}
Message was sent while issue was closed.
On 2016/02/05 00:11:33, commit-bot: I haz the power wrote: > Patchset 7 (id:??) landed as > https://crrev.com/1384534a94e6324d74906f0cbc1e707606c063f2 > Cr-Commit-Position: refs/heads/master@{#373673} Somehow this is causing failures on the following devices: x86-alex peach_pit daisy_skate Unless there is a quick fix available we need to revert this. The PFQ has not updated in 5 days.
Message was sent while issue was closed.
I can try to make a fix today On Wed, Feb 10, 2016 at 8:40 AM, <stevenjb@chromium.org> wrote: > > On 2016/02/05 00:11:33, commit-bot: I haz the power wrote: > > Patchset 7 (id:??) landed as > > https://crrev.com/1384534a94e6324d74906f0cbc1e707606c063f2 > > Cr-Commit-Position: refs/heads/master@{#373673} > > Somehow this is causing failures on the following devices: > x86-alex > peach_pit > daisy_skate > > Unless there is a quick fix available we need to revert this. The PFQ has not > updated in 5 days. > > https://codereview.chromium.org/1619713007/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/1687033002/ by stevenjb@chromium.org. The reason for reverting is: Causing failures on x86-alex, peach_pit, and daisy_skate (render process crash in login screen). . |
