[closes #354, #258] myproc()의 사용을 줄이고, usertrap()에서 syscall 함수로 proc전달 #385

anemoneflower · 2021-01-29T05:59:52Z

closes myproc() refactoring #354 , closes ProcExecutionGuard #258

주요 변경사항입니다.

myproc() 을 Kernel의 메소드로 이동
struct ExecutingProc 추가: *mut Proc을 감싸는 struct
- kernel().myexproc() 추가: myproc()에서 Proc이 존재함을 확인하고 ExecutingProc을 리턴
- usertrap()에서 sys_* 함수들에 ExecutingProc을 전달
proc이 null이 아니어야 하는 경우에는 myproc()의 사용을 모두 myexproc()으로 수정했습니다.

kernel-rs/src/exec.rs

kernel-rs/src/fs/path.rs

kernel-rs/src/proc.rs

kernel-rs/src/syscall.rs

kernel-rs/src/trap.rs

kernel-rs/src/proc.rs

kernel-rs/src/fs/inode.rs

Medowhill · 2021-02-02T08:32:27Z

kernel-rs/src/proc.rs

+}
+
+impl DerefMut for CurrentProc {
+    fn deref_mut(&mut self) -> &mut Self::Target {


아래는 아까 오프라인 미팅에서 말씀드린 내용입니다.

rv6/kernel-rs/src/proc.rs

Lines 946 to 953 in f06956c

for p in &kernel().procs.process_pool {

let mut guard = p.lock();

if guard.deref_info().state == Procstate::RUNNABLE {

// Switch to chosen process. It is the process's job

// to release its lock and then reacquire it

// before jumping back to us.

guard.deref_mut_info().state = Procstate::RUNNING;

unsafe { (*c).proc = p as *const _ as *mut _ };

Cpu의 proc이 *mut Proc인데, schedule에서 &Proc을 *mut Proc으로 변환하여 proc에 저장하고 있습니다. &T를 *mut T로 변환한 다음, 값을 쓰거나 &mut T로 변환하면 UB이기 때문에 현재 구현에 다소 문제가 있어 보입니다. Cpu의 proc을 *const Proc으로 바꾸고 Proc은 항상 &mut Proc 대신 &Proc으로만 사용되게 해야 할 것 같습니다.

*const Proc으로 사용하기 위해 다음과 같은 변경사항을 적용했습니다.

Cpu.proc, ProcessSystem.initial_proc, Proc.parent 에서 *mut Proc대신 *const Proc사용

CurrentProc에서 *const Proc사용 (NonNull은 *mut T를 받기 때문에 사용하지 않았습니다.)

CurrentProc, Proc의 DerefMut 대신 Proc에 deref_mut_data() 구현

Proc.name을 ProcData의 field로 이동 (exec에서 디버깅을 위해 &mut p.name을 사용하는데, 이때 Proc이 mutable 하게 참조되기 때문입니다. )

rv6/kernel-rs/src/exec.rs

Lines 188 to 193 in 551a6b1

let p_name = &mut p.name;

let len = cmp::min(p_name.len(), name.len());

p_name[..len].copy_from_slice(&name[..len]);

if len < p_name.len() {

p_name[len] = 0;

}

anemoneflower

리뷰 반영했습니다 :)

CurrentProc, Proc에 Deref 구현 추가
CurentProc에 lifetime 추가
myproc() 대신 CurrentProc이 Option리턴
Cpu.proc, ProcessSystem.initial_proc, Proc.parent 에서 *mut Proc대신 *const Proc사용 (관련해서 구체적인 내용은 아래 코멘트에 있습니다.)

kernel-rs/src/fs/path.rs

anemoneflower · 2021-02-02T11:34:02Z

kernel-rs/src/proc.rs

+}
+
+impl DerefMut for CurrentProc {
+    fn deref_mut(&mut self) -> &mut Self::Target {


*const Proc으로 사용하기 위해 다음과 같은 변경사항을 적용했습니다.

Cpu.proc, ProcessSystem.initial_proc, Proc.parent 에서 *mut Proc대신 *const Proc사용

CurrentProc에서 *const Proc사용 (NonNull은 *mut T를 받기 때문에 사용하지 않았습니다.)

CurrentProc, Proc의 DerefMut 대신 Proc에 deref_mut_data() 구현

Proc.name을 ProcData의 field로 이동 (exec에서 디버깅을 위해 &mut p.name을 사용하는데, 이때 Proc이 mutable 하게 참조되기 때문입니다. )

rv6/kernel-rs/src/exec.rs

Lines 188 to 193 in 551a6b1

let p_name = &mut p.name;

let len = cmp::min(p_name.len(), name.len());

p_name[..len].copy_from_slice(&name[..len]);

if len < p_name.len() {

p_name[len] = 0;

}

kernel-rs/src/proc.rs

kernel-rs/src/exec.rs

kernel-rs/src/file.rs

kernel-rs/src/proc.rs

kernel-rs/src/sleeplock.rs

kernel-rs/src/trap.rs

kernel-rs/src/proc.rs

Medowhill · 2021-02-02T15:08:08Z

kernel-rs/src/proc.rs

+    pub fn current_proc(&self) -> Option<CurrentProc<'_>> {
+        unsafe { push_off() };
+        let c = self.mycpu();
+        let p = unsafe { (*c).proc };


CurrentProc의 lifetime에 대한 invariant를 보장할 수 있도록 (*c).proc이 nonnull이면 &self의 lifetime 동안 살아있다는 설명이 추가되어야 할 것 같습니다. 이를 위해서는 Kernel의 invariant로 cpus안에 있는 proc들의 lifetime에 대한 조건이 기술되어야 할 것으로 보입니다.

8cc4aab
반영했습니다. 제가 맞게 이해한 것인지 궁금합니다. @Medowhill

anemoneflower

코멘트 반영을 완료했습니다. (commit이 많은 것 같아서 코멘트에 해당하는 commit을 기록했습니다.)

주요 변경점입니다.

syscall관련 함수들은 모두 CurrentProc을 input으로 받아서 사용합니다.
CurrentProc과 관련된 safety invarient를 추가했습니다.
&mut Proc대신 &Proc을 사용하도록 수정했습니다.
추가적으로 Proc과 ProcGuard에서 Deref를 사용할 수 있는 일부 코드를 수정했습니다.

kernel-rs/src/exec.rs

kernel-rs/src/proc.rs

anemoneflower · 2021-02-03T11:03:54Z

kernel-rs/src/proc.rs

+    pub fn current_proc(&self) -> Option<CurrentProc<'_>> {
+        unsafe { push_off() };
+        let c = self.mycpu();
+        let p = unsafe { (*c).proc };


8cc4aab
반영했습니다. 제가 맞게 이해한 것인지 궁금합니다. @Medowhill

kernel-rs/src/proc.rs

kernel-rs/src/sleeplock.rs

kernel-rs/src/proc.rs

jeehoonkang

굉장히 잘 해주셨습니다. 감사합니다.
마이너한 코멘트 몇개 더 남깁니다.

kernel-rs/src/kernel.rs

kernel-rs/src/proc.rs

jeehoonkang · 2021-02-03T11:52:44Z

kernel-rs/src/proc.rs

@@ -453,12 +499,6 @@ impl Deref for ProcGuard {
    }
 }

-impl DerefMut for ProcGuard {


혹시 지워진 까닭은?

DerefMut이 &mut Proc을 리턴하기 때문에 지웠습니다.

관련된 재민님의 코멘트입니다.
#385 (comment)
#385 (comment)

kernel-rs/src/proc.rs

Medowhill · 2021-02-03T11:55:47Z

kernel-rs/src/proc.rs

+/// `inner` is current Cpu's proc, this means it's state is `RUNNING`.
+/// `Proc` lives during lifetime `'p`.
+pub struct CurrentProc<'p> {
+    inner: *const Proc,


위의 invariant가 의미하는 것이 결국 inner가 'p 동안 유효한 Proc을 가리킨다는 것이니, 지금처럼 inner: *const Proc으로 raw pointer를 사용하기보다는 그냥 inner: &'p Proc으로 해도 되지 않을까 싶습니다.

kernel-rs/src/proc.rs

kernel-rs/src/kernel.rs

anemoneflower

리뷰 반영했습니다 :)

주요 변경점입니다.

CurrentProc이 *const Proc대신 &'p Proc을 inner로 가집니다.
코멘트주신 내용을 반영하여 invariant 관련 주석을 수정했습니다.
- CurrentProc이 Proc의 포인터를 가지지 않기 때문에 non-null pointer와 관련된 주석은 삭제했습니다.

kernel-rs/src/proc.rs

anemoneflower · 2021-02-04T05:14:56Z

kernel-rs/src/proc.rs

@@ -453,12 +499,6 @@ impl Deref for ProcGuard {
    }
 }

-impl DerefMut for ProcGuard {


DerefMut이 &mut Proc을 리턴하기 때문에 지웠습니다.

관련된 재민님의 코멘트입니다.
#385 (comment)
#385 (comment)

anemoneflower · 2021-02-04T05:38:42Z

kernel-rs/src/proc.rs

+/// `inner` is current Cpu's proc, this means it's state is `RUNNING`.
+/// `Proc` lives during lifetime `'p`.
+pub struct CurrentProc<'p> {
+    inner: *const Proc,


…ents

Medowhill · 2021-02-04T13:57:36Z

bors retry

Medowhill · 2021-02-04T13:58:18Z

bors r+

ghost · 2021-02-04T14:05:13Z

Build succeeded:

ci

anemoneflower marked this pull request as draft January 29, 2021 06:22

anemoneflower force-pushed the exeproc branch from d959e9f to 10319cf Compare February 1, 2021 08:23

anemoneflower mentioned this pull request Feb 1, 2021

myproc() refactoring #354

Closed

anemoneflower changed the title ~~WIP: Exeproc~~ [closes #354] myproc()의 사용을 줄이고, usertrap()에서 syscall 함수로 proc전달 Feb 1, 2021

anemoneflower requested review from jeehoonkang, travis1829, coolofficials and Medowhill February 1, 2021 15:49

anemoneflower self-assigned this Feb 1, 2021

anemoneflower changed the title ~~[closes #354] myproc()의 사용을 줄이고, usertrap()에서 syscall 함수로 proc전달~~ [closes #354, #258] myproc()의 사용을 줄이고, usertrap()에서 syscall 함수로 proc전달 Feb 1, 2021

anemoneflower marked this pull request as ready for review February 1, 2021 15:56

jeehoonkang requested changes Feb 1, 2021

View reviewed changes

anemoneflower force-pushed the exeproc branch from 60a9ac5 to e477acf Compare February 2, 2021 03:59

Medowhill reviewed Feb 2, 2021

View reviewed changes

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

kernel-rs/src/fs/inode.rs Outdated Show resolved Hide resolved

Medowhill reviewed Feb 2, 2021

View reviewed changes

anemoneflower commented Feb 2, 2021

View reviewed changes

coolofficials suggested changes Feb 2, 2021

View reviewed changes

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

anemoneflower mentioned this pull request Feb 2, 2021

PR #386 CI Failure #393

Closed

anemoneflower requested review from Medowhill, coolofficials and jeehoonkang February 2, 2021 12:47

jeehoonkang requested changes Feb 2, 2021

View reviewed changes

Medowhill reviewed Feb 2, 2021

View reviewed changes

travis1829 mentioned this pull request Feb 3, 2021

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

Merged

anemoneflower commented Feb 3, 2021

View reviewed changes

anemoneflower requested review from Medowhill and jeehoonkang February 3, 2021 11:38

jeehoonkang requested changes Feb 3, 2021

View reviewed changes

Medowhill reviewed Feb 3, 2021

View reviewed changes

anemoneflower commented Feb 4, 2021

View reviewed changes

anemoneflower added 22 commits February 4, 2021 13:42

apply NonNull in CurrentProc

862b212

use *const Proc instead *mut Proc

765d9f6

resolve clippy error

72ae0d4

fix typo and add comment

5bfc654

typo

225d139

Fix comment

7cd4a70

rename data -> proc_data

73ae8fc

rename p_name -> proc_name

c1695c4

rename CurrentProc.ptr -> inner

def09f7

rename myproc, p -> current_proc, proc

d5e2476

proc_yield to CurrentProc, wakeup to ProcGuard, and resolve some comm…

18b1be1

…ents

update safety comment about CurrentProc, sleep gets CurrentProc

d98c829

CurrentProc related functions get CurrentProc as input

81347cd

replace data.get() -> deref_mut_data()

335d8b3

use Deref in ProcGuard

150ec9d

add comments

46e6188

reduce using deref_mut_data()

b9a1b9e

minor comment resolve

39251e0

CurrentProc's inner -> &'p Proc

6969c81

fix some comments

de41b88

resolve explicit lifetime error

c528b03

remove unnecssary lines, rename CurrentProc::from_raw -> new

78e8a8e

anemoneflower force-pushed the exeproc branch from 2a0cd19 to 78e8a8e Compare February 4, 2021 13:45

ghost merged commit 9d0b25a into kaist-cp:riscv Feb 4, 2021

This was referenced Feb 5, 2021

pid, state should be methods of ProcGuard #363

Closed

[Closes #398] use &mut CurrentProc, deref_data from CurrentProc #407

Merged

Medowhill mentioned this pull request Feb 15, 2021

Make argfd not return &'static RcFile #415

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[closes #354, #258] myproc()의 사용을 줄이고, usertrap()에서 syscall 함수로 proc전달 #385

[closes #354, #258] myproc()의 사용을 줄이고, usertrap()에서 syscall 함수로 proc전달 #385

anemoneflower commented Jan 29, 2021 •

edited

Loading

Medowhill Feb 2, 2021

anemoneflower Feb 2, 2021

anemoneflower left a comment

anemoneflower Feb 2, 2021

Medowhill Feb 2, 2021

anemoneflower Feb 3, 2021

anemoneflower left a comment

anemoneflower Feb 3, 2021

jeehoonkang left a comment

jeehoonkang Feb 3, 2021

anemoneflower Feb 4, 2021

Medowhill Feb 3, 2021

anemoneflower Feb 4, 2021

anemoneflower left a comment

anemoneflower Feb 4, 2021

anemoneflower Feb 4, 2021

Medowhill commented Feb 4, 2021

Medowhill commented Feb 4, 2021

ghost commented Feb 4, 2021

	for p in &kernel().procs.process_pool {
	let mut guard = p.lock();
	if guard.deref_info().state == Procstate::RUNNABLE {
	// Switch to chosen process. It is the process's job
	// to release its lock and then reacquire it
	// before jumping back to us.
	guard.deref_mut_info().state = Procstate::RUNNING;
	unsafe { (c).proc = p as const _ as *mut _ };

	let p_name = &mut p.name;
	let len = cmp::min(p_name.len(), name.len());
	p_name[..len].copy_from_slice(&name[..len]);
	if len < p_name.len() {
	p_name[len] = 0;
	}

[closes #354, #258] myproc()의 사용을 줄이고, usertrap()에서 syscall 함수로 proc전달 #385

[closes #354, #258] myproc()의 사용을 줄이고, usertrap()에서 syscall 함수로 proc전달 #385

Conversation

anemoneflower commented Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anemoneflower left a 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

anemoneflower left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeehoonkang left a 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

anemoneflower left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Medowhill commented Feb 4, 2021

Medowhill commented Feb 4, 2021

ghost commented Feb 4, 2021

anemoneflower commented Jan 29, 2021 •

edited

Loading