Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Dec 15, 2021

This was split off from #129 per @osandov's request.

In preparation for introducing an API to represent threads, the linux
helper iterators, radix_tree_for_each, idr_for_each, for_each_pid, and
for_each_task have been rewritten in C. This will allow them to be
accessed from libdrgn, which will be necessary for the threads API.

Signed-off-by: Kevin Svetlitski [email protected]

@ghost ghost force-pushed the rewrite-linux-iterators branch 9 times, most recently from 4d79288 to e0956c8 Compare December 17, 2021 00:46
@ghost
Copy link
Author

ghost commented Dec 17, 2021

@osandov All of your comments on the iterator implementations have been addressed here, this PR is ready for review.

@ghost ghost force-pushed the rewrite-linux-iterators branch from e0956c8 to 6b3ef3e Compare December 17, 2021 20:51
osandov added a commit that referenced this pull request Dec 17, 2021
PR #133 adds a test case using multiprocessing.Barrier(), which needs
/dev/shm.

Signed-off-by: Omar Sandoval <[email protected]>
@ghost ghost force-pushed the rewrite-linux-iterators branch from 6b3ef3e to e23e187 Compare December 17, 2021 21:26
@ghost ghost force-pushed the rewrite-linux-iterators branch 3 times, most recently from 6dc8f0a to ff4c3a5 Compare December 17, 2021 22:54
@ghost ghost requested a review from osandov December 17, 2021 22:55
Copy link
Owner

@osandov osandov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few trivial things. Are those test failures in your temporary test just because the vmtest environment doesn't have many processes? Once you get rid of the temporary shims, I'll just clean up trivial code style issues myself (I wish clang-format was usable for the Linux kernel style).

@ghost ghost force-pushed the rewrite-linux-iterators branch from ff4c3a5 to 67e98fa Compare December 17, 2021 23:31
@ghost
Copy link
Author

ghost commented Dec 17, 2021

Just a few trivial things. Are those test failures in your temporary test just because the vmtest environment doesn't have many processes? Once you get rid of the temporary shims, I'll just clean up trivial code style issues myself (I wish clang-format was usable for the Linux kernel style).

The failures are because we now iterate the pid_hash in a different order. FWIW, I've been using clang-format thus far with very little manual tweaking necessary.

@osandov
Copy link
Owner

osandov commented Dec 17, 2021

This is the failure I'm referring to:

======================================================================
FAIL: test_for_each_pid (tests.helpers.linux.test_c_helpers.TestCHelpers)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/drgn/drgn/tests/helpers/linux/test_c_helpers.py", line 24, in test_for_each_pid
    self.verify_iterators_match(old_for_each_pid, for_each_pid, self.prog)
  File "/home/runner/work/drgn/drgn/tests/helpers/linux/test_c_helpers.py", line 17, in verify_iterators_match
    self.assertTrue(sum(1 for _ in old_iter(arg)) >= 100)
AssertionError: False is not true

@ghost ghost force-pushed the rewrite-linux-iterators branch from 67e98fa to f5e3c55 Compare December 17, 2021 23:35
@ghost
Copy link
Author

ghost commented Dec 17, 2021

Alright, temporary shims have been removed, and everything has been combined into a single commit.

@ghost
Copy link
Author

ghost commented Dec 17, 2021

This is the failure I'm referring to:

======================================================================
FAIL: test_for_each_pid (tests.helpers.linux.test_c_helpers.TestCHelpers)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/drgn/drgn/tests/helpers/linux/test_c_helpers.py", line 24, in test_for_each_pid
    self.verify_iterators_match(old_for_each_pid, for_each_pid, self.prog)
  File "/home/runner/work/drgn/drgn/tests/helpers/linux/test_c_helpers.py", line 17, in verify_iterators_match
    self.assertTrue(sum(1 for _ in old_iter(arg)) >= 100)
AssertionError: False is not true

I would assume so yes, as old_iter is the pure-Python implementation that you wrote, and I didn't touch those so I don't know how that would be broken.

@ghost
Copy link
Author

ghost commented Dec 17, 2021

I'll fix the isort real quick

@ghost ghost force-pushed the rewrite-linux-iterators branch from f5e3c55 to 27774c6 Compare December 17, 2021 23:42
In preparation for introducing an API to represent threads, the linux
helper iterators, radix_tree_for_each, idr_for_each, for_each_pid, and
for_each_task have been rewritten in C. This will allow them to be
accessed from libdrgn, which will be necessary for the threads API.

Signed-off-by: Kevin Svetlitski <[email protected]>
@osandov osandov force-pushed the rewrite-linux-iterators branch from 27774c6 to 3142141 Compare December 18, 2021 00:00
Copy link
Owner

@osandov osandov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed to make isort and black happy. Once those tests all pass, I'll merge this.

@osandov osandov merged commit 2b47583 into osandov:main Dec 18, 2021
@ghost ghost deleted the rewrite-linux-iterators branch March 9, 2022 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants