Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Pinned #983

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Remove Pinned #983

merged 2 commits into from
Feb 5, 2024

Conversation

cagatay-y
Copy link
Contributor

@cagatay-y cagatay-y commented Nov 23, 2023

Pinned was used to keep track of if a raw pointer to the object was handed out via the use of a boolean and free or not free accordingly. The initial commit replaces it with Rc to keep track of the sharing in a standard and safer way. The second commit eliminates the need to keep track of shares completely by removing the sharing of TransferTokens and adapting the only instance of the use of the returned TransferToken.

@cagatay-y cagatay-y force-pushed the pinned-removal branch 2 times, most recently from 3617a81 to c11e038 Compare November 30, 2023 14:10
@cagatay-y cagatay-y marked this pull request as ready for review November 30, 2023 14:17
@cagatay-y
Copy link
Contributor Author

I think it is possible that I introduced a memory leak. Can we profile the memory consumption of main and this branch to check?

@mkroening
Copy link
Member

We'll sure, you could accumulate the sizes of all allocs minus all deallocs somewhere and compare.
I did something similar for debugging purposes, hacked into the allocator once using an AtomicUsize for #899.
You could also start VMs with minimal memory and create/destroy connections a lot and see if we run out of memory.

@cagatay-y
Copy link
Contributor Author

I've tested the branch with siege using 10 concurrent connections for 115 seconds (just under the xtask timeout). It handled 34460 requests in total without error. The QEMU process also seems to have a stable memory usage during the test and returns the memory at the end of the test.

@mkroening
Copy link
Member

Thanks for testing! How much memory did the VM get?

@cagatay-y
Copy link
Contributor Author

I am not on my computer right now, so I cannot check to be sure but if I recall correctly, ~95 MB during the test and ~80 MB at idle. The VM limit was the xtask default (128 MB (?)).

@mkroening mkroening self-requested a review December 6, 2023 13:40
@mkroening mkroening self-assigned this Dec 6, 2023
Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! :)

Sorry for taking so long with the review.

Just a few remarks regarding the introduction of additional unsafe blocks.

src/drivers/virtio/virtqueue/mod.rs Outdated Show resolved Hide resolved
src/drivers/virtio/virtqueue/packed.rs Outdated Show resolved Hide resolved
src/drivers/virtio/virtqueue/packed.rs Outdated Show resolved Hide resolved
src/drivers/virtio/virtqueue/split.rs Outdated Show resolved Hide resolved
@cagatay-y cagatay-y marked this pull request as draft January 2, 2024 00:29
@cagatay-y cagatay-y force-pushed the pinned-removal branch 2 times, most recently from 8c842a3 to 293703f Compare January 2, 2024 12:27
@cagatay-y
Copy link
Contributor Author

Looking closer at the uses of unsafe, I think they are fundamentally not thread-safe. It is possible to use synchronization to make it safe, but I realized that the only uses of shared Transfers are inside the module itself to check for Transfer status and they can be avoided. The latest commit does that.

If it is preferable to keep the possibility to check status through the Transfer for future use, we can also switch to using synchronization. I kept the initial commit (the one with reference counting) to make it more convenient to review the changes to the original PR and to switch back to the Rc approach if necessary. The two commits can be squashed if the approach described above is acceptable.

@cagatay-y cagatay-y marked this pull request as ready for review January 2, 2024 12:38
@mkroening mkroening self-requested a review January 25, 2024 11:10
Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

Thanks a lot! This looks great.

Just one question about MaybeUninit inline.

Another question: Is the PR description still up to date?

You mention one-bit refcounts. What do you mean with that? I mean a refcounter that can only count to one is exactly the same as just having a single owner.

The description also talks about replacing Pinned<T> with Rc<T>, but to me, the code reads more like replacing it with Box<T> and T, which is good, I was just wondering about the description. :)

src/drivers/virtio/virtqueue/packed.rs Outdated Show resolved Hide resolved
It seems like Pinned was more of a single bit reference counter than an
actual Pin. This commit replaces it with Rc.
Sharing the transfer with the sender is not really necessary, they
can be notified through a await queue. Change the relevant methods to
let the virtqueue own the Transfers and avoid reference counting
complications.
@cagatay-y
Copy link
Contributor Author

I've updated the PR description and implemented the change from MaybeUninit to Option.

@mkroening mkroening self-requested a review February 3, 2024 21:13
Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

Thanks a lot! :)

@mkroening mkroening added this pull request to the merge queue Feb 5, 2024
Merged via the queue into hermit-os:main with commit 774c757 Feb 5, 2024
39 checks passed
@cagatay-y cagatay-y deleted the pinned-removal branch April 4, 2024 10:29
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.

None yet

2 participants