Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
4dcb43f
Fix: procdump mis-printing & magic number process name length to para…
coolofficials Nov 21, 2020
91a7381
refactor Console
coolofficials Nov 21, 2020
779ac15
name changed
coolofficials Nov 21, 2020
9108c3a
clear ownership of uart
coolofficials Nov 21, 2020
239f597
variable name changed
coolofficials Nov 21, 2020
480b020
specify unsafe block
coolofficials Nov 21, 2020
f0ac9d7
comment fixed
coolofficials Nov 21, 2020
53930a9
specify unsafe block
coolofficials Nov 21, 2020
38f7f15
new line on pattern matching
coolofficials Nov 21, 2020
fa1dd1c
name changed
coolofficials Nov 21, 2020
8d9363e
comment fixed
coolofficials Nov 21, 2020
c02ce31
Commentation for console/terminal/printer/uart.
coolofficials Nov 22, 2020
69fa134
fmt & comment style changed.
coolofficials Nov 22, 2020
72f2a0d
Make comment detailed
coolofficials Nov 22, 2020
38b6dde
commentation grammar fixed
coolofficials Nov 22, 2020
46cacaa
Add more comments.
coolofficials Nov 22, 2020
991ac04
Method name changed to reveal ownership clear
coolofficials Nov 22, 2020
a98abe9
[Fix Error in previous commit]Method name changed to reveal ownership…
coolofficials Nov 22, 2020
5009196
Fix error from previous commit
coolofficials Nov 22, 2020
92d1db3
Minor comment fix to clarify meaning.
coolofficials Nov 23, 2020
e4dfdd5
Grammar fixed
coolofficials Nov 23, 2020
0361330
addressed review for proc.rs
coolofficials Nov 23, 2020
b2c26c4
Printer to ZST
coolofficials Nov 23, 2020
2451fed
ownership modified
coolofficials Nov 23, 2020
427eed5
comment fixed
coolofficials Nov 23, 2020
5b40d8f
safty reduced
coolofficials Nov 23, 2020
1d3eb69
make meaning of console strong
coolofficials Nov 23, 2020
29a18d9
now uartintr is not method of console
coolofficials Nov 24, 2020
be7288a
reference type changed
coolofficials Nov 25, 2020
5dee6fa
Merge branch 'riscv' into procdump-debug
coolofficials Nov 25, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
205 changes: 127 additions & 78 deletions kernel-rs/src/console.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,104 +3,154 @@ use crate::{
kernel::kernel,
param::NDEV,
proc::myproc,
sleepablelock::SleepablelockGuard,
sleepablelock::{Sleepablelock, SleepablelockGuard},
spinlock::Spinlock,
uart::Uart,
utils::spin_loop,
vm::{UVAddr, VAddr},
};
use core::fmt;
use core::fmt::{self, Write};

const CONSOLE_IN_DEVSW: usize = 1;
/// Size of console input buffer.
const INPUT_BUF: usize = 128;

/// The integrated interface for console device.
/// Managing every communication between real-world users and system.
pub struct Console {
buf: [u8; INPUT_BUF],
/// An interface for user programs.
/// I/O interactions between users and processes are done by this interface.
/// Inputs: User commands.
/// Outputs: Graphic(text) results.
terminal: Sleepablelock<Terminal>,

/// Read index.
r: u32,

/// Write index.
w: u32,

/// Edit index.
e: u32,
/// A struct for println! macro.
/// Lock to avoid interleaving concurrent println! macros.
pub printer: Spinlock<Printer>,

/// A console's uart interface which guarantees uniqueness.
/// Interaction with UART Registers should always held by this interface.
uart: Uart,
}

impl fmt::Write for Console {
fn write_str(&mut self, s: &str) -> fmt::Result {
for c in s.bytes() {
self.putc(c as _);
}
Ok(())
}
}

impl Console {
pub const fn new() -> Self {
Self {
buf: [0; INPUT_BUF],
r: 0,
w: 0,
e: 0,
terminal: Sleepablelock::new("Terminal", Terminal::new()),
printer: Spinlock::new("Println", Printer::new()),
uart: Uart::new(),
}
}

pub fn init(&mut self, devsw: &mut [Devsw; NDEV]) {
self.uart.init();

// Connect read and write system calls
// to consoleread and consolewrite.
devsw[CONSOLE_IN_DEVSW] = Devsw {
read: Some(consoleread),
write: Some(consolewrite),
};
}

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@coolofficials coolofficials Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

// From printf.rs.
if kernel().is_panicked() {
spin_loop();
}
if c == BACKSPACE {
// If the user typed backspace, overwrite with a space.
Uart::putc_sync('\u{8}' as i32);
Uart::putc_sync(' ' as i32);
Uart::putc_sync('\u{8}' as i32);
self.uart.putc_sync('\u{8}' as i32);
self.uart.putc_sync(' ' as i32);
self.uart.putc_sync('\u{8}' as i32);
} else {
Uart::putc_sync(c);
self.uart.putc_sync(c);
};
}

unsafe fn write(&mut self, src: UVAddr, n: i32) -> i32 {
unsafe fn write(&self, src: UVAddr, n: i32) -> i32 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 수정한다면, writeuart를 사용하고 있어서 kernel()을 불러야 할 것 같습니다.
  • 아래 Printer::Write과 같은 정책을 사용하는 것이 좋다고 생각합니다. (둘 다 각자 소유 / 다 Console이 소유)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@coolofficials coolofficials Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let terminal = self.terminal.lock();
terminal.write(src, n)
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@coolofficials coolofficials Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • console이라는 abstraction을 만든 이유가 uart의 ownership을 분명히하고자 하는 것인데, 이런식으로 terminal을 부르는게 그 의미를 담지 못하는 것 같습니다.
  • b2c26c4 에서는 console의 uart를 이용하는 방식이었는데, 해당 디자인에서는 console abstraction의 존재 의미를 잘 살리고 있다고 생각하여 role back 하겠습니다.
  • 혹시 맥락이 잘못되었다면 말씀해주세요.

}

/// The console input interrupt handler.
/// uartintr() calls this for input character.
/// Do erase/kill processing, append to TERMINAL.buf,
/// wake up consoleread() if a whole line has arrived.
pub fn intr(&self, cin: i32) {
let mut terminal = self.terminal.lock();
terminal.intr(cin);
}
}

/// Accept user commands and show result texts.
pub struct Terminal {
buf: [u8; INPUT_BUF],

/// Read index.
r: u32,

/// Write index.
w: u32,

/// Edit index.
e: u32,
}

impl Terminal {
pub const fn new() -> Self {
Self {
buf: [0; INPUT_BUF],
r: 0,
w: 0,
e: 0,
}
}
}

impl SleepablelockGuard<'_, Terminal> {
unsafe fn write(&self, src: UVAddr, n: i32) -> i32 {
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@coolofficials coolofficials Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

}
n
}

unsafe fn read(this: &mut SleepablelockGuard<'_, Self>, mut dst: UVAddr, mut n: i32) -> i32 {
unsafe fn read(&mut self, mut dst: UVAddr, mut n: i32) -> i32 {
let target = n as u32;
while n > 0 {
// Wait until interrupt handler has put some
// input into CONS.buffer.
while this.r == this.w {
while self.r == self.w {
if (*myproc()).killed() {
return -1;
}
this.sleep();
self.sleep();
}
let fresh0 = this.r;
this.r = this.r.wrapping_add(1);
let cin = this.buf[fresh0.wrapping_rem(INPUT_BUF as u32) as usize] as i32;
let fresh0 = self.r;
self.r = self.r.wrapping_add(1);
let cin = self.buf[fresh0.wrapping_rem(INPUT_BUF as u32) as usize] as i32;

// end-of-file
if cin == ctrl('D') {
if (n as u32) < target {
// Save ^D for next time, to make sure
// caller gets a 0-byte result.
this.r = this.r.wrapping_sub(1)
self.r = self.r.wrapping_sub(1)
}
break;
} else {
Expand All @@ -121,58 +171,77 @@ impl Console {
target.wrapping_sub(n as u32) as i32
}

unsafe fn intr(this: &mut SleepablelockGuard<'_, Self>, mut cin: i32) {
fn intr(&mut self, mut cin: i32) {
match cin {
// Print process list.
m if m == ctrl('P') => {
m if m == ctrl('P') => unsafe {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 block은 왜 unsafe한가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

kernel().procs.dump();
}
},

// Kill line.
m if m == ctrl('U') => {
while this.e != this.w
&& this.buf[this.e.wrapping_sub(1).wrapping_rem(INPUT_BUF as u32) as usize]
while self.e != self.w
&& self.buf[self.e.wrapping_sub(1).wrapping_rem(INPUT_BUF as u32) as usize]
as i32
!= '\n' as i32
{
this.e = this.e.wrapping_sub(1);
this.putc(BACKSPACE);
self.e = self.e.wrapping_sub(1);
kernel().console.uart.putc(BACKSPACE);
}
}

// Backspace
m if m == ctrl('H') | '\x7f' as i32 => {
if this.e != this.w {
this.e = this.e.wrapping_sub(1);
this.putc(BACKSPACE);
if self.e != self.w {
self.e = self.e.wrapping_sub(1);
kernel().console.uart.putc(BACKSPACE);
}
}

_ => {
if cin != 0 && this.e.wrapping_sub(this.r) < INPUT_BUF as u32 {
if cin != 0 && self.e.wrapping_sub(self.r) < INPUT_BUF as u32 {
cin = if cin == '\r' as i32 { '\n' as i32 } else { cin };

// Echo back to the user.
this.putc(cin);
kernel().console.uart.putc(cin);

// Store for consumption by consoleread().
let fresh1 = this.e;
this.e = this.e.wrapping_add(1);
this.buf[fresh1.wrapping_rem(INPUT_BUF as u32) as usize] = cin as u8;
let fresh1 = self.e;
self.e = self.e.wrapping_add(1);
self.buf[fresh1.wrapping_rem(INPUT_BUF as u32) as usize] = cin as u8;
if cin == '\n' as i32
|| cin == ctrl('D')
|| this.e == this.r.wrapping_add(INPUT_BUF as u32)
|| self.e == self.r.wrapping_add(INPUT_BUF as u32)
{
// Wake up consoleread() if a whole line (or end-of-file)
// has arrived.
this.w = this.e;
this.wakeup();
self.w = self.e;
self.wakeup();
}
}
}
}
}
}

/// A printer formats UTF-8 bytes.
pub struct Printer {}

impl Printer {
pub const fn new() -> Self {
Self {}
}
}

impl Write for Printer {
fn write_str(&mut self, s: &str) -> fmt::Result {
for c in s.bytes() {
kernel().console.putc(c as _);
coolofficials marked this conversation as resolved.
Show resolved Hide resolved
}
Ok(())
}
}

/// Console input and output, to the uart.
/// Reads are line at a time.
/// Implements special input characters:
Expand All @@ -188,35 +257,15 @@ const fn ctrl(x: char) -> i32 {
x as i32 - '@' as i32
}

pub unsafe fn consoleinit(devsw: &mut [Devsw; NDEV]) {
// Connect read and write system calls
// to consoleread and consolewrite.
devsw[CONSOLE_IN_DEVSW] = Devsw {
read: Some(consoleread),
write: Some(consolewrite),
};
}

/// User write()s to the console go here.
unsafe fn consolewrite(src: UVAddr, n: i32) -> i32 {
let mut console = kernel().console.lock();
console.write(src, n)
kernel().console.write(src, n)
}

/// User read()s from the console go here.
/// Copy (up to) a whole input line to dst.
/// User_dist indicates whether dst is a user
/// or kernel address.
unsafe fn consoleread(dst: UVAddr, n: i32) -> i32 {
let mut console = kernel().console.lock();
Console::read(&mut console, dst, n)
}

/// The console input interrupt handler.
/// uartintr() calls this for input character.
/// Do erase/kill processing, append to CONS.buf,
/// wake up consoleread() if a whole line has arrived.
pub unsafe fn consoleintr(cin: i32) {
let mut console = kernel().console.lock();
Console::intr(&mut console, cin);
kernel().console.read(dst, n)
}
20 changes: 9 additions & 11 deletions kernel-rs/src/kernel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use spin::Once;
use crate::{
arena::{ArrayArena, ArrayEntry, MruArena, MruEntry},
bio::BufEntry,
console::{consoleinit, Console},
console::Console,
file::{Devsw, File},
fs::{FileSystem, Inode},
kalloc::{end, kinit, Kmem},
Expand All @@ -19,7 +19,6 @@ use crate::{
sleepablelock::Sleepablelock,
spinlock::Spinlock,
trap::{trapinit, trapinithart},
uart::Uart,
virtio_disk::{virtio_disk_init, Disk},
vm::{KVAddr, PageTable},
};
Expand All @@ -37,7 +36,7 @@ pub struct Kernel {
panicked: AtomicBool,

/// Sleeps waiting for there are some input in console buffer.
pub console: Sleepablelock<Console>,
pub console: Console,

kmem: Spinlock<Kmem>,

Expand Down Expand Up @@ -90,7 +89,7 @@ impl Kernel {
const fn zero() -> Self {
Self {
panicked: AtomicBool::new(false),
console: Sleepablelock::new("CONS", Console::new()),
console: Console::new(),
kmem: Spinlock::new("KMEM", Kmem::new()),
page_table: PageTable::zero(),
ticks: Sleepablelock::new("time", 0),
Expand Down Expand Up @@ -154,12 +153,12 @@ impl Kernel {
Some(page)
}

/// Prints the given formatted string with the Console.
pub fn console_write_fmt(&self, args: fmt::Arguments<'_>) -> fmt::Result {
/// Prints the given formatted string with the Printer.
pub fn printer_write_fmt(&self, args: fmt::Arguments<'_>) -> fmt::Result {
if self.is_panicked() {
unsafe { kernel().console.get_mut_unchecked().write_fmt(args) }
unsafe { self.console.printer.get_mut_unchecked().write_fmt(args) }
} else {
let mut lock = kernel().console.lock();
let mut lock = self.console.printer.lock();
lock.write_fmt(args)
}
}
Expand Down Expand Up @@ -190,7 +189,7 @@ impl Kernel {
#[macro_export]
macro_rules! print {
($($arg:tt)*) => {
$crate::kernel::kernel().console_write_fmt(format_args!($($arg)*)).unwrap();
$crate::kernel::kernel().printer_write_fmt(format_args!($($arg)*)).unwrap();
};
}

Expand Down Expand Up @@ -220,8 +219,7 @@ pub unsafe fn kernel_main() -> ! {
// Initialize the kernel.

// Console.
Uart::init();
consoleinit(&mut KERNEL.devsw);
KERNEL.console.init(&mut KERNEL.devsw);

println!();
println!("rv6 kernel is booting");
Expand Down
Loading