[Closes #346, #363] Atomicpid, proc id type, state in procguard #401

anemoneflower · 2021-02-05T07:12:47Z

closes Process ID type 만들기 #346 , closes pid, state should be methods of ProcGuard #363

주요 변경점입니다.

prod id type인 Pid를 만들었습니다.
ProcInfo에 있던 pid를 AtomicI32로 바꾸고 Proc으로 옮겼습니다.
Proc에서 unsafe하게 정의되어 있던 state()함수를 ProcGuard로 옮겼습니다.
- ProcGuard없이 state에 접근하지 않습니다.

anemoneflower · 2021-02-05T07:14:07Z

kernel-rs/src/proc.rs

@@ -205,6 +205,8 @@ pub enum Procstate {
    USED,
 }

+pub type Pid = i32;


PID가 더 좋을까요?

kernel-rs/src/proc.rs

coolofficials

rv6/kernel-rs/src/proc.rs

Line 658 in 9d0b25a

fn allocpid(&self) -> i32 {

의 return type도 Pid가 되어야 할 것 같습니다.

rv6/kernel-rs/src/proc.rs

Line 671 in 9d0b25a

guard.deref_mut_info().pid = self.allocpid();

처럼 procinfo의 pid를 업데이트 해주기 때문에...

coolofficials

LGTM

kernel-rs/src/proc.rs

Medowhill · 2021-02-06T16:24:27Z

kernel-rs/src/trap.rs

@@ -184,7 +184,7 @@ pub unsafe fn kerneltrap() {
    // Give up the CPU if this is a timer interrupt.
    if which_dev == 2 {
        if let Some(proc) = kernel().current_proc() {
-            if unsafe { proc.state() } == Procstate::RUNNING {
+            if proc.lock().state() == Procstate::RUNNING {


여기서 state에 접근할 때 lock을 걸지 않던 것은 xv6의 버그일까요? 아니면 lock을 걸 필요가 없는 이유가 있는 것일까요?

procdump와 kerneltrap을 제외하고는 다 lock을 걸고 state에 접근하는데, state를 읽고, proc의 값을 수정하는 작업이 lock을 걸고 있는 동안 진행됩니다.

kerneltrap()의 경우에는 yield에서 lock을 걸기 때문에 state를 읽을 때는 lock없이 읽은 것 같습니다. RUNNING상태를 바꾸는 곳이 yield이고, yield에서 sched가 실행되면 context switch가 일어나기 때문에 lock을 걸지 않은 것이 아닐까요?

다시 생각해보니

기존 코드처럼 unsafe하게 state를 읽거나

proc_yield를 ProcGuard의 함수로 만드는 것이 좋을 것 같습니다.

@Medowhill

제 생각에는 1을 선택하고 이 unsafe가 안전한 이유를 주석으로 설명하는 것이 좋은 것 같습니다. proc_yield는 현재 실행 중인 프로세스만 할 수 있는 일이니 지금처럼 CurrentProc의 메서드로 두는 것이 더 나아 보여서요.

anemoneflower

리뷰 반영했습니다 :)

pid를 ProcInfo에 그대로 유지
dump를 unsafe하게 수정

kernel-rs/src/proc.rs

anemoneflower · 2021-02-08T03:31:51Z

kernel-rs/src/trap.rs

@@ -184,7 +184,7 @@ pub unsafe fn kerneltrap() {
    // Give up the CPU if this is a timer interrupt.
    if which_dev == 2 {
        if let Some(proc) = kernel().current_proc() {
-            if unsafe { proc.state() } == Procstate::RUNNING {
+            if proc.lock().state() == Procstate::RUNNING {


procdump와 kerneltrap을 제외하고는 다 lock을 걸고 state에 접근하는데, state를 읽고, proc의 값을 수정하는 작업이 lock을 걸고 있는 동안 진행됩니다.

kerneltrap()의 경우에는 yield에서 lock을 걸기 때문에 state를 읽을 때는 lock없이 읽은 것 같습니다. RUNNING상태를 바꾸는 곳이 yield이고, yield에서 sched가 실행되면 context switch가 일어나기 때문에 lock을 걸지 않은 것이 아닐까요?

다시 생각해보니

기존 코드처럼 unsafe하게 state를 읽거나

proc_yield를 ProcGuard의 함수로 만드는 것이 좋을 것 같습니다.

@Medowhill

Medowhill · 2021-02-08T13:35:01Z

kernel-rs/src/trap.rs

@@ -184,7 +184,7 @@ pub unsafe fn kerneltrap() {
    // Give up the CPU if this is a timer interrupt.
    if which_dev == 2 {
        if let Some(proc) = kernel().current_proc() {
-            if unsafe { proc.state() } == Procstate::RUNNING {
+            if proc.lock().state() == Procstate::RUNNING {


제 생각에는 1을 선택하고 이 unsafe가 안전한 이유를 주석으로 설명하는 것이 좋은 것 같습니다. proc_yield는 현재 실행 중인 프로세스만 할 수 있는 일이니 지금처럼 CurrentProc의 메서드로 두는 것이 더 나아 보여서요.

anemoneflower

코멘트 반영했습니다. :)

kerneltrap() 에서 ProcInfo와 state에 접근해야 하기 때문에 관련된 field를 public으로 변경했습니다.

Medowhill · 2021-02-09T03:34:04Z

bors r+

ghost · 2021-02-09T03:40:00Z

Build succeeded:

ci

anemoneflower commented Feb 5, 2021

View reviewed changes

anemoneflower requested review from jeehoonkang and Medowhill February 5, 2021 07:14

coolofficials suggested changes Feb 5, 2021

View reviewed changes

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

anemoneflower requested a review from coolofficials February 5, 2021 07:20

coolofficials reviewed Feb 5, 2021

View reviewed changes

coolofficials self-requested a review February 5, 2021 07:30

coolofficials approved these changes Feb 5, 2021

View reviewed changes

Medowhill reviewed Feb 5, 2021

View reviewed changes

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

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

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

coolofficials suggested changes Feb 5, 2021

View reviewed changes

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

Medowhill reviewed Feb 6, 2021

View reviewed changes

anemoneflower commented Feb 8, 2021

View reviewed changes

anemoneflower requested a review from Medowhill February 8, 2021 03:42

Medowhill requested changes Feb 8, 2021

View reviewed changes

anemoneflower added 8 commits February 9, 2021 02:43

move pid to Proc and move state() to ProcGuard

322e5e2

new type Pid

7138cff

Pid to private

b082c9a

allopid() return type Pid

b1ac017

revert dump to access state directly

0a70443

revert pid to exist in ProcInfo with type Pid

21ab1a9

get procstate in kerneltrap without lock

3637364

leave comment about unsafe state in kerneltrap

fc0685e

anemoneflower force-pushed the atomicpid branch from d6e9b25 to fc0685e Compare February 9, 2021 02:44

anemoneflower requested a review from Medowhill February 9, 2021 02:44

anemoneflower commented Feb 9, 2021

View reviewed changes

Medowhill approved these changes Feb 9, 2021

View reviewed changes

ghost merged commit 1c9ed84 into kaist-cp:riscv Feb 9, 2021

Medowhill mentioned this pull request May 1, 2021

Reasoning safety of self.get_ctx() in KernelRef::kernel_trap #517

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Closes #346, #363] Atomicpid, proc id type, state in procguard #401

[Closes #346, #363] Atomicpid, proc id type, state in procguard #401

anemoneflower commented Feb 5, 2021

anemoneflower Feb 5, 2021

coolofficials left a comment •

edited

Loading

coolofficials left a comment

Medowhill Feb 6, 2021

anemoneflower Feb 8, 2021

Medowhill Feb 8, 2021

anemoneflower left a comment

anemoneflower Feb 8, 2021

Medowhill Feb 8, 2021

anemoneflower left a comment

Medowhill commented Feb 9, 2021

ghost commented Feb 9, 2021

[Closes #346, #363] Atomicpid, proc id type, state in procguard #401

[Closes #346, #363] Atomicpid, proc id type, state in procguard #401

Conversation

anemoneflower commented Feb 5, 2021

anemoneflower Feb 5, 2021

Choose a reason for hiding this comment

coolofficials left a comment • edited Loading

Choose a reason for hiding this comment

coolofficials left a comment

Choose a reason for hiding this comment

Medowhill Feb 6, 2021

Choose a reason for hiding this comment

anemoneflower Feb 8, 2021

Choose a reason for hiding this comment

Medowhill Feb 8, 2021

Choose a reason for hiding this comment

anemoneflower left a comment

Choose a reason for hiding this comment

anemoneflower Feb 8, 2021

Choose a reason for hiding this comment

Medowhill Feb 8, 2021

Choose a reason for hiding this comment

anemoneflower left a comment

Choose a reason for hiding this comment

Medowhill commented Feb 9, 2021

ghost commented Feb 9, 2021

coolofficials left a comment •

edited

Loading