Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2070083002:
Add Linux MemAvailable to SystemMemoryInfoKB (Closed)
DescriptionAdd Linux MemAvailable to SystemMemoryInfoKB
This provides a better estimate of available memory:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34e431b0ae398fc54ea69ff85ec700722c9da773
We would like to use it in Chromecast's memory pressure monitor.
BUG=620532
Committed: https://crrev.com/c2eb0f97d26fc468881340104dbea0545808f375
Cr-Commit-Position: refs/heads/master@{#400472}
Patch Set 1 #Patch Set 2 : Include Android + add comment on kernel version #Patch Set 3 : Exclude Android and improve commenting #
Messages
Total messages: 36 (11 generated)
Description was changed from ========== Add Linux MemAvailable to SystemMemoryInfoKB This provides a better estimate of available memory: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34... We would like to use it in Chromecast's memory pressure monitor. BUG= ========== to ========== Add Linux MemAvailable to SystemMemoryInfoKB This provides a better estimate of available memory: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34... We would like to use it in Chromecast's memory pressure monitor. BUG= ==========
halliwell@chromium.org changed reviewers: + thestig@chromium.org
What's the minimum kernel version for MemAvailable? I don't have it on this computer for instance. My concern is that others will try to use this, but it's 0 for older kernels and that may be unexpected.
On 2016/06/15 21:45:52, Lei Zhang wrote: > What's the minimum kernel version for MemAvailable? I don't have it on this > computer for instance. My concern is that others will try to use this, but it's > 0 for older kernels and that may be unexpected. I think it's 3.14? Can add a comment about this if that's useful?
On 2016/06/15 22:44:48, halliwell wrote: > On 2016/06/15 21:45:52, Lei Zhang wrote: > > What's the minimum kernel version for MemAvailable? I don't have it on this > > computer for instance. My concern is that others will try to use this, but > it's > > 0 for older kernels and that may be unexpected. > > I think it's 3.14? > > Can add a comment about this if that's useful? Sure. I also have no idea off the top of my head what kernel versions Android devices run, but this might be useful on OS_ANDROID as well?
+nyquist randomly from base/android/OWNERS to get the Android perspective.
On 2016/06/15 22:57:40, Lei Zhang wrote: > On 2016/06/15 22:44:48, halliwell wrote: > > On 2016/06/15 21:45:52, Lei Zhang wrote: > > > What's the minimum kernel version for MemAvailable? I don't have it on this > > > computer for instance. My concern is that others will try to use this, but > > it's > > > 0 for older kernels and that may be unexpected. > > > > I think it's 3.14? > > > > Can add a comment about this if that's useful? > > Sure. I also have no idea off the top of my head what kernel versions Android > devices run, but this might be useful on OS_ANDROID as well? Good point. According to http://android.stackexchange.com/questions/51651/which-android-runs-which-lin... it should be in Lollipop. Will update.
The CQ bit was checked by halliwell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2070083002/20001
On 2016/06/15 22:59:28, halliwell wrote: > On 2016/06/15 22:57:40, Lei Zhang wrote: > > On 2016/06/15 22:44:48, halliwell wrote: > > > On 2016/06/15 21:45:52, Lei Zhang wrote: > > > > What's the minimum kernel version for MemAvailable? I don't have it on > this > > > > computer for instance. My concern is that others will try to use this, but > > > it's > > > > 0 for older kernels and that may be unexpected. > > > > > > I think it's 3.14? > > > > > > Can add a comment about this if that's useful? > > > > Sure. I also have no idea off the top of my head what kernel versions Android > > devices run, but this might be useful on OS_ANDROID as well? > > Good point. According to > http://android.stackexchange.com/questions/51651/which-android-runs-which-lin... > it should be in Lollipop. Will update. Updated with these changes.
lgtm If this is not urgent, wait a bit and see if nyquist@ has any additional comments?
And if there is a crbug number, please add it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add Linux MemAvailable to SystemMemoryInfoKB This provides a better estimate of available memory: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34... We would like to use it in Chromecast's memory pressure monitor. BUG= ========== to ========== Add Linux MemAvailable to SystemMemoryInfoKB This provides a better estimate of available memory: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34... We would like to use it in Chromecast's memory pressure monitor. BUG=620532 ==========
On 2016/06/16 00:55:17, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Bug number added. nyquist: PTAL, this is somewhat urgent, would like to submit tomorrow morning.
On 2016/06/16 01:13:15, halliwell wrote: > On 2016/06/16 00:55:17, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > Bug number added. nyquist: PTAL, this is somewhat urgent, would like to submit > tomorrow morning. +torne too. If everyone is on holiday, feel free to hit submit button tomorrow morning.
On 2016/06/15 22:59:28, halliwell wrote: > On 2016/06/15 22:57:40, Lei Zhang wrote: > > On 2016/06/15 22:44:48, halliwell wrote: > > > On 2016/06/15 21:45:52, Lei Zhang wrote: > > > > What's the minimum kernel version for MemAvailable? I don't have it on > this > > > > computer for instance. My concern is that others will try to use this, but > > > it's > > > > 0 for older kernels and that may be unexpected. > > > > > > I think it's 3.14? > > > > > > Can add a comment about this if that's useful? > > > > Sure. I also have no idea off the top of my head what kernel versions Android > > devices run, but this might be useful on OS_ANDROID as well? > > Good point. According to > http://android.stackexchange.com/questions/51651/which-android-runs-which-lin... > it should be in Lollipop. Will update. Chrome on Android supports JB and KK devices still, not just L, and the kernel versions listed there are *not* guaranteed to be the minimum for that Android release in any case (that's just the kernel version we use for Nexus devices and the emulator - other device vendors may use newer or older versions). There are definitely many supported Android devices that have kernels older than 3.14. What's this going to be used for? Having it set to 0 for a bunch of devices seems like a problem waiting to happen :/
On 2016/06/16 10:22:16, Torne wrote: > On 2016/06/15 22:59:28, halliwell wrote: > > On 2016/06/15 22:57:40, Lei Zhang wrote: > > > On 2016/06/15 22:44:48, halliwell wrote: > > > > On 2016/06/15 21:45:52, Lei Zhang wrote: > > > > > What's the minimum kernel version for MemAvailable? I don't have it on > > this > > > > > computer for instance. My concern is that others will try to use this, > but > > > > it's > > > > > 0 for older kernels and that may be unexpected. > > > > > > > > I think it's 3.14? > > > > > > > > Can add a comment about this if that's useful? > > > > > > Sure. I also have no idea off the top of my head what kernel versions > Android > > > devices run, but this might be useful on OS_ANDROID as well? > > > > Good point. According to > > > http://android.stackexchange.com/questions/51651/which-android-runs-which-lin... > > it should be in Lollipop. Will update. > > Chrome on Android supports JB and KK devices still, not just L, and the kernel > versions listed there are *not* guaranteed to be the minimum for that Android > release in any case (that's just the kernel version we use for Nexus devices and > the emulator - other device vendors may use newer or older versions). There are > definitely many supported Android devices that have kernels older than 3.14. > > What's this going to be used for? Having it set to 0 for a bunch of devices > seems like a problem waiting to happen :/ As mentioned in bug + description, plan is to use it on Chromecast linux. Would you prefer to just ifdef for LINUX (as in earlier patchset here)?
On 2016/06/16 11:37:21, halliwell wrote: > On 2016/06/16 10:22:16, Torne wrote: > > On 2016/06/15 22:59:28, halliwell wrote: > > > On 2016/06/15 22:57:40, Lei Zhang wrote: > > > > On 2016/06/15 22:44:48, halliwell wrote: > > > > > On 2016/06/15 21:45:52, Lei Zhang wrote: > > > > > > What's the minimum kernel version for MemAvailable? I don't have it on > > > this > > > > > > computer for instance. My concern is that others will try to use this, > > but > > > > > it's > > > > > > 0 for older kernels and that may be unexpected. > > > > > > > > > > I think it's 3.14? > > > > > > > > > > Can add a comment about this if that's useful? > > > > > > > > Sure. I also have no idea off the top of my head what kernel versions > > Android > > > > devices run, but this might be useful on OS_ANDROID as well? > > > > > > Good point. According to > > > > > > http://android.stackexchange.com/questions/51651/which-android-runs-which-lin... > > > it should be in Lollipop. Will update. > > > > Chrome on Android supports JB and KK devices still, not just L, and the kernel > > versions listed there are *not* guaranteed to be the minimum for that Android > > release in any case (that's just the kernel version we use for Nexus devices > and > > the emulator - other device vendors may use newer or older versions). There > are > > definitely many supported Android devices that have kernels older than 3.14. > > > > What's this going to be used for? Having it set to 0 for a bunch of devices > > seems like a problem waiting to happen :/ > > As mentioned in bug + description, plan is to use it on Chromecast linux. Would > you prefer to just ifdef for LINUX (as in earlier patchset here)? ping on this question for Android OWNERS.
Torne's concern applies to Linux too. It all depends on how the caller uses it and whether they know some of these fields may return 0 or -1 when the field is invalid/non available. BTW, I also noticed the free field above. That seems confusing. Maybe more documentation is needed?
On 2016/06/16 22:50:12, Lei Zhang wrote: > Torne's concern applies to Linux too. It all depends on how the caller uses it > and whether they know some of these fields may return 0 or -1 when the field is > invalid/non available. > > BTW, I also noticed the free field above. That seems confusing. Maybe more > documentation is needed? As far as understanding free vs available, I could add link to the official docs (https://www.kernel.org/doc/Documentation/filesystems/proc.txt)? And clarify it'll be 0 for pre-3.14 kernels. Although Android is a similar concern, the difference is that I have no intention to use this on Android right now. So it seems reasonable to ifdef it out to protect against misuse. For Linux, I don't see a good way besides commenting.
On 2016/06/16 23:11:37, halliwell wrote: > On 2016/06/16 22:50:12, Lei Zhang wrote: > > Torne's concern applies to Linux too. It all depends on how the caller uses it > > and whether they know some of these fields may return 0 or -1 when the field > is > > invalid/non available. > > > > BTW, I also noticed the free field above. That seems confusing. Maybe more > > documentation is needed? > > As far as understanding free vs available, I could add link to the official docs > (https://www.kernel.org/doc/Documentation/filesystems/proc.txt) And clarify > it'll be 0 for pre-3.14 kernels. https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34... is more helpful and it includes the relevant changes to proc.txt. Though proc.txt change in the future, but I would hope the things exposed through /proc remain stable. > Although Android is a similar concern, the difference is that I have no > intention to use this on Android right now. So it seems reasonable to ifdef it > out to protect against misuse. For Linux, I don't see a good way besides > commenting. If you want to go back to patch set 1 + more documentation, I'm fine with that as well.
On 2016/06/16 23:19:37, Lei Zhang wrote: > On 2016/06/16 23:11:37, halliwell wrote: > > On 2016/06/16 22:50:12, Lei Zhang wrote: > > > Torne's concern applies to Linux too. It all depends on how the caller uses > it > > > and whether they know some of these fields may return 0 or -1 when the field > > is > > > invalid/non available. > > > > > > BTW, I also noticed the free field above. That seems confusing. Maybe more > > > documentation is needed? > > > > As far as understanding free vs available, I could add link to the official > docs > > (https://www.kernel.org/doc/Documentation/filesystems/proc.txt) And clarify > > it'll be 0 for pre-3.14 kernels. > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34... > is more helpful and it includes the relevant changes to proc.txt. Though > proc.txt change in the future, but I would hope the things exposed through /proc > remain stable. > > > Although Android is a similar concern, the difference is that I have no > > intention to use this on Android right now. So it seems reasonable to ifdef > it > > out to protect against misuse. For Linux, I don't see a good way besides > > commenting. > > If you want to go back to patch set 1 + more documentation, I'm fine with that > as well. Ok, back to PS1 with improved commenting. I'll submit around midday tomorrow if no further comments.
The CQ bit was checked by halliwell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2070083002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by halliwell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2070083002/#ps40001 (title: "Exclude Android and improve commenting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2070083002/40001
Message was sent while issue was closed.
Description was changed from ========== Add Linux MemAvailable to SystemMemoryInfoKB This provides a better estimate of available memory: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34... We would like to use it in Chromecast's memory pressure monitor. BUG=620532 ========== to ========== Add Linux MemAvailable to SystemMemoryInfoKB This provides a better estimate of available memory: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34... We would like to use it in Chromecast's memory pressure monitor. BUG=620532 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add Linux MemAvailable to SystemMemoryInfoKB This provides a better estimate of available memory: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34... We would like to use it in Chromecast's memory pressure monitor. BUG=620532 ========== to ========== Add Linux MemAvailable to SystemMemoryInfoKB This provides a better estimate of available memory: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=34... We would like to use it in Chromecast's memory pressure monitor. BUG=620532 Committed: https://crrev.com/c2eb0f97d26fc468881340104dbea0545808f375 Cr-Commit-Position: refs/heads/master@{#400472} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c2eb0f97d26fc468881340104dbea0545808f375 Cr-Commit-Position: refs/heads/master@{#400472} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
