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 11 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
158 changes: 93 additions & 65 deletions kernel-rs/src/console.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,48 +3,29 @@ use crate::{
kernel::kernel,
param::NDEV,
proc::myproc,
sleepablelock::SleepablelockGuard,
sleepablelock::Sleepablelock,
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;

pub struct Console {
buf: [u8; INPUT_BUF],

/// Read index.
r: u32,

/// Write index.
w: u32,

/// Edit index.
e: u32,

terminal: Sleepablelock<Terminal>,
pub printer: Spinlock<Printer>,
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(),
}
}
Expand All @@ -54,22 +35,23 @@ impl Console {
}

/// 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으로 보호됩니다. 여러 쓰레드가 동시에 접근하면 어떤 문제가 생길 수 있나요?

self.terminal.lock();
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() {
Expand All @@ -80,27 +62,28 @@ impl Console {
n
}

unsafe fn read(this: &mut SleepablelockGuard<'_, Self>, mut dst: UVAddr, mut n: i32) -> i32 {
unsafe fn read(&self, mut dst: UVAddr, mut n: i32) -> i32 {
let mut terminal = self.terminal.lock();
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 terminal.r == terminal.w {
if (*myproc()).killed() {
return -1;
}
this.sleep();
terminal.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 = terminal.r;
terminal.r = terminal.r.wrapping_add(1);
let cin = terminal.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)
terminal.r = terminal.r.wrapping_sub(1)
}
break;
} else {
Expand All @@ -121,58 +104,104 @@ impl Console {
target.wrapping_sub(n as u32) as i32
}

unsafe fn intr(this: &mut SleepablelockGuard<'_, Self>, mut cin: i32) {
fn intr(&self, mut cin: i32) {
let mut terminal = self.terminal.lock();
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 terminal.e != terminal.w
&& terminal.buf
[terminal.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);
terminal.e = terminal.e.wrapping_sub(1);
self.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 terminal.e != terminal.w {
terminal.e = terminal.e.wrapping_sub(1);
self.putc(BACKSPACE);
}
}

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

// Echo back to the user.
this.putc(cin);
self.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 = terminal.e;
terminal.e = terminal.e.wrapping_add(1);
terminal.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)
|| terminal.e == terminal.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();
terminal.w = terminal.e;
terminal.wakeup();
}
}
}
}
}
}

pub struct Terminal {
coolofficials marked this conversation as resolved.
Show resolved Hide resolved
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,
}
}
}

pub struct Printer {
_padding: u8,
coolofficials marked this conversation as resolved.
Show resolved Hide resolved
}

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

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,7 +217,9 @@ const fn ctrl(x: char) -> i32 {
x as i32 - '@' as i32
}

pub unsafe fn consoleinit(devsw: &mut [Devsw; NDEV]) {
pub fn consoleinit(devsw: &mut [Devsw; NDEV]) {
kernel().console.uart.init();
coolofficials marked this conversation as resolved.
Show resolved Hide resolved

// Connect read and write system calls
// to consoleread and consolewrite.
devsw[CONSOLE_IN_DEVSW] = Devsw {
Expand All @@ -199,24 +230,21 @@ pub unsafe fn consoleinit(devsw: &mut [Devsw; NDEV]) {

/// 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)
kernel().console.read(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);
pub fn consoleintr(cin: i32) {
kernel().console.intr(cin);
}
10 changes: 4 additions & 6 deletions kernel-rs/src/kernel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 @@ -157,9 +156,9 @@ impl Kernel {
/// Prints the given formatted string with the Console.
pub fn console_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 @@ -220,7 +219,6 @@ pub unsafe fn kernel_main() -> ! {
// Initialize the kernel.

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

println!();
Expand Down
31 changes: 17 additions & 14 deletions kernel-rs/src/param.rs
Original file line number Diff line number Diff line change
@@ -1,39 +1,42 @@
/// maximum number of processes
/// Maximum number of processes.
pub const NPROC: usize = 64;

/// maximum number of CPUs
/// Maximum number of CPUs.
pub const NCPU: usize = 8;

/// open files per process
/// Open files per process.
pub const NOFILE: usize = 16;

/// open files per system
/// Open files per system.
pub const NFILE: usize = 100;

/// maximum number of active i-nodes
/// Maximum number of active i-nodes.
pub const NINODE: usize = 50;

/// maximum major device number
/// Maximum major device number.
pub const NDEV: usize = 10;

/// device number of file system root disk
/// Device number of file system root disk.
pub const ROOTDEV: i32 = 1;

/// max exec arguments
/// Max exec arguments.
pub const MAXARG: usize = 32;

/// max # of blocks any FS op writes
/// Will be handled in #31
/// Max # of blocks any FS op writes.
/// Will be handled in #31.
pub const MAXOPBLOCKS: usize = 10;

/// max data blocks in on-disk log
/// Max data blocks in on-disk log.
pub const LOGSIZE: usize = MAXOPBLOCKS * 3;

/// size of disk block cache
/// Size of disk block cache.
pub const NBUF: usize = MAXOPBLOCKS * 3;

/// size of file system in blocks
/// Size of file system in blocks.
pub const FSSIZE: usize = 1000;

/// maximum file path name
/// Maximum file path name.
pub const MAXPATH: usize = 128;

/// Maximum length of process name.
pub const MAXPROCNAME: usize = 16;
Loading