[Closes #241] Use Pin API for unmovable structs #356

travis1829 · 2021-01-26T11:35:18Z

이 Draft PR에서는 Pin API를 이용하여 특정 struct들이 move되지 못하도록 강제해보려 하였고, 몇몇 부분에서는 효과적이긴 하였으나, 한계 또한 확실한 듯 하여 먼저 draft을 올려본 후 추가적인 제안을 해보고 싶습니다. (1번은 그냥 보충설명이므로, 2, 3번만 읽으셔도 무방합니다.)

1. 한계(+`Pin` API 정리)

Pin관련 issue([Stabilization] Pin APIs rust-lang/rust#55766 (comment)) 에도 나와있듯이, Pin != !Move 입니다. 즉, Pin은 그저 또다른 pointer일 뿐이며, Pin을 이용한다고 해서 특정 struct을 !Move로 표시하는게 아니고 그저 Pin만을 가지고서는 move를 할 수 없는 것 뿐입니다. 예로, 다음과 같은 코드는 compile error가 발생하지 않습니다. (즉, move가 가능합니다.)

fn move_pinned_ref<T>(mut a: T, mut b: T) {
    unsafe {
        let p: Pin<&mut T> = Pin::new_unchecked(&mut a);
        // This should mean the pointee `a` can never move again.
    }
    mem::swap(&mut a, &mut b);
    // The address of `a` changed to `b`'s stack slot, so `a` got moved even
    // though we have previously pinned it! We have violated the pinning API contract.
}

만약 Pin<&mut T>밖에 없다면 (즉, &mut T는 가지고 있지 않고 얻을 수도 없다면), T를 move할 방법이 없으므로 move를 근본적으로 방지할 수 있습니다.
- 다만, 이러기 위해서는 T를 stack이나 다른 struct의 field이 아니라, heap에 저장해야 합니다. 그렇지 않으면 safe code에서도 &mut T를 얻을 수 있기에, 프로그래머가 직접 조심해야 됩니다. (macro를 쓰면 조금 더 편해지긴 하지만, 그래도 마찬가지입니다.) 그리고 Pin된 상태를 계속 유지해야 합니다.
- rv6에서는 struct을 kernel().alloc()으로 page의 주소값을 받아서 거기에 struct을 저장하는 경우에는, move를 근본적으로 방지할 수 있습니다. (즉, heap에 저장하는 경우와 유사한 예입니다.) 다만, 이런 struct는 Pipe하나 뿐입니다.
- rv6의 나머지 struct들은 모두 또다른 더 큰 struct의 field로 저장되기 때문에, 그 (더 큰) struct 내부에서는 자유롭게 &mut T를 얻을 수 있고, 내부 struct를 move할 수 있으며, 조심하지 않으면 그 &mut T를 외부로 내보낼 수도 있습니다.

2. 이 Draft에서의 변경사항

Pin덕분에, 이제는 Pipe을 move할 수 있는 방법이 없습니다. 추가적으로, Pipe의 Copy, Clone trait을 없앴으며, Pipe::Drop trait을 추가했습니다.
WaitChannel을 이용하는 코드들의 경우, &WaitChannel -> Pin<&WaitChannel>로 변경했습니다. 다만, 임시적으로 Pin<&WaitChannel>을 만들고 바로 drop하는 부분들도 많으므로, 큰 의미가 없는 곳도 많습니다.
아직 TODO로 표시된 미완성 부분들이 많습니다.

3. 제안사항

현재 목표는 최대한 xv6과 비슷한 구조를 유지하는 것입니다. 그러므로, 위에서 설명한 한계를 피할 방법은 없어 보입니다.
일단은, T가 move되어서는 안되는 struct라면, &mut T -> Pin<&mut T>로 바꿔서 &mut T가 노출되지 않도록 하고 있습니다.

* 다만, 추가로, `T` 자체는 `Unsafecell`안에 넣고 `Pin<&mut T>`만을 노출시키는 건 어떨지 제안해봅니다.

즉,

struct blah {
	...
	waitchannel: WaitChannel, //should not move!
}

를

struct blah {
	...
	_waitchannel: Unsafecell<WaitChannel>, //should not move!
	waitchannel: Pin<&mut WaitChannel>,
}

로 바꾸는 겁니다. (다만, 이러면 const fn안에서 initialize할 수 없을 것 같습니다.) 이러면, _waitchannel을 직접 접근하거나 move하지만 않으면 되므로 더 안전하고 훨씬 간단할 것 같습니다.)

jeehoonkang · 2021-01-26T12:26:28Z

kernel-rs/src/bio.rs

+
+    /// Returns the `WaitChannel` saying virtio_disk request is done.
+    pub fn get_waitchannel(&self) -> Pin<&WaitChannel> {
+        // TODO: change `&self` -> `self: Pin<&Self>`,


이 todo는 어떻게 해결 가능한가요?

메소드 이름에 vdisk_request 이 들어가는게 나을지도 모르겠습니다. 어떻게 보시나요? 일반적으로 이 채널이 어떤건지 30번줄에 주석이 좀더 있으면 좋겠습니다.

&self -> self: Pin<&Self>로 바꾼 후, virtio_disk.rs에서 Buf을 access할때는 항상 Pin<&...>형태로 access하도록 바꿔야 할텐데, 바꿔야할 부분이 많아서 일단은 TODO로만 표시했습니다.

맞는 것 같습니다!

jeehoonkang · 2021-01-26T12:27:20Z

kernel-rs/src/bio.rs

@@ -41,6 +42,13 @@ impl BufEntry {
            inner: Sleeplock::new("buffer", BufInner::zero()),
        }
    }
+
+    /// Returns the `WaitChannel` saying virtio_disk request is done.
+    pub fn get_waitchannel(&self) -> Pin<&WaitChannel> {


이게 pin을 리턴함으로써 어떤 장점이 생기는지 알 수 있을까요?

&self가 Pin이어야할 필요는 없을까요?

WaitChannel::sleep() 함수의 signature를 sleep(&self, ...) -> sleep(self: Pin<&Self>, ...)로 바꿨기에 어차피 언젠가는 Pin을 만들어야 하기 때문에, getter 함수가 그냥 처음부터 Pin을 리턴하도록 했습니다.

&self가 Pin이어야할 필요는 없을까요?

맞습니다. 이유는 https://github.com/kaist-cp/rv6/pull/356#discussion_r564504104https://github.com/kaist-cp/rv6/pull/356#discussion_r564504104 와 비슷합니다.

jeehoonkang · 2021-01-26T12:27:47Z

kernel-rs/src/kernel.rs

@@ -60,8 +60,6 @@ pub struct Kernel {

    /// Memory for virtio descriptors `&c` for queue 0.
    ///
-    /// This is a global instead of allocated because it must be multiple contiguous pages, which


이 주석은 왜 사라지는지 궁금합니다.

eadf240 에서 이미 pub virtque -> virtque로 바꿨는데 주석은 그대로 놔둔었길래 주석만 없앴습니다. (즉, 이 PR에서의 변경사항과는 사실 관련 없습니다.)

jeehoonkang · 2021-01-26T12:28:24Z

kernel-rs/src/pipe.rs

    write_waitchannel: WaitChannel,
 }

 impl Pipe {
    /// PipeInner::try_read() tries to read as much as possible.
    /// Pipe::read() executes try_read() until all bytes in pipe are read.
    //TODO : `n` should be u32
-    pub unsafe fn read(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {
-        let mut inner = self.inner.lock();
+    pub unsafe fn read(self: Pin<&Self>, addr: UVAddr, n: usize) -> Result<usize, ()> {


&self는 어차피 move가 안되는데 pin으로 감싸야할 이유가 있나요?

사실 safety랑은 상관이 없고, Pipe을 쓸때는 항상 Pin을 한 후에만 사용하라는 뜻입니다. Pin을 한 후에 사용해야 하는 struct는 method들이 항상 self:Pin<&...>형태인 것이 관례인 것 같습니다.

https://rust-lang.github.io/async-book/04_pinning/01_chapter.html

use std::pin::Pin; use std::marker::PhantomPinned; #[derive(Debug)] struct Test { a: String, b: *const String, _marker: PhantomPinned, } impl Test { fn new(txt: &str) -> Self { Test { a: String::from(txt), b: std::ptr::null(), _marker: PhantomPinned, // This makes our type `!Unpin` } } fn init<'a>(self: Pin<&'a mut Self>) { let self_ptr: *const String = &self.a; let this = unsafe { self.get_unchecked_mut() }; this.b = self_ptr; } fn a<'a>(self: Pin<&'a Self>) -> &'a str { &self.get_ref().a } fn b<'a>(self: Pin<&'a Self>) -> &'a String { assert!(!self.b.is_null(), "Test::b called without Test::init being called first"); unsafe { &*(self.b) } } }

jeehoonkang · 2021-01-26T12:29:21Z

kernel-rs/src/proc.rs

@@ -817,7 +819,9 @@ impl ProcessSystem {

            // Wait for a child to exit.
            //DOC: wait-sleep
-            ((*p).info.get_mut_unchecked().child_waitchannel).sleep(&mut parent_guard);
+            // TODO: Obtain a pin with `project_ref()` instead of making a new pin with `new_unchecked()`.


이 todo는 해결하기 어렵나요?

이 부분은 Spinlock이랑 관련이 있는데, 현재 Spinlock::get_mut_unchecked()는 &mut T를 리턴합니다. 그런데, T가 move 되서는 안되는 type인 경우 (ex: 위의 예에서는 ProcInfo) 어떠한 방법으로든 &mut T를 얻을 수 있어선 안됩니다. 그래서, 이 부분은 Proc으로부터 Pin<&mut ProcInfo>를 얻을 수 있는 새로운 method을 추가함으로써 해결해야할 것 같습니다.

다만, 이것 말고도 move되선 안되는 T에 대해서 Spinlock::get_mut_unchecked()를 쓰는 경우가 많은 것 같은데, 차라리 Spinlock의 API를 바꾸는게 나을지 (ex: T가 move되도 괜찮다면 &mut T를 리턴하고 그렇지 않으면 Pin<&mut T>를 리턴하도록) 고민중이어서 TODO로 표시해놨습니다.

362: [Closes #365] Fix TODOs in pipe.rs r=Medowhill a=travis1829 Closes #365 * #356 에서의 변경사항 중 `Pin` API와 관련없는 내용만 따로 빼서 새롭게 PR을 만들었습니다. (주로 `Pipe`에 대한 변경사항입니다.) * `Pipe`의 `Drop` trait을 추가했고 `Copy`, `Clone` trait을 없앴습니다. * 추가로, `Pipe::copy`가 `unsafe`일 필요가 없는 것 같아서 없앴습니다. (혹시 `unsafe`이여야하는 이유가 있다면 말씀해주세요.) * kernel.rs에 있던 주석을 일부 지웠습니다. (#356 (comment)) Co-authored-by: travis1829 <[email protected]>

travis1829 added 4 commits January 25, 2021 13:55

Use Pin API in Pipe, AllocatedPipe.

7139cfb

Add AllocatedPipe::Drop, and remove some unsafe blocks.

dbb21c8

Change WaitChannel to use Pin.

d859011

Add comments.

75fed01

travis1829 mentioned this pull request Jan 26, 2021

WaitChannel design #241

Closed

jeehoonkang requested changes Jan 26, 2021

View reviewed changes

travis1829 mentioned this pull request Jan 27, 2021

[Closes #365] Fix TODOs in pipe.rs #362

Merged

travis1829 closed this Jan 27, 2021

travis1829 deleted the issue-241 branch January 27, 2021 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Closes #241] Use Pin API for unmovable structs #356

[Closes #241] Use Pin API for unmovable structs #356

travis1829 commented Jan 26, 2021

jeehoonkang Jan 26, 2021

travis1829 Jan 26, 2021

jeehoonkang Jan 26, 2021

travis1829 Jan 26, 2021 •

edited

Loading

jeehoonkang Jan 26, 2021

travis1829 Jan 26, 2021

jeehoonkang Jan 26, 2021

travis1829 Jan 26, 2021

jeehoonkang Jan 26, 2021

travis1829 Jan 26, 2021

[Closes #241] Use Pin API for unmovable structs #356

[Closes #241] Use Pin API for unmovable structs #356

Conversation

travis1829 commented Jan 26, 2021

1. 한계(+Pin API 정리)

2. 이 Draft에서의 변경사항

3. 제안사항

* 다만, 추가로, T 자체는 Unsafecell안에 넣고 Pin<&mut T>만을 노출시키는 건 어떨지 제안해봅니다.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travis1829 Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1. 한계(+`Pin` API 정리)

* 다만, 추가로, `T` 자체는 `Unsafecell`안에 넣고 `Pin<&mut T>`만을 노출시키는 건 어떨지 제안해봅니다.

travis1829 Jan 26, 2021 •

edited

Loading