[Sub-issue of #378] Add PinnedKernel, and use Pin in locks, MruArena, and MruEntry

[travis1829]

Partially resolves # 378.

• T: Unpin인 경우에만 (즉, T가 move되어도 상관없는 경우에만) Spinlock, SpinlockGuard와 같은 lock 및 guard들이 get_mut(), get_mut_unchecked(), DerefMut을 impl하도록 바꿨습니다. 그 대신, get_pin()을 추가했습니다.
  • 추가적으로 ,다른 PR에서 lock과 guard들을 더 정리할 계획입니다.
• ListEntry API가 Pin을 사용하게끔 바꿨습니다.
• MruArena, MruEntry에서 mutable reference가 필요할 때는 항상 Pin<&mut ...>을 사용하게끔 바꿨습니다.
  • 편의를 위해 pin_project crate를 사용했습니다. move되선 안되는 field에는 #[pin]을 붙여서 Pin으로만 접근하도록 제한했습니다. 다만 pin_project가 procedural macro를 사용해서인지 dependency를 약 10개나 늘려버립니다. 참고로, procedural macro를 쓰지 않는 pin_project_lite도 있기는 한데 이건 기능이 제한적입니다.

EDIT:

• 특히, Pin의 안전성을 깔끔하게 보장하기 위해서 PinnedKernel이라는 struct 및 static 변수를 추가했습니다. 지금은 BcacheInner만 들어있지만 추후 Pin이 필요한 struct들을 Kernel에서 여기로 옮길 계획입니다.
  • [Sub-issue of #378] Add PinnedKernel, and use Pin in locks, MruArena, and MruEntry #394 (comment)

[travis1829]

rv6/kernel-rs/src/arena.rs

Lines 118 to 124 in 6fd7edc

	/// # Safety
	///
	/// TODO
	pub struct MruPtr<T> {
	ptr: NonNull<MruEntry<T>>,
	_marker: PhantomData<T>,
	}

추가로, @Medowhill 님께서 여기에 TODO를 남기셨는데, "ptr is a valid pointer to MruEntry<T>" 정도로만 추가해도 될까요? 아니면 ArrayPtr처럼 lifetime을 추가하실 계획이시거나 다른 PR에서 작업하실 계획이신가요?

[Medowhill]

MruArena의 안전성을 보장할 수 있는 올바른 invariant를 적어 달라는 의미였습니다.

[Medowhill]

pin_project가 사용된 부분에서는 rust analyzer가 타입을 못 알아내서 다소 불편하네요... 다른 분들도 같은 문제가 있나요?

[travis1829]

pin_project가 사용된 부분에서는 rust analyzer가 타입을 못 알아내서 다소 불편하네요... 다른 분들도 같은 문제가 있나요?

저도 똑같은 문제가 발생합니다. 그래서 일단 pin_project 대신 pin_project_lite를 쓰면 문제가 해결되는지 알아보고 있습니다.

문제가 잘 해결이 안 되는것 같네요...

• 일단 cargo new를 해서 간단하게 새로 만든 package에서 실험을 해보면
  • Rust(rls): pin_project, pin_project_lite 모두 type을 잘 인식
  • rust-analyzer: pin_project, pin_project_lite 모두 type을 인식하지 못함
• 다만, 정작 중요한 rv6에서는 Rust와 rust-analyzer 둘다 type을 인식하지 못하는 것 같군요.

조금 더 실험을 해보겠습니다만, 계속 안되면 편의를 위해 차라리 getter 함수들을 일일이 다 만드는게 나을지 고민해봐야 할 것 같습니다. (다만, 이러면 쓸데없는 unsafe가 많이 생깁니다.)

[travis1829]

• SpinlockProtected, Sleeplock, Sleepablelock도 Pin을 쓰도록 변경했습니다. (지금은 쓰이는 곳은 없습니다.)
• ListEntry에 safety invariant관련된 doc을 조금 추가했습니다.

[jeehoonkang]

이 PR에서 되어야하지 않을까 생각합니다.

[travis1829]

현재 손서윤님께서 하고계신 PR이랑 겹치는 내용이 많을것 같아 보류하였습니다. 특히, PR #385 가 끝나고 나면 Proc의 내부를 접근하는 방법이 조금 바뀔 것 같아서 일단은 TODO로만 표시했습니다.

[travis1829]

• MruArena::get_entry()를 추가했습니다. 배열의 index를 받은 후 Pin<&mut [T]>로부터 Pin<&mut T>를 얻어낼 수 있는 함수입니다.
  • 다만, 디자인이 그렇게 깔끔하지는 않은 것 같은데, 혹시 더 좋은 방법이 있으시면 말씀해주세요. (저도 고민 중입니다. 개인적으로는 PinnedArray라는 새로운 type을 만들고 거기에 index를 이용해서 get_mut()를 하면 i번째 elem의 pinned mut reference를 얻게하면 어떨지도 생각해보고 있습니다.)

Spinlock이 move되면 그 안에 있는 data도 같이 move되지 않나요?

• 이 부분을 명시적으로 나타내기 위해서 unsafe 함수인 Spinlock::new_unchecked()를 추가했습니다.
  • 다만, 사실 new_unchecked()라는 함수를 Spinlock뿐만 아니라 다른 lock들에도 모두 추가해야되는데, 이걸 나타내기 위해 Lock이라는 trait을 추가하면 어떨지 제안해봅니다.

[Medowhill]

Sleeplock과 Sleepablelock도 동일한 설명이 들어가야 하지 않나요?

[travis1829]

#394 (comment)
맞습니다! 위 comment에 제 생각을 적어놨습니다. (두번째 부분)

[Medowhill]

아 그렇네요. 죄송합니다. 저는 Lock trait을 추가하는 것도 좋아 보입니다.

[travis1829]

• ListEntry에 PinnedDrop trait을 추가했습니다. Drop과 매우 비슷한 trait이며, 마찬가지로 ListEntry가 drop될때 fn drop()이 실행됩니다.
  • 다만, 컴파일하는데는 문제가 없지만 어째서인지 rust-analyzer가 계속 pin_project::pinned_drop을 찾을 수 없다고 에러를 띄웁니다. pin_project와 rust-analyzer가 잘 안 맞는 것 같습니다.

[Medowhill]

최종적으로는 *lock 타입들에서 unsafe를 없애고 해당 lock을 사용하는 쪽에서 적절히 pin을 하게 바꾸어야 할 것 같기는 한데, 이 PR에서 거기까지 할 필요는 없고 지금 상태 정도면 중간 과정으로서 괜찮은 것 같습니다.

[travis1829]

최종적으로는 *lock 타입들에서 unsafe를 없애고 해당 lock을 사용하는 쪽에서 적절히 pin을 하게 바꾸어야 할 것 같기는 한데, 이 PR에서 거기까지 할 필요는 없고 지금 상태 정도면 중간 과정으로서 괜찮은 것 같습니다.

이 부분은 TODO로 표시해두면 될 것 같습니다.

[travis1829]

최종적으로는 *lock 타입들에서 unsafe를 없애고 해당 lock을 사용하는 쪽에서 적절히 pin을 하게 바꾸어야 할 것 같기는 한데, 이 PR에서 거기까지 할 필요는 없고 지금 상태 정도면 중간 과정으로서 괜찮은 것 같습니다.

실험적으로, 이걸 지금 미리 해봤습니다. (다만, *lock들의 API들은 아직은 그대로 놔뒀습니다.)

• Bcache: Spinlock<MruArena<...>>를 BcacheInner: MruArena<...>와 Bcache: Spinlock<Pin<&mut MruArena<...>>> 2개로 나누었습니다. Kernel은 이 두개를 모두 저장하지만, Bcache만을 밖으로 노출시키며 BcacheInner는 kernel_main() 이후에는 절대로 다시는 직접 access하지 않습니다.
  • BcacheInner를 다시는 access하면 안 되는 것을 나타내기 위해 UnsafeCell안에 넣었습니다. (즉, interior mutability때문에 넣은 건 아닙니다.)
  • 이 방법이 Pin을 제일 안전하게 사용하는 방법 같습니다. (Kernel이 BcacheInner만 노출시키지 않으면 내부 struct들을 safe하게 move할 방법이 없습니다.) 또, 이러면 *lock들이 내부 data가 Unpin인지를 신경쓸 필요가 없습니다.
  • 그러나, Kernel의 field를 두개로 나누어야되고, Bcache는 BcacheInner를 만든 후에만 initialize할 수 있으므로 const fn안에서 할 수 없습니다. 그래서 MaybeUninit<Bcache>이 필요합니다. 이 부분은 디자인이 조금 별로인 것 같습니다.

@Medowhill 혹시 어떻게 생각하시는지 말씀해주실 수 있으신가요?

EDIT: 다만, Kernel 전체가 Pin되는 상황을 가정하면, 차라리 이전처럼 계속 *lock들의 Pin관련 API를 사용하게끔 하는게 오히려 나을 수도 있을 것 같습니다.
EDIT2: 제 생각에는 위와 같은 방법을 사용하더라도 *lock들의 Pin관련 API를 계속 놔두는게 좋을 것 같습니다. rv6에는 heap의 개념이 없기 때문에, 이렇게 하지 않으면 프로그래머가 실수하기 쉬운 것 같습니다. 추가로, 이렇게 한다면 MruArena는 어차피 Spinlock으로 보호받으므로 위와 같은 방법을 굳이 사용할 필요가 없습니다. (다만, Spinlock으로 보호받지 못하는 struct들은 위와 같은 방법을 사용해야할 것 같습니다.)

[Medowhill]

정리해 보면 2가지 방법이 있는 것이 맞나요?

1. Spinlock<Pin<&mut MruArena>> 사용
   현재 구현입니다. 이 경우 MruArena를 커널에 넣되 처음에 pin한 후에는 직접 접근하지 않습니다. Pin해서 만들어진 Pin<&mut MruArena>를 Spinlock에 넣으면 Spinlock의 구현에는 수정이 필요 없고, lock을 걸면 Pin<&mut MruArena>가 나오니 이를 사용하면 됩니다. 이 방법을 사용하면 2번처럼 커널 전체를 pin할 필요는 없고, 커널에서 pin해야 하는 부분만 별도의 필드로 선언하고 pin한 뒤에 사용하지 않으면 됩니다. 만약 이 방법을 선택한다면 Kernel에서 pin되어야 하는 부분은 따로 빼서 PinnedKernel 같은 것을 만드는 것은 어떨까 하는 생각도 듭니다.

2. Pin<&mut Spinlock<MruArena>> 사용
   커널 전체를 pin합니다. 그러면 커널 안에 있는 Spinlock<T>는 Pin<&mut Spinlock<T>> 형태로 접근 가능합니다. Spinlock의 구현을 수정하여 Pin<&mut Spinlock<T>>의 lock을 걸면 Pin<&mut T>가 나오게 만들고 이를 사용합니다. 이 경우에는 1번처럼 필드를 분리해서 정의할 필요가 없습니다.

2가 좀 더 좋지 않을까 하는데, 2를 해보고 나면 그 쪽이 더 복잡해서 1이 나아 보일 수도 있을 것 같습니다. 둘 다 해 보고 결정하는 것이 좋아 보이지만, 지금 PR에서는 1로 해 두고 이후에 2를 시도해 본 뒤 괜찮으면 merge하고 별로면 그대로 두는 식으로 하는 것이 어떨까요?

[travis1829]

#394 (comment)
제가 위 comment에 EDIT2를 추가하느라 서로 약간 엇갈린 것 같은데, 제 의견은 @Medowhill 님과 비슷합니다.

• 1. Spinlock<Pin<&mut MruArena>> 사용은 caller입장에서 신경써야되는 부분이 너무 많습니다. 지금은 Bcache에서만 이 방법을 쓰긴 하지만, 다른 곳에서도 똑같은 이런식으로 T는 안전하게 어딘가에 저장한 후 Pin<&mut T>만을 노출시키는 방법을 계속 사용해야됩니다.
• 적어도, Bcache에 대해서는 굳이 이렇게 할 필요가 없고, 오히려 약간 비직관적인 것 같습니다.
• 다만, 이거는 Spinlock의 API 덕분에 이렇게 해도 되는 것 뿐이므로, T가 *lock으로 보호받지 못하는 type들에 대해서는 결국 [Sub-issue of #378] Add PinnedKernel, and use Pin in locks, MruArena, and MruEntry #394 (comment) 에서 한 것과 비슷한 방법 (또는 PinnedKernel을 만드는 방식 등)을 써야할 것 같습니다.

[travis1829]

Spinlock의 구현을 수정하여 Pin<&mut Spinlock>의 lock을 걸면 Pin<&mut T>가 나오게 만들고 이를 사용합니다.

조금 첨언을 하자면, 어차피 Pin<&mut Spinlock<T>>으로부터 &Spinlock<T>를 얻는 것은 legal하므로, 결국에는 T: Unpin인지 아닌지를 구분하는 방법으로 해야 합니다.

[travis1829]

• 말씀하신대로 PinnedKernel을 추가해봤습니다.
• 제 생각에는 @Medowhill 께서 제안하신 PinnedKernel이 매우 좋은 방법 같습니다. Pin<&mut T>을 가장 이상적으로 쓰는 방법이 T 자체는 heap과 같이 외부에서 접근할 방법이 전혀 없는 안전한 곳에 저장하는 것인데, 여기서 PinnedKernel type의 static 변수를 만들어서 heap처럼 쓰면 이를 모방할 수 있습니다.
• 다만, 굳이 단점을 찾자면 이러면 Kernel의 field를 별도의 static한 변수에 저장하는 것이므로, Kernel 내부에서의 spatial locality가 아주 조금 안 좋아질 수도 있을 것 같습니다. PinnedKernel을 Kernel 내부에 저장하는 형태로 바꾸면 이 점을 없앨 수 있겠지만, safety 측면에서는 지금이 더 나은 것 같습니다.

[Medowhill]

수고하셨습니다~

[travis1829]

감사합니다!
참고로 지금 아직 리뷰반영이 안 된 부분에 대해 말씀드리자면,

Lock trait을 추가하는 것도 좋아 보입니다.

• *lock들을 trait로 정리하는 거는 바꿔야 하는 코드 양이 꽤 있는 것 같아 새로운 PR에서 하는게 좋을 것 같고

이 PR에서 되어야하지 않을까 생각합니다.

• Proc을 Pin하는 건 [closes #354, #258] myproc()의 사용을 줄이고, usertrap()에서 syscall 함수로 proc전달 #385 가 끝난 후 하는 게 좋을 것 같습니다.

[jeehoonkang]

• 이번주에 제가 review capacity가 안나올 것 같아서, 일단 한동안은 @Medowhill 님만 승인해주시면 머지하겠습니다.
• lock trait같이 반영 안된건 issue로 만들어주세요 @travis1829

bors r+

[ghost]

Build succeeded:

• ci