Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1320493006:
Oilpan: Add a RELEASE_ASSERT to catch out-of-virtual-address-space errors (Closed)
|
Created:
5 years, 3 months ago by haraken Modified:
5 years, 3 months ago CC:
blink-reviews, haraken, oilpan-reviews, kouhei+heap_chromium.org, Mads Ager (chromium) Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOilpan: Add a RELEASE_ASSERT to catch out-of-virtual-address-space errors
Currently we run into an infinite loop when we hit the limit of the number
of mmapped regions OS can support. It is better to have a RELEASE_ASSERT
about it.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201892
Patch Set 1 #Patch Set 2 : #
Total comments: 5
Messages
Total messages: 12 (3 generated)
haraken@chromium.org changed reviewers: + keishi@chromium.org
PTAL
LGTM
The CQ bit was checked by haraken@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320493006/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320493006/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201892
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
https://codereview.chromium.org/1320493006/diff/20001/Source/platform/heap/He... File Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1320493006/diff/20001/Source/platform/heap/He... Source/platform/heap/HeapPage.cpp:552: RELEASE_ASSERT(numberOfTrials < 64); Could you explain a bit more what you were seeing? The PageMemoryRegion was able to allocate (and not OOMedly crash), but something failed when setting up the region?
Message was sent while issue was closed.
https://codereview.chromium.org/1320493006/diff/20001/Source/platform/heap/He... File Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1320493006/diff/20001/Source/platform/heap/He... Source/platform/heap/HeapPage.cpp:552: RELEASE_ASSERT(numberOfTrials < 64); On 2015/09/08 05:30:34, sof wrote: > Could you explain a bit more what you were seeing? The PageMemoryRegion was able > to allocate (and not OOMedly crash), but something failed when setting up the > region? The above memory->commit() starts failing (because mprotect fails) after we map more than 65530 (/proc/sys/vm/max_map_count) regions.
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/1320493006/diff/20001/Source/platform/heap/He... File Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1320493006/diff/20001/Source/platform/heap/He... Source/platform/heap/HeapPage.cpp:552: RELEASE_ASSERT(numberOfTrials < 64); On 2015/09/08 05:35:19, haraken wrote: > On 2015/09/08 05:30:34, sof wrote: > > Could you explain a bit more what you were seeing? The PageMemoryRegion was > able > > to allocate (and not OOMedly crash), but something failed when setting up the > > region? > > The above memory->commit() starts failing (because mprotect fails) after we map > more than 65530 (/proc/sys/vm/max_map_count) regions. > And once it does & that OS' limit is reached, the remaining blink pages will not be committed. I suppose a different way to handle it is to catch if all commit()s fail for a region. But once we hit this resource exhausted state, a few more of these loop iterations won't make much of a difference.
Message was sent while issue was closed.
https://codereview.chromium.org/1320493006/diff/20001/Source/platform/heap/He... File Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1320493006/diff/20001/Source/platform/heap/He... Source/platform/heap/HeapPage.cpp:552: RELEASE_ASSERT(numberOfTrials < 64); On 2015/09/08 06:25:57, sof wrote: > On 2015/09/08 05:35:19, haraken wrote: > > On 2015/09/08 05:30:34, sof wrote: > > > Could you explain a bit more what you were seeing? The PageMemoryRegion was > > able > > > to allocate (and not OOMedly crash), but something failed when setting up > the > > > region? > > > > The above memory->commit() starts failing (because mprotect fails) after we > map > > more than 65530 (/proc/sys/vm/max_map_count) regions. > > > > And once it does & that OS' limit is reached, the remaining blink pages will not > be committed. > > I suppose a different way to handle it is to catch if all commit()s fail for a > region. But once we hit this resource exhausted state, a few more of these loop > iterations won't make much of a difference. Right. BTW, do you have any idea on why we have a 'while (!pageMemory)' loop? I guess it's somehow related to threading race, but I don't understand why it's needed. Also maybe can we just RELEASE_ASSERT once the first memory->commit() fails? I don't think it fails (except for the out-of-virtual-memory scenario).
Message was sent while issue was closed.
https://codereview.chromium.org/1320493006/diff/20001/Source/platform/heap/He... File Source/platform/heap/HeapPage.cpp (right): https://codereview.chromium.org/1320493006/diff/20001/Source/platform/heap/He... Source/platform/heap/HeapPage.cpp:552: RELEASE_ASSERT(numberOfTrials < 64); On 2015/09/08 06:33:14, haraken wrote: > On 2015/09/08 06:25:57, sof wrote: > > On 2015/09/08 05:35:19, haraken wrote: > > > On 2015/09/08 05:30:34, sof wrote: > > > > Could you explain a bit more what you were seeing? The PageMemoryRegion > was > > > able > > > > to allocate (and not OOMedly crash), but something failed when setting up > > the > > > > region? > > > > > > The above memory->commit() starts failing (because mprotect fails) after we > > map > > > more than 65530 (/proc/sys/vm/max_map_count) regions. > > > > > > > And once it does & that OS' limit is reached, the remaining blink pages will > not > > be committed. > > > > I suppose a different way to handle it is to catch if all commit()s fail for a > > region. But once we hit this resource exhausted state, a few more of these > loop > > iterations won't make much of a difference. > > Right. > > BTW, do you have any idea on why we have a 'while (!pageMemory)' loop? I guess > it's somehow related to threading race, but I don't understand why it's needed. > > Also maybe can we just RELEASE_ASSERT once the first memory->commit() fails? I > don't think it fails (except for the out-of-virtual-memory scenario). Yes, it looks out of place to have a loop there now. http://codereview.chromium.org/393823003 added it for the purposes of repeatedly polling the cross-thread-usable free page pool (after having filled it up with the pages of a new region.) As we now take the first page & don't add that to the pool, the loop looks redundant. |
