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

[Closes #596] Fix List (Remove unsoundness, interior mutability) #598

Merged
merged 2 commits into from
Feb 16, 2022

Conversation

travis1829
Copy link
Collaborator

@travis1829 travis1829 commented Feb 14, 2022

Closes #596

  • Changed fn iter_shared_mut -> unsafe fn iter_strong_pin_mut_unchecked
  • Removed interior mutability in ListEntry

ListRefCell이나 tock의 list등과 달리, interior mutability를 꼭 사용해야하는 이유가 없는 것 같아 이를 제거했습니다.

@travis1829
Copy link
Collaborator Author

참고로, 이 PR이 끝나면, List 관련해서 다음 작업을 할 예정입니다.

@travis1829 travis1829 changed the title [Closes #596] Change fn iter_shared_mut into an unsafe fn. [Closes #596] Fix List (Remove unsoundness, interior mutability) Feb 15, 2022
@travis1829 travis1829 marked this pull request as ready for review February 15, 2022 14:24
@Medowhill
Copy link
Collaborator

#451 에서 ListEntry*mut Self 대신 Cell<*const Self>를 갖도록 수정했는데, 이번 PR에서는 다시 Cell<*const Self> 대신 *mut Self를 사용하도록 바꾼 것으로 보입니다. 오랜만에 보다 보니 제가 기억이 잘 나지 않는데, 혹시 예전에 Cell이 필요했던 이유와 다시 Cell이 필요하지 않게 된 이유를 정리해 주실 수 있나요?

@travis1829
Copy link
Collaborator Author

travis1829 commented Feb 16, 2022

결론만 말씀드리자면, Cell을 추가했던 건 예전에 제가 Cell에 대한 이해가 부족해서 그랬던 것 같습니다.

  • 처음에 Cell을 추가한 건 tock의 list와 비슷한 list를 만들려고 했던 흔적입니다. 이때는 단순히 Cell을 사용하면 & type만을 가지고도 ListEntry를 mutate할 수 있어 Pin때문에 API가 복잡해지는걸 피할 수 있다는 점만 주목했고, Cell이 일종의 last resort같은 개념이라는 건 잘 몰랐습니다.
  • intrusive linked list의 unsound한 부분 해결하기 #450Cell을 사용하지 않더라도 해결할 수 있고, Cell을 사용하더라도 unsafe가 줄어들지는 않으며, 오히려 사용자가 Pin<&mut T>을 사용하도록 하는 게 Rust에도 더 맞고 더 안전한 것 같아 변경했습니다.
  • 다만, Cell을 없애게 되면서 compiler optimization때문에 UB가 일어나는 곳이 있을지도 모르겠습니다. 좀 더 확인해보겠습니다.
  • 혹시 생각이 다르시다면 말씀해주세요. 0aefd55만 revert하면 됩니다.

@travis1829
Copy link
Collaborator Author

travis1829 commented Feb 16, 2022

추가적으로, 아직 고민 중입니다만, 나중에 rv6가 intrusive_list.rs의 List 대신 #495List를 사용하도록 전부 변경할 생각이 있습니다. 현재 rv6에서 list를 사용하는 곳 (MruArena, Runs)를 보면, 항상 Pin<&mut List>를 갖고 있는 (또는 가질 수 있는) 상태에서만 List/ListEntry를 mutate하는 것 같습니다.

@Medowhill
Copy link
Collaborator

Medowhill commented Feb 16, 2022

답변 고맙습니다. 제가 이해한 바로는 이 PR에서 interior mutability를 제거하면서 리스트 API 사용자 입장에서는 "원래 Pin<&Self>가 필요하던 곳에 Pin<&mut Self>가 필요하게 된 것"이니 전보다 사용할 때 더 제약이 많은 intrusive list가 되었지만, rv6에서는 intrusive list를 제한적인 용도로만 사용하기에 이렇게 제약 사항이 늘어난 것이 문제가 되지 않는 것 같은데, 이게 맞나요?

@travis1829
Copy link
Collaborator Author

travis1829 commented Feb 16, 2022

답변 고맙습니다. 제가 이해한 바로는 이 PR에서 interior mutability를 제거하면서 리스트 API 사용자 입장에서는 "원래 Pin<&Self>가 필요하던 곳에 Pin<&mut Self>가 필요하게 된 것"이니 전보다 사용할 때 더 제약이 많은 intrusive list가 되었지만, rv6에서는 intrusive list를 제한적인 용도로만 사용하기에 이렇게 제약 사항이 늘어난 것이 문제가 되지 않는 것 같은데, 이게 맞나요?

예, 이해하신 바가 맞습니다.
그리고, 일반적인 list들 (예1, 예2)도 이런식인 것 같습니다.

@jeehoonkang
Copy link
Member

bors r+

@kaist-cp-bors
Copy link
Contributor

kaist-cp-bors bot commented Feb 16, 2022

Build succeeded:

@kaist-cp-bors kaist-cp-bors bot merged commit 50daee6 into kaist-cp:main Feb 16, 2022
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.

unsoundness in intrusive_list.rs
3 participants