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

Don't lock the allocator with spin. #26

Merged
merged 4 commits into from
Feb 19, 2020
Merged

Conversation

gendx
Copy link
Collaborator

@gendx gendx commented Feb 7, 2020

This change uses the heap of linked_list_allocator directly instead of locking it via a mutex. Since the app is single threaded this shouldn't be a problem.

This also allows to remove the spin dependency of linked_list_allocator, which isn't maintained anymore since RUSTSEC-2019-0031, thereby addressing one half of #24.

After this change, cargo audit should only mention spin via the ring dev-dependency.

  • Tests pass
  • Appropriate changes to README are included in PR

@ia0
Copy link
Member

ia0 commented Feb 7, 2020

I don't think #24 is an issue that should trigger code changes on our side. The fact that a crate is not maintained since a few months is not a short-term issue.

Do we believe adding one more patch to maintain is worth it?

@gendx
Copy link
Collaborator Author

gendx commented Feb 17, 2020

Although there's no urgency, I think keeping a clean cargo audit is worth it in the long term for better code hygiene.

As for the additional maintenance, it's indeed an extra patch, but not that many more lines of code and also following what was done upstream in tock/libtock-rs#107. If it ever turns out to be an issue, we can also coordinate that with upstream libtock-rs.

Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

I agree having a clean cargo audit is good, but it's just a tool and its output has to be interpreted. In that case, I think the output can be dismissed.

However since this patch reduces the diff with upstream, it might make the merging easier if that merging doesn't happen too far in the future.

@gendx
Copy link
Collaborator Author

gendx commented Feb 17, 2020

I agree having a clean cargo audit is good, but it's just a tool and its output has to be interpreted. In that case, I think the output can be dismissed.

Since we chose to integrate this tool in our CI, I don't think it makes sense to dismiss the output when we can reasonably fix it.

@gendx gendx merged commit 51f2016 into google:master Feb 19, 2020
@gendx gendx deleted the no-spin-allocator branch February 19, 2020 09:33
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