This is an archive of the discontinued LLVM Phabricator instance.

scudo-standalone: Add GetRSS method on Linux
ClosedPublic

Authored by 1c3t3a on Dec 6 2022, 6:59 AM.

Details

Summary

This change adds a GetRSS method on Linux that parses
the number from /proc/self/statm. This change is part
of splitting up https://reviews.llvm.org/D126752.

Diff Detail

Event Timeline

1c3t3a created this revision.Dec 6 2022, 6:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2022, 6:59 AM
1c3t3a requested review of this revision.Dec 6 2022, 6:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2022, 6:59 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
1c3t3a updated this revision to Diff 480489.EditedDec 6 2022, 7:42 AM

Fix clang-format error

1c3t3a updated this revision to Diff 480492.EditedDec 6 2022, 7:49 AM

Add implementation for GetRSS and GetRSSFromBuffer for different platforms.

1c3t3a updated this revision to Diff 480493.EditedDec 6 2022, 7:56 AM

Only run tests on Linux

vitalybuka added inline comments.Dec 6 2022, 11:32 AM
compiler-rt/lib/scudo/standalone/linux.cpp
171

It looks unrelated

184–188

I don't see scudo uses internal_ names

184–192
192
206

looks wrong
and please use static_cast

compiler-rt/lib/scudo/standalone/tests/common_test.cpp
72

is this going to work on Android?
I assume SCUDO_LINUX is android as well?

1c3t3a updated this revision to Diff 480809.Dec 7 2022, 1:22 AM
1c3t3a marked 5 inline comments as done.

Add requested changes

1c3t3a added a comment.Dec 7 2022, 1:22 AM

Thanks for the review! @vitalybuka :)

compiler-rt/lib/scudo/standalone/tests/common_test.cpp
72

Let me check that.

1c3t3a updated this revision to Diff 480952.Dec 7 2022, 9:28 AM

Include check for procfile in test

vitalybuka added inline comments.Dec 7 2022, 11:16 AM
compiler-rt/lib/scudo/standalone/linux.cpp
184–192

static is not done?

198

It's correct, but () are redundant
I would prefer *(pos++) for better precendence visibility

or even just make it for loop

for (;*pos >= '0' && *pos <= '9'; ++pos)
   rss = rss * 10 + static_cast<u64>(*pos) - '0'
compiler-rt/lib/scudo/standalone/tests/common_test.cpp
74–83

can you please hide everything as static
and only test GetRSS()

you can e.g. new int[10000000], memset, and see that RSS before and after is ~10M

1c3t3a updated this revision to Diff 481179.Dec 8 2022, 12:42 AM
1c3t3a marked 3 inline comments as done.

Add requested changes

This comment was removed by 1c3t3a.
vitalybuka accepted this revision.Dec 8 2022, 12:44 AM
This revision is now accepted and ready to land.Dec 8 2022, 12:44 AM

Can we use the LLVM style for this change?
eg: no k* constants, casing for pos, fd etc.

1c3t3a updated this revision to Diff 481297.Dec 8 2022, 8:44 AM

Adopt to LLVM naming rules

1c3t3a added a comment.EditedDec 8 2022, 8:44 AM

Can we use the LLVM style for this change?
eg: no k* constants, casing for pos, fd etc.

Done :) I don't have commit access to LLVM, can you or someone else commit the patch for me?

cryptoad accepted this revision.Dec 8 2022, 9:03 AM

Thanks! Missed one :)

compiler-rt/lib/scudo/standalone/internal_defs.h
93 ↗(On Diff #481297)

LLVM naming + no C-cast please

1c3t3a updated this revision to Diff 481326.Dec 8 2022, 9:17 AM
1c3t3a marked an inline comment as done.

Fix missing C-cast

vitalybuka updated this revision to Diff 481436.Dec 8 2022, 1:37 PM

remove unneded inlcudes and declarations
improve the test

This revision was landed with ongoing or failed builds.Dec 8 2022, 1:38 PM
This revision was automatically updated to reflect the committed changes.
saghir added a subscriber: saghir.Dec 9 2022, 7:00 AM

Just a heads up that this seems to be breaking one of our bots: https://lab.llvm.org/staging/#/builders/14/builds/1305.

I am building LLVM to confirm this, will update in a bit.

1c3t3a added a comment.Dec 9 2022, 8:37 AM

Oh. I think I know where this comes from - for fixing this: Would I submit to this changelist or open a new one?

saghir added a comment.Dec 9 2022, 8:50 AM

Oh. I think I know where this comes from - for fixing this: Would I submit to this changelist or open a new one?

Thanks for taking a look at this!
If it's a minor one line fix then you can just add another patch. If it needs significant changes to fix, then you can revert the patch, fix it and then commit it again...
If you were asking about using this phabricator review, I think this one is closed and a new one would be needed.
Let me know if you need any help.
Thanks again!

I'll fix it in a few min.

Oh. I think I know where this comes from - for fixing this: Would I submit to this changelist or open a new one?

Thanks for taking a look at this!
If it's a minor one line fix then you can just add another patch. If it needs significant changes to fix, then you can revert the patch, fix it and then commit it again...
If you were asking about using this phabricator review, I think this one is closed and a new one would be needed.
Let me know if you need any help.
Thanks again!

amyk added a subscriber: amyk.Feb 14 2023, 12:09 PM

Hi there @vitalybuka @1c3t3a! I am now realizing that we are experiencing some intermittent failures with the GetRssFromBuffer test case (sometimes it fails, and sometimes it passes) on PPC.

Specifically, the EXPECT_LE() failure (when it does fail) will look like this, where the Rss - AllocSize - Prev can succeed the Error (3000000):

/home/amyk/community/llvm-project/compiler-rt/lib/scudo/standalone/tests/common_test.cpp:93: Failure
Expected: (std::abs(Rss - AllocSize - Prev)) <= (Error), actual: 3708544 vs 3000000

I am noticing a couple things with respect to this test case:

  • This test can can fail on any iteration of the for loop (sometimes it can fail on the first iteration, other times the ninth, etc.)
  • The amount that it fails with is always 3708544. It is always 708544 more than the expected error.
  • I could be mistaken, but the Rss prior to the EXPECT_LE() check always appears to be less than Prev + AllocSize. Perhaps something is being deallocated here?

Basically, I'd like to try to understand this test case a bit more and was wondering if any of you have any thoughts with regards to this issue I am seeing on this test case.
Moreover, I was wondering if there was a specific justification behind having Error be 3000000, as we can see in my example, it is possible to exceed this value.

Any assistance and/or insight would be greatly appreciated! Thanks in advance.

Anther way to test this is instead of failure on the first EXPECT_LE, we rather count mismatches and after the loop we check that count is e.g. <=3

amyk added a comment.Feb 14 2023, 12:46 PM

Anther way to test this is instead of failure on the first EXPECT_LE, we rather count mismatches and after the loop we check that count is e.g. <=3

That could be possible. Just curious, is 3 just an arbitrary value that we can pick, or is there any particular reason that we would pick that?

it's totally made up: 3 out of 10

amyk added a comment.Feb 21 2023, 8:25 AM

it's totally made up: 3 out of 10

Thanks @vitalybuka for the explanation. Were we still planning to update this test case in the future?

it's totally made up: 3 out of 10

Thanks @vitalybuka for the explanation. Were we still planning to update this test case in the future?

I am not looking into that. Would you be interested to send a patch to improves the test for your platform?

amyk added a comment.Feb 23 2023, 6:28 AM

it's totally made up: 3 out of 10

Thanks @vitalybuka for the explanation. Were we still planning to update this test case in the future?

I am not looking into that. Would you be interested to send a patch to improves the test for your platform?

Thanks for getting back to me, @vitalybuka! I've discussed this offline with a few others from our backend and we are deciding to disable this test on Power for the time being. I will follow up with a commit shortly for this.

amyk added a comment.Feb 23 2023, 9:58 AM

it's totally made up: 3 out of 10

Thanks @vitalybuka for the explanation. Were we still planning to update this test case in the future?

I am not looking into that. Would you be interested to send a patch to improves the test for your platform?

Thanks for getting back to me, @vitalybuka! I've discussed this offline with a few others from our backend and we are deciding to disable this test on Power for the time being. I will follow up with a commit shortly for this.

I have committed this at 0e3ef5f89749a599c652c546214a8307e3495526.