[#closes 243] Procdump debug & Refactor console struct. #286

coolofficials · 2020-11-21T18:52:50Z

구현 상 아직 논의가 필요한 부분이 있지만, 리뷰 부탁드립니다.
#276 의 논의가 너무 길어지고 #281 이후 resolve 하기 힘든 conflict가 많이 생겨서 새롭게 PR을 작성했습니다.

Processsystem::dump 출력 시 null character recognizing 문제 해결.

while loop을 이용한 해결방법이 현재 발견한 최선입니다.
magic number 16을 MAXPROCNAME으로 정의했습니다.

지난 주 팀 미팅에서 논의한대로 Console / Printer / Uart의 Ownership을 정돈했습니다.

다음과 같이 struct Console을 변경했습니다.

struct Console {
    terminal: Sleepablelock<Terminal>,
    printer: Spinlock<Printer>,
    uart: Uart,
}

기존의 struct Console를 struct Terminal이라는 임시 이름으로 변경했습니다.
역할이 다르기 때문에 struct Printer와 struct Terminal(구 Console)을 분리해야합니다.
- Terminal은 user program의 input output logging, Printer는 println! 매크로를 위해 존재합니다.
- xv6에서 pr.lock과 cons.lock은 항상 따로 잡힙니다.
Uart가 하나만 존재하게 하는 ownership 정리가 필요합니다.

…meter

kernel-rs/src/console.rs

coolofficials · 2020-11-21T19:22:16Z

kernel-rs/src/console.rs

        match cin {
            // Print process list.
-            m if m == ctrl('P') => {
+            m if m == ctrl('P') => unsafe {


해당 block만 unsafe 하다고 판단했습니다. 나머지 block은 Console::putc->Uart::putc_sync->UartCtrlRegs::read로 safety가 reduce 됩니다.

이 block은 왜 unsafe한가요?

ProcessSystem::dump->p.info.get_mut_unchecked()'를 통해 ProcInfo에 접근합니다.
dump의 unsafe block을 특정지어서 safety를 reduce 해봤습니다.
safety에 대해서 미팅 때 여쭤보겠습니다.

coolofficials · 2020-11-21T19:38:09Z

kernel-rs/src/console.rs

        };
    }

-    unsafe fn write(&mut self, src: UVAddr, n: i32) -> i32 {
+    unsafe fn write(&self, src: UVAddr, n: i32) -> i32 {


write/read/intr 함수들의 소유를 struct Terminal로 옮기는 편이 좋을까요?
그 경우 intr는 putc를 사용하고 있어서 kernel()을 부르게 됩니다.

수정한다면, write도 uart를 사용하고 있어서 kernel()을 불러야 할 것 같습니다.

아래 Printer::Write과 같은 정책을 사용하는 것이 좋다고 생각합니다. (둘 다 각자 소유 / 다 Console이 소유)

@anemoneflower 소유권 표현 상으로는 kernel()을 부르더라도 Terminal으로 이동하는 것이 좋다는 말씀이시죠?

소유권만 보면 Terminal과 Printer가 각각 소유하는 것이 맞는 것 같습니다. 그런데, 코드 구조 상으로는 Console이 다 소유하는 것이 더 깔끔해 보입니다.. Printer와 Terminal이 Console에서만 사용되기 때문에 Console의 소유로 남겨도 괜찮을 것 같은데, 다른 분들의 의견은 어떠신가요?

저는 ownership을 중점적으로 생각해 Terminal과 Printer가 각각 소유하면 좋겠습니다! 추가로 아래의 현재 kaist-cp/rv6의 구현처럼, write/read/intr을 실행하기 전 consoleread/consolewrite/consoleintr에서 lock()을 실행하면 소유권을 더욱 명확히 나타낼 수 있다고 생각합니다!

rv6/kernel-rs/src/console.rs

Lines 201 to 204 in 53d87f7

unsafe fn consolewrite(src: UVAddr, n: i32) -> i32 {

let mut console = kernel().console.lock();

console.write(src, n)

}

@jeehoonkang 교수님, 현재 논점이 global function kernel()의 사용을 지양하는 것과 ownership을 조금 더 세분화하는 것 중에 선택하는 것입니다. 교수님의 의견이 궁금합니다.

@kimjungwow #286 (comment)

같은 질문을 드리고자 하는데 왜 &self로 해도 안전한가요? 여러 쓰레드가 동시 접근해도 문제가 안생기나요?

terminal에 lock을 잡고 있습니다.

Uart::putc의 경우 uart.tx_lock으로 보호됩니다. 여러 쓰레드가 동시에 접근하면 어떤 문제가 생길 수 있나요?

jeehoonkang

감사합니다.. 다만 제가 console, terminal, printer, uart 사이의 관계에 대해 잘 파악하기 어렵습니다. 혹시 주석으로 얘네들이 뭐 하는건지 남겨주시면 감사하겠습니다.

아울러 원래 xv6의 의도를 100% 반영하는지, 아니면 좀 바뀐 부분이 있는지 궁금합니다.

kernel-rs/src/uart.rs

coolofficials · 2020-11-22T16:14:57Z

@jeehoonkang
말씀하신 부분 Commentation 완료했습니다. 별도의 issue로 분리해야하는 discussion들 address 했습니다. Discussion들(especially #286 (comment) #286 (comment) #286 (comment)) 확인하시고 의견 주시거나 resolve 해주시면 감사하겠습니다.

"아울러 원래 xv6의 의도를 100% 반영하는지, 아니면 좀 바뀐 부분이 있는지 궁금합니다."

결론적으로 말하자면 의도를 100% 반영한다고 생각합니다.
xv6가 의도한대로 Uart의 uniqueness, Printer와 Console(현 Terminal)의 분리를 확실히 표현해주는 추상화라고 생각합니다.

결국 세 struct가 전부 User와 interact 하는 Console device와 관련된 interface라는 점에서도 의미를 더욱 명확히 표현하고 있다고 생각합니다. xv6의 조금은 정돈되지 못한 표현들이 rust의 ownership을 통해 정리가 된 것 같습니다.

kernel-rs/src/console.rs

… clear.

kimjungwow

수고하셨습니다! 저는 ownership 위주로 고민해봤습니다 👍👍

kernel-rs/src/console.rs

kimjungwow · 2020-11-23T08:09:51Z

kernel-rs/src/console.rs

        };
    }

-    unsafe fn write(&mut self, src: UVAddr, n: i32) -> i32 {
+    unsafe fn write(&self, src: UVAddr, n: i32) -> i32 {


저는 ownership을 중점적으로 생각해 Terminal과 Printer가 각각 소유하면 좋겠습니다! 추가로 아래의 현재 kaist-cp/rv6의 구현처럼, write/read/intr을 실행하기 전 consoleread/consolewrite/consoleintr에서 lock()을 실행하면 소유권을 더욱 명확히 나타낼 수 있다고 생각합니다!

rv6/kernel-rs/src/console.rs

Lines 201 to 204 in 53d87f7

unsafe fn consolewrite(src: UVAddr, n: i32) -> i32 {

let mut console = kernel().console.lock();

console.write(src, n)

}

kernel-rs/src/console.rs

kernel-rs/src/proc.rs

coolofficials · 2020-11-23T11:01:36Z

@jeehoonkang ping 리뷰 요청드립니다.
소유권 표현의 문제로 global 함수 kernel()의 사용이 많아졌습니다.
소유권의 표현이 kernel()을 지양하는 것보다 우선적인 것으로 합의가 되어 그렇게 진행했습니다.

coolofficials · 2020-11-23T11:11:38Z

@efenniht #186 (comment) 이 부분에 대한 refactoring도 진행된 것 같은데, 확인 부탁드립니다.

struct Console이 모듈이라기보다는 Console device의 추상화가 되었기 때문에 interrupt를 받는다고 생각해도 괜찮을까요?

jeehoonkang

일반적으로 이 PR이 달성하고자하는 바를 잘 모르겠습니다.

jeehoonkang · 2020-11-23T11:46:01Z

kernel-rs/src/console.rs

    pub fn uartintr(&self) {
        self.uart.intr()
    }

    /// Send one character to the uart.
-    pub fn putc(&mut self, c: i32) {
+    pub fn putc(&self, c: i32) {


여전히 이게 &self인게 이해가 잘 안갑니다. 여러 thread가 동시에 putc를 부르면 어떻게 되나요?

생각해봤는데, 명확히 어떤 문제가 생기는지 reasoning이 잘 안됩니다. 면담 때 이런 경우에 어떻게 reasoning 할 수 있는지 가르침을 청해도 될까요?

Uart::putc_sync는 interrupt disable을 통해 synchronization을 handling 중입니다.

jeehoonkang · 2020-11-23T11:47:10Z

kernel-rs/src/console.rs

        };
    }

-    unsafe fn write(&mut self, src: UVAddr, n: i32) -> i32 {
+    unsafe fn write(&self, src: UVAddr, n: i32) -> i32 {


같은 질문을 드리고자 하는데 왜 &self로 해도 안전한가요? 여러 쓰레드가 동시 접근해도 문제가 안생기나요?

jeehoonkang · 2020-11-23T11:47:51Z

kernel-rs/src/console.rs

+
+    unsafe fn read(&self, dst: UVAddr, n: i32) -> i32 {
+        let mut terminal = self.terminal.lock();
+        terminal.read(dst, n)


이렇게 terminal로 바로 보내는 함수들이 많다면, console이라는 abstraction이 틀린 걸지도 모르겠습니다. 어떻게 보시나요?

이전 커밋 상태로 role back 하겠습니다. 그쪽 디자인이 옳은 것 같아서요.

console이라는 abstraction을 만든 이유가 uart의 ownership을 분명히하고자 하는 것인데, 이런식으로 terminal을 부르는게 그 의미를 담지 못하는 것 같습니다.

b2c26c4 에서는 console의 uart를 이용하는 방식이었는데, 해당 디자인에서는 console abstraction의 존재 의미를 잘 살리고 있다고 생각하여 role back 하겠습니다.

혹시 맥락이 잘못되었다면 말씀해주세요.

jeehoonkang · 2020-11-23T11:48:34Z

kernel-rs/src/console.rs

        for i in 0..n {
            let mut c = [0 as u8];
            if VAddr::copyin(&mut c, UVAddr::new(src.into_usize() + (i as usize))).is_err() {
                return i;
            }
-            self.uart.putc(c[0] as i32);
+            kernel().console.uart.putc(c[0] as i32);


저는 이건 잘못된 디자인이라고 생각합니다. uart의 소유권에 대한 명확한 정립이 먼저 필요한 것 같아요.

생각해봤는데, 이러한 디자인을 사용하는 것이 잘못된 것이 맞는 것 같습니다. 이번 PR의 목적 중 하나가 uart의 소유권을 console이 가지도록 하는 것인데, 이렇게 사용하면 의미가 퇴색되는 것 같습니다.

b2c26c4 에서처럼 uart의 소유권을 console이 지니는 것을 분명히 하여 console의 존재 의미를 살리는 방식으로 role back 하겠습니다.

jeehoonkang · 2020-11-23T11:49:19Z

kernel-rs/src/console.rs

        match cin {
            // Print process list.
-            m if m == ctrl('P') => {
+            m if m == ctrl('P') => unsafe {


이 block은 왜 unsafe한가요?

coolofficials

putc의 multithread 접근에 대해서 조금 더 생각해보겠습니다.

coolofficials · 2020-11-23T21:50:22Z

kernel-rs/src/console.rs

        };
    }

-    unsafe fn write(&mut self, src: UVAddr, n: i32) -> i32 {
+    unsafe fn write(&self, src: UVAddr, n: i32) -> i32 {


terminal에 lock을 잡고 있습니다.

Uart::putc의 경우 uart.tx_lock으로 보호됩니다. 여러 쓰레드가 동시에 접근하면 어떤 문제가 생길 수 있나요?

coolofficials · 2020-11-23T22:02:53Z

kernel-rs/src/console.rs

        match cin {
            // Print process list.
-            m if m == ctrl('P') => {
+            m if m == ctrl('P') => unsafe {


ProcessSystem::dump->p.info.get_mut_unchecked()'를 통해 ProcInfo에 접근합니다.
dump의 unsafe block을 특정지어서 safety를 reduce 해봤습니다.
safety에 대해서 미팅 때 여쭤보겠습니다.

coolofficials · 2020-11-23T22:19:43Z

kernel-rs/src/console.rs

+
+    unsafe fn read(&self, dst: UVAddr, n: i32) -> i32 {
+        let mut terminal = self.terminal.lock();
+        terminal.read(dst, n)


이전 커밋 상태로 role back 하겠습니다. 그쪽 디자인이 옳은 것 같아서요.

console이라는 abstraction을 만든 이유가 uart의 ownership을 분명히하고자 하는 것인데, 이런식으로 terminal을 부르는게 그 의미를 담지 못하는 것 같습니다.

b2c26c4 에서는 console의 uart를 이용하는 방식이었는데, 해당 디자인에서는 console abstraction의 존재 의미를 잘 살리고 있다고 생각하여 role back 하겠습니다.

혹시 맥락이 잘못되었다면 말씀해주세요.

coolofficials · 2020-11-23T22:21:35Z

kernel-rs/src/console.rs

        for i in 0..n {
            let mut c = [0 as u8];
            if VAddr::copyin(&mut c, UVAddr::new(src.into_usize() + (i as usize))).is_err() {
                return i;
            }
-            self.uart.putc(c[0] as i32);
+            kernel().console.uart.putc(c[0] as i32);


생각해봤는데, 이러한 디자인을 사용하는 것이 잘못된 것이 맞는 것 같습니다. 이번 PR의 목적 중 하나가 uart의 소유권을 console이 가지도록 하는 것인데, 이렇게 사용하면 의미가 퇴색되는 것 같습니다.

b2c26c4 에서처럼 uart의 소유권을 console이 지니는 것을 분명히 하여 console의 존재 의미를 살리는 방식으로 role back 하겠습니다.

coolofficials · 2020-11-26T13:38:05Z

우선 procdump debugging만 하는 정도에서 PR을 마무리하고 console/printer/uart refactoring은 추후에 별도의 issue로 다루기로 해서 close합니다.

coolofficials added 8 commits November 21, 2020 17:27

Fix: procdump mis-printing & magic number process name length to para…

4dcb43f

…meter

refactor Console

91a7381

name changed

779ac15

clear ownership of uart

9108c3a

variable name changed

239f597

specify unsafe block

480b020

comment fixed

f0ac9d7

specify unsafe block

53930a9

coolofficials commented Nov 21, 2020

View reviewed changes

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

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

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

coolofficials marked this pull request as draft November 21, 2020 19:10

new line on pattern matching

38f7f15

coolofficials commented Nov 21, 2020

View reviewed changes

name changed

fa1dd1c

coolofficials commented Nov 21, 2020

View reviewed changes

coolofficials marked this pull request as ready for review November 21, 2020 19:53

comment fixed

8d9363e

coolofficials mentioned this pull request Nov 22, 2020

Modeling safety of UART or is it bug? #188

Closed

coolofficials requested review from jeehoonkang, kimjungwow and anemoneflower November 22, 2020 14:05

jeehoonkang requested changes Nov 22, 2020

View reviewed changes

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

Commentation for console/terminal/printer/uart.

c02ce31

This was referenced Nov 22, 2020

Ownership of UartCtrlRegs to UART? #287

Closed

Make consoleinit as Kernel's method. (Refactoring Devsw?) #288

Closed

coolofficials added 5 commits November 22, 2020 16:16

fmt & comment style changed.

69fa134

Make comment detailed

72f2a0d

commentation grammar fixed

38b6dde

Add more comments.

46cacaa

Method name changed to reveal ownership clear

991ac04

coolofficials commented Nov 22, 2020

View reviewed changes

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

coolofficials requested a review from jeehoonkang November 22, 2020 18:29

coolofficials added 2 commits November 22, 2020 18:31

[Fix Error in previous commit]Method name changed to reveal ownership…

a98abe9

… clear.

Fix error from previous commit

5009196

coolofficials mentioned this pull request Nov 23, 2020

Merge mit pdos [4] (related to #261) #281

Merged

21 tasks

coolofficials added 2 commits November 23, 2020 02:07

Minor comment fix to clarify meaning.

92d1db3

Grammar fixed

e4dfdd5

coolofficials mentioned this pull request Nov 23, 2020

[closes #265] Intrusive doubly linked list #289

Closed

kimjungwow reviewed Nov 23, 2020

View reviewed changes

coolofficials added 4 commits November 23, 2020 08:38

addressed review for proc.rs

0361330

Printer to ZST

b2c26c4

ownership modified

2451fed

comment fixed

427eed5

jeehoonkang requested changes Nov 23, 2020

View reviewed changes

coolofficials commented Nov 23, 2020

View reviewed changes

coolofficials added 5 commits November 23, 2020 22:27

safty reduced

5b40d8f

make meaning of console strong

1d3eb69

now uartintr is not method of console

29a18d9

reference type changed

be7288a

Merge branch 'riscv' into procdump-debug

5dee6fa

coolofficials closed this Nov 26, 2020

coolofficials mentioned this pull request Nov 30, 2020

Console-Uart-Printer Refactoring #298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#closes 243] Procdump debug & Refactor console struct. #286

[#closes 243] Procdump debug & Refactor console struct. #286

coolofficials commented Nov 21, 2020 •

edited

Loading

coolofficials Nov 21, 2020

jeehoonkang Nov 23, 2020

coolofficials Nov 23, 2020

coolofficials Nov 21, 2020

anemoneflower Nov 23, 2020

coolofficials Nov 23, 2020

anemoneflower Nov 23, 2020

kimjungwow Nov 23, 2020

coolofficials Nov 23, 2020

coolofficials Nov 23, 2020

jeehoonkang Nov 23, 2020

coolofficials Nov 23, 2020 •

edited

Loading

jeehoonkang left a comment

coolofficials commented Nov 22, 2020 •

edited

Loading

kimjungwow left a comment

kimjungwow Nov 23, 2020

coolofficials commented Nov 23, 2020 •

edited

Loading

coolofficials commented Nov 23, 2020 •

edited

Loading

jeehoonkang left a comment

jeehoonkang Nov 23, 2020

coolofficials Nov 23, 2020 •

edited

Loading

jeehoonkang Nov 23, 2020

jeehoonkang Nov 23, 2020

coolofficials Nov 23, 2020 •

edited

Loading

jeehoonkang Nov 23, 2020

coolofficials Nov 23, 2020 •

edited

Loading

jeehoonkang Nov 23, 2020

coolofficials left a comment

coolofficials Nov 23, 2020 •

edited

Loading

coolofficials Nov 23, 2020

coolofficials Nov 23, 2020 •

edited

Loading

coolofficials Nov 23, 2020 •

edited

Loading

coolofficials commented Nov 26, 2020

	unsafe fn consolewrite(src: UVAddr, n: i32) -> i32 {
	let mut console = kernel().console.lock();
	console.write(src, n)
	}

[#closes 243] Procdump debug & Refactor console struct. #286

[#closes 243] Procdump debug & Refactor console struct. #286

Conversation

coolofficials commented Nov 21, 2020 • 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

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

coolofficials Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

jeehoonkang left a comment

Choose a reason for hiding this comment

coolofficials commented Nov 22, 2020 • edited Loading

kimjungwow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coolofficials commented Nov 23, 2020 • edited Loading

coolofficials commented Nov 23, 2020 • edited Loading

jeehoonkang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coolofficials Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coolofficials Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coolofficials Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coolofficials left a comment

Choose a reason for hiding this comment

coolofficials Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coolofficials Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

coolofficials Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

coolofficials commented Nov 26, 2020

coolofficials commented Nov 21, 2020 •

edited

Loading

coolofficials Nov 23, 2020 •

edited

Loading

coolofficials commented Nov 22, 2020 •

edited

Loading

coolofficials commented Nov 23, 2020 •

edited

Loading

coolofficials commented Nov 23, 2020 •

edited

Loading

coolofficials Nov 23, 2020 •

edited

Loading

coolofficials Nov 23, 2020 •

edited

Loading

coolofficials Nov 23, 2020 •

edited

Loading

coolofficials Nov 23, 2020 •

edited

Loading

coolofficials Nov 23, 2020 •

edited

Loading

coolofficials Nov 23, 2020 •

edited

Loading