Fix WaitChannel's API #349

travis1829 · 2021-01-20T08:39:20Z

#241 작업을 하기 전에 먼저 WaitChannel의 지저분한 부분을 조금 다듬어봤습니다.

기존에는 WaitChannel에 대해서 sleep을 하기 위해서는 SpinlockGuard는 sleep()로, SleepablelockGuard는 sleep_sleepable()로, 그리고 SpinlockProtected는 sleep_raw()로 여러가지로 API가 나눠져있는 걸 하나로 합쳤습니다.
- 이를 위해, Guard라는 trait을 추가했습니다. (현재 SpinlockGuard, SpinlockProtectedGuard, SleepablelockGuard이 이걸 impl합니다.)
다만, 별개로, 현재 SleepablelockGuard::sleep()이 unsafe이 아닌데, 이래도 괜찮은지 잘 모르겠습니다.
- 먼저, WaitChannel::sleep()이 unsafe한 이유는 guard.sched()을 call할때는 항상 p->lock이외에는 어떠한 RawSpinlock도 acquire한 상태여선 안되기 때문입니다. (guard.sched()내에서도 assert_eq!((*kernel().mycpu()).noff, 1, "sched locks");로 이걸 확인합니다). 그러나 SleepablelockGuard::sleep()은 내부에서 WaitChannel::sleep()을 사용하는데도 unsafe이 아닙니다.

jeehoonkang · 2021-01-20T08:43:06Z

kernel-rs/src/proc.rs

-        let p: *mut Proc = myproc();
+    /// # Safety
+    ///
+    /// Make sure `lk` is the only lock we currently hold.


이게 아니면 어떻게 되나요?

저도 아직 확실하지는 않지만, 일단은 xv6에서 사용하던 invariant인것 같습니다.
일단, sleep()내부에서 sched()을 부르게 되는데, sched()함수에 있는 주석에는

/// Switch to scheduler. Must hold only p->lock /// and have changed proc->state. Saves and restores /// interrupt_enabled because interrupt_enabled is a property of this /// kernel thread, not this CPU. It should /// be proc->interrupt_enabled and proc->noff, but that would /// break in the few places where a lock is held but /// there's no process.

이라고 나와있어서 이걸 반영한겁니다.

다만, 이건 좀 더 알아보겠습니다!

EDIT: 일단 xv6책을 보면 의도적으로 이런 invariant을 넣어놓은 것 같긴 한데, 이유는 안 나와있는 것 같네요. 제 생각에는 deadlock을 좀 더 쉽게 방지하기 위한 장치 중 하나인 것 같습니다.

Now that sleep holds p->lock and no others, it can put the process to sleep by recording the
sleep channel, changing the process state to SLEEPING, and calling sched (kernel/proc.c:564-567).

이 조건을 만족하지 않고 sleep 을 호출하면 memory safety 에 영향을 주나요? 말씀하신 내용대로면 deadlock 이 문제인데 이것은 safety 와 상관없는 것 같습니다.

이 조건을 만족하지 않고 sleep 을 호출하면 memory safety 에 영향을 주나요? 말씀하신 내용대로면 deadlock 이 문제인데 이것은 safety 와 상관없는 것 같습니다.

sched()와 달리 scheduler()에서는 저런 조건이 없는 걸 보면 딱히 그런건 아닌 것 같습니다. 그러면 unsafe fn sleep() -> fn sleep()으로 바꿀까요? (참고로, sleep()을 call하기 전에는 항상 Cpu->proc이 non-null이여야 한다는 추가적인 safety 조건이 있기는 한데, 이거는 #258 와 관련해서 나중에 해결해야될 문제 같습니다.)

네 말씀하신 대로 Proc refactoring 을 전제하면 이 함수는 safe하다고 말할 수 있어 보입니다. sleep 은 safe function 으로 바꾸어 주시고, 그 때문에 생기는 unsafe block 에 safety argument 를 추가해 주세요.

kernel-rs/src/proc.rs

kernel-rs/src/sleepablelock.rs

travis1829 · 2021-01-20T12:11:55Z

Guard trait을 WaitableGuard로 바꾸고 proc.rs로 옮겼습니다.
sleep(&self, lk: &mut dyn Guard) -> sleep<T: WaitableGuard>(&self, lk: &mut T)로 바꿨습니다.

kernel-rs/src/proc.rs

efenniht · 2021-01-20T13:53:14Z

kernel-rs/src/proc.rs

-        let p: *mut Proc = myproc();
+    /// # Safety
+    ///
+    /// Make sure `lk` is the only lock we currently hold.


이 조건을 만족하지 않고 sleep 을 호출하면 memory safety 에 영향을 주나요? 말씀하신 내용대로면 deadlock 이 문제인데 이것은 safety 와 상관없는 것 같습니다.

kernel-rs/src/spinlock.rs

jeehoonkang · 2021-01-21T00:03:25Z

이 PR은 @efenniht 님이 리뷰해주시고 r+ 해주시면 감사하겠습니다.

efenniht

전반적으로 safety argument 에 관련된 comment를 추가해 주시길 부탁드립니다. 🙏

efenniht · 2021-01-21T04:26:36Z

kernel-rs/src/proc.rs

-        let p: *mut Proc = myproc();
+    /// # Safety
+    ///
+    /// Make sure `lk` is the only lock we currently hold.


네 말씀하신 대로 Proc refactoring 을 전제하면 이 함수는 safe하다고 말할 수 있어 보입니다. sleep 은 safe function 으로 바꾸어 주시고, 그 때문에 생기는 unsafe block 에 safety argument 를 추가해 주세요.

efenniht · 2021-01-21T04:31:57Z

kernel-rs/src/proc.rs

+    /// You should manually prove the correctness when directly accessing
+    /// the inner `RawSpinlock` instead of using the lock's API.


실제로 어떤 걸 증명해야 하는 지 설명해주셨으면 합니다:

get_raw 로 얻은 RawSpinlock 을 release 하면, 다시 acquire 하기 전 까지는 self에 접근하면 안 됨.

그리고 나서 이 함수를 사용하는 지점에서 해당 조건을 만족함을 주석으로 설명해 주세요.

travis1829 · 2021-01-21T05:50:41Z

&RawSpinlock 자체가 노출되는 건 너무 불필요하게 unsafe한것 같아서, 실제 필요한 operation인 RawSpinlock::acquire()와 RawSpinlock::release()만 사용할 수 있도록 바꿨습니다.
- Waitable trait에 대한 unsafe document도 추가했습니다.
- 참고로, 이제 proc.rs에서 RawSpinlock을 직접 사용하는 곳은 wait_lock을 initialize할때 뿐입니다. (이 부분까지 없어지면, use crate::spinlock::RawSpinlock을 아예 없앨 수 있긴 합니다.)
WaitChannel::sleep()은 이제 unsafe이 아닙니다.

efenniht · 2021-01-21T05:58:29Z

bors r+

349: Fix WaitChannel's API r=efenniht a=travis1829 #241 작업을 하기 전에 먼저 `WaitChannel`의 지저분한 부분을 조금 다듬어봤습니다. * 기존에는 `WaitChannel`에 대해서 sleep을 하기 위해서는 `SpinlockGuard`는 `sleep()`로, `SleepablelockGuard`는 `sleep_sleepable()`로, 그리고 `SpinlockProtected`는 `sleep_raw()`로 **여러가지로 API가 나눠져있는 걸 하나로 합쳤습니다.** * 이를 위해, `Guard`라는 trait을 추가했습니다. (현재 `SpinlockGuard`, `SpinlockProtectedGuard`, `SleepablelockGuard`이 이걸 impl합니다.) * 다만, 별개로, **현재 `SleepablelockGuard::sleep()`이 `unsafe`이 아닌데, 이래도 괜찮은지 잘 모르겠습니다.** * 먼저, `WaitChannel::sleep()`이 `unsafe`한 이유는 `guard.sched()`을 call할때는 항상 `p->lock`이외에는 어떠한 RawSpinlock도 acquire한 상태여선 안되기 때문입니다. (`guard.sched()`내에서도 `assert_eq!((*kernel().mycpu()).noff, 1, "sched locks");`로 이걸 확인합니다). 그러나 `SleepablelockGuard::sleep()`은 내부에서 `WaitChannel::sleep()`을 사용하는데도 unsafe이 아닙니다. Co-authored-by: travis1829 <travis1829@naver.com>

ghost · 2021-01-21T06:18:49Z

Build failed:

ci

efenniht · 2021-01-21T06:25:28Z

bors r+

ghost · 2021-01-21T06:31:18Z

Build succeeded:

ci

jeehoonkang requested changes Jan 20, 2021

View reviewed changes

kernel-rs/src/proc.rs Outdated Show resolved Hide resolved

efenniht requested changes Jan 20, 2021

View reviewed changes

travis1829 force-pushed the WaitChannel-fix branch from 3b9bcc8 to ddecffe Compare January 20, 2021 16:00

efenniht requested changes Jan 21, 2021

View reviewed changes

travis1829 requested a review from efenniht January 21, 2021 05:56

travis1829 added 5 commits January 21, 2021 06:19

Fix WaitChannel's API

f0c83e4

Change Guard trait -> WaitableGuard trait

b6db529

Mark get_raw as unsafe.

96b5483

Remove Waitable::get_raw() and add raw_release(), raw_acquire().

488155e

Change to mutable reference.

dcfd735

travis1829 force-pushed the WaitChannel-fix branch from a46a91c to dcfd735 Compare January 21, 2021 06:20

ghost merged commit ef1d8fe into kaist-cp:riscv Jan 21, 2021

travis1829 deleted the WaitChannel-fix branch January 21, 2021 06:38

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix WaitChannel's API #349

Fix WaitChannel's API #349

travis1829 commented Jan 20, 2021

jeehoonkang Jan 20, 2021

travis1829 Jan 20, 2021 •

edited

efenniht Jan 20, 2021

travis1829 Jan 20, 2021

efenniht Jan 21, 2021

travis1829 commented Jan 20, 2021

efenniht Jan 20, 2021

jeehoonkang commented Jan 21, 2021

efenniht left a comment

efenniht Jan 21, 2021

efenniht Jan 21, 2021 •

edited

travis1829 commented Jan 21, 2021

efenniht commented Jan 21, 2021

ghost commented Jan 21, 2021

efenniht commented Jan 21, 2021

ghost commented Jan 21, 2021

		/// You should manually prove the correctness when directly accessing
		/// the inner `RawSpinlock` instead of using the lock's API.

Fix WaitChannel's API #349

Fix WaitChannel's API #349

Conversation

travis1829 commented Jan 20, 2021

jeehoonkang Jan 20, 2021

Choose a reason for hiding this comment

travis1829 Jan 20, 2021 • edited

Choose a reason for hiding this comment

efenniht Jan 20, 2021

Choose a reason for hiding this comment

travis1829 Jan 20, 2021

Choose a reason for hiding this comment

efenniht Jan 21, 2021

Choose a reason for hiding this comment

travis1829 commented Jan 20, 2021

efenniht Jan 20, 2021

Choose a reason for hiding this comment

jeehoonkang commented Jan 21, 2021

efenniht left a comment

Choose a reason for hiding this comment

efenniht Jan 21, 2021

Choose a reason for hiding this comment

efenniht Jan 21, 2021 • edited

Choose a reason for hiding this comment

travis1829 commented Jan 21, 2021

efenniht commented Jan 21, 2021

ghost commented Jan 21, 2021

efenniht commented Jan 21, 2021

ghost commented Jan 21, 2021

travis1829 Jan 20, 2021 •

edited

efenniht Jan 21, 2021 •

edited