Skip to content

Commit

Permalink
Merge #362
Browse files Browse the repository at this point in the history 
362: [Closes #365] Fix TODOs in pipe.rs r=Medowhill a=travis1829

Closes #365 
* #356 에서의 변경사항 중 `Pin` API와 관련없는 내용만 따로 빼서 새롭게 PR을 만들었습니다. (주로 `Pipe`에 대한 변경사항입니다.)
* `Pipe`의 `Drop` trait을 추가했고 `Copy`, `Clone` trait을 없앴습니다.
* 추가로, `Pipe::copy`가 `unsafe`일 필요가 없는 것 같아서 없앴습니다. (혹시 `unsafe`이여야하는 이유가 있다면 말씀해주세요.)
* kernel.rs에 있던 주석을 일부 지웠습니다. (#356 (comment))

Co-authored-by: travis1829 <[email protected]>
  • Loading branch information
@travis1829
kaist-cp-bors[bot] and travis1829 authored Jan 29, 2021
2 parents 026a808 + 9918c32 commit 7bf39e7
Show file tree
Hide file tree
Showing 2 changed files with 96 additions and 60 deletions.
6 changes: 3 additions & 3 deletions kernel-rs/src/file.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ impl File {
}

match &self.typ {
FileType::Pipe { pipe } => unsafe { pipe.read(addr, usize::try_from(n).unwrap_or(0)) },
FileType::Pipe { pipe } => pipe.read(addr, usize::try_from(n).unwrap_or(0)),
FileType::Inode { ip, off } => {
let mut ip = ip.deref().lock();
let curr_off = unsafe { *off.get() };
Expand All @@ -126,7 +126,7 @@ impl File {
}

match &self.typ {
FileType::Pipe { pipe } => unsafe { pipe.write(addr, usize::try_from(n).unwrap_or(0)) },
FileType::Pipe { pipe } => pipe.write(addr, usize::try_from(n).unwrap_or(0)),
FileType::Inode { ip, off } => {
// write a few blocks at a time to avoid exceeding
// the maximum log transaction size, including
Expand Down Expand Up @@ -174,7 +174,7 @@ impl ArenaObject for File {
A::reacquire_after(guard, || {
let typ = mem::replace(&mut self.typ, FileType::None);
match typ {
FileType::Pipe { mut pipe } => unsafe { pipe.close(self.writable) },
FileType::Pipe { pipe } => pipe.close(self.writable),
FileType::Inode { ip, .. } | FileType::Device { ip, .. } => {
// TODO(https://github.com/kaist-cp/rv6/issues/290)
// The inode ip will be dropped by drop(ip). Deallocation
Expand Down
150 changes: 93 additions & 57 deletions kernel-rs/src/pipe.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
spinlock::Spinlock,
vm::UVAddr,
};
use core::{mem, ops::Deref, ptr};
use core::{mem, ops::Deref, ptr::NonNull};
use static_assertions::const_assert;

const PIPESIZE: usize = 512;
Expand Down Expand Up @@ -39,13 +39,14 @@ pub struct Pipe {
}

impl Pipe {
/// PipeInner::try_read() tries to read as much as possible.
/// Pipe::read() executes try_read() until all bytes in pipe are read.
//TODO(https://github.com/kaist-cp/rv6/issues/366) : `n` should be u32
pub unsafe fn read(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {
/// Tries to read up to `n` bytes using `Pipe::try_read()`.
/// If successfully read i > 0 bytes, wakeups the `write_waitchannel` and returns `Ok(i: usize)`.
/// If the pipe was empty, sleeps at `read_waitchannel` and tries again after wakeup.
/// If an error happened, returns `Err(())`.
pub fn read(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {
let mut inner = self.inner.lock();
loop {
match unsafe { inner.try_read(addr, n) } {
match inner.try_read(addr, n) {
Ok(r) => {
//DOC: piperead-wakeup
self.write_waitchannel.wakeup();
Expand All @@ -60,13 +61,17 @@ impl Pipe {
}
}

/// PipeInner::try_write() tries to write as much as possible.
/// Pipe::write() executes try_write() until `n` bytes are written.
pub unsafe fn write(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {
/// Tries to write up to `n` bytes by repeatedly calling `Pipe::try_write()`.
/// Wakeups `read_waitchannel` for every successful `Pipe::try_write()`.
/// After successfully writing i >= 0 bytes, returns `Ok(i)`.
/// Note that we may have i < `n` if an copy-in error happened.
/// If the pipe was full, sleeps at `write_waitchannel` and tries again after wakeup.
/// If an error happened, returns `Err(())`.
pub fn write(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {
let mut written = 0;
let mut inner = self.inner.lock();
loop {
match unsafe { inner.try_write(addr + written, n - written) } {
match inner.try_write(addr + written, n - written) {
Ok(r) => {
written += r;
self.read_waitchannel.wakeup();
Expand All @@ -85,7 +90,7 @@ impl Pipe {
}
}

unsafe fn close(&self, writable: bool) -> bool {
fn close(&self, writable: bool) -> bool {
let mut inner = self.inner.lock();

if writable {
Expand All @@ -96,73 +101,80 @@ impl Pipe {
self.write_waitchannel.wakeup();
}

// Return whether pipe would be freed or not
// Return whether pipe should be freed or not.
!inner.readopen && !inner.writeopen
}
}

/// # Safety
///
/// `ptr` always refers to a `Pipe`.
/// Also, for a single `Pipe`, we have a single read-only `AllocatedPipe` and a single write-only `AllocatedPipe`.
/// The `PipeInner`'s readopen/writeopen field denotes whether the read-only/write-only `AllocatedPipe` is still open,
/// and hence, we can safely free the `Pipe` only after both the readopen/writeopen field is false, since this means
/// all `AllocatedPipe`s were closed.
pub struct AllocatedPipe {
ptr: *mut Pipe,
ptr: NonNull<Pipe>,
}

impl Deref for AllocatedPipe {
type Target = Pipe;
fn deref(&self) -> &Self::Target {
unsafe { &*self.ptr }
// Safe since `ptr` always refers to a `Pipe`.
unsafe { self.ptr.as_ref() }
}
}

impl AllocatedPipe {
pub fn alloc() -> Result<(RcFile<'static>, RcFile<'static>), ()> {
let page = kernel().alloc().ok_or(())?;
let ptr = page.into_usize() as *mut Pipe;
let mut ptr = unsafe {
// Safe since by the invariant of `Page`, `page` is always non-null.
NonNull::new_unchecked(page.into_usize() as *mut Pipe)
};

// `Pipe` must be align with `Page`
// `Pipe` must be aligned with `Page`.
const_assert!(mem::size_of::<Pipe>() <= PGSIZE);

//TODO(https://github.com/kaist-cp/rv6/issues/367): Since Pipe is a huge struct, need to check whether stack is used to fill `*ptr`
// It is safe because unique access to page is guaranteed since page is just allocated,
// and the pipe size and alignment are compatible with the page.
//TODO(https://github.com/kaist-cp/rv6/issues/367): Since Pipe is a huge struct, need to check whether stack is used to fill `*ptr`.
unsafe {
ptr::write(
ptr,
Pipe {
inner: Spinlock::new(
"pipe",
PipeInner {
data: [0; PIPESIZE],
nwrite: 0,
nread: 0,
readopen: true,
writeopen: true,
},
),
read_waitchannel: WaitChannel::new(),
write_waitchannel: WaitChannel::new(),
},
);
};
// Safe since `ptr` holds a valid, unique page allocated from `kernel().alloc()`,
// and the pipe size and alignment are compatible with the page.
*ptr.as_mut() = Pipe {
inner: Spinlock::new(
"pipe",
PipeInner {
data: [0; PIPESIZE],
nwrite: 0,
nread: 0,
readopen: true,
writeopen: true,
},
),
read_waitchannel: WaitChannel::new(),
write_waitchannel: WaitChannel::new(),
};
}
let f0 = kernel()
.ftable
.alloc_file(FileType::Pipe { pipe: Self { ptr } }, true, false)
// It is safe because ptr is an address of page, which obtained by alloc()
.map_err(|_| kernel().free(unsafe { Page::from_usize(ptr as _) }))?;
// Safe since ptr is an address of a page obtained by alloc().
.map_err(|_| kernel().free(unsafe { Page::from_usize(ptr.as_ptr() as _) }))?;
let f1 = kernel()
.ftable
.alloc_file(FileType::Pipe { pipe: Self { ptr } }, false, true)
// It is safe because ptr is an address of page, which obtained by alloc()
.map_err(|_| kernel().free(unsafe { Page::from_usize(ptr as _) }))?;
// Safe since ptr is an address of a page obtained by alloc().
.map_err(|_| kernel().free(unsafe { Page::from_usize(ptr.as_ptr() as _) }))?;

Ok((f0, f1))
}

// TODO(https://github.com/kaist-cp/rv6/issues/365): use `Drop` instead of `close`
// TODO(https://github.com/kaist-cp/rv6/issues/365): use `self` instead of `&mut self`
// `&mut self` is used because `Drop` of `File` uses AllocatedPipe inside File.
// https://github.com/kaist-cp/rv6/pull/211#discussion_r491671723
pub unsafe fn close(&mut self, writable: bool) {
if unsafe { (*self.ptr).close(writable) } {
kernel().free(unsafe { Page::from_usize(self.ptr as *mut Pipe as _) });
pub fn close(self, writable: bool) {
if self.deref().close(writable) {
// If `Pipe::close()` returned true, this means all `AllocatedPipe`s were closed.
// Hence, we can free the `Pipe`.
// Also, the following is safe since `ptr` holds a `Pipe` stored in a valid page allocated from `kernel().alloc()`.
kernel().free(unsafe { Page::from_usize(self.ptr.as_ptr() as _) });
}
}
}
Expand All @@ -174,13 +186,26 @@ pub enum PipeError {
}

impl PipeInner {
unsafe fn try_write(&mut self, addr: UVAddr, n: usize) -> Result<usize, PipeError> {
/// Tries to write up to `n` bytes.
/// If the process was killed, returns `Err(InvalidStatus)`.
/// If an copy-in error happened after successfully writing i >= 0 bytes, returns `Err(InvalidCopyIn(i))`.
/// Otherwise, returns `Ok(i)` after successfully writing i >= 0 bytes.
fn try_write(&mut self, addr: UVAddr, n: usize) -> Result<usize, PipeError> {
let mut ch = [0u8];
let proc = unsafe { myproc() };
if !self.readopen || unsafe { (*proc).killed() } {
let proc = unsafe {
// TODO(https://github.com/kaist-cp/rv6/issues/354)
// Remove this unsafe part after resolving #354.
&*myproc()
};
if !self.readopen || proc.killed() {
return Err(PipeError::InvalidStatus);
}
let data = unsafe { &mut *(*proc).data.get() };

let data = unsafe {
// TODO(https://github.com/kaist-cp/rv6/issues/354)
// Remove this unsafe part after resolving #354.
&mut *proc.data.get()
};
for i in 0..n {
if self.nwrite == self.nread.wrapping_add(PIPESIZE as u32) {
//DOC: pipewrite-full
Expand All @@ -195,18 +220,29 @@ impl PipeInner {
Ok(n)
}

unsafe fn try_read(&mut self, addr: UVAddr, n: usize) -> Result<usize, PipeError> {
let proc = unsafe { myproc() };
let data = unsafe { &mut *(*proc).data.get() };

/// Tries to read up to `n` bytes.
/// If successful read i > 0 bytes, returns `Ok(i: usize)`.
/// If the pipe was empty, returns `Err(WaitForIO)`.
/// If the process was killed, returns `Err(InvalidStatus)`.
fn try_read(&mut self, addr: UVAddr, n: usize) -> Result<usize, PipeError> {
let proc = unsafe {
// TODO(https://github.com/kaist-cp/rv6/issues/354)
// Remove this unsafe part after resolving #354.
&*myproc()
};
//DOC: pipe-empty
if self.nread == self.nwrite && self.writeopen {
if unsafe { (*proc).killed() } {
if proc.killed() {
return Err(PipeError::InvalidStatus);
}
return Err(PipeError::WaitForIO);
}

let data = unsafe {
// TODO(https://github.com/kaist-cp/rv6/issues/354)
// Remove this unsafe part after resolving #354.
&mut *proc.data.get()
};
//DOC: piperead-copy
for i in 0..n {
if self.nread == self.nwrite {
Expand Down

0 comments on commit 7bf39e7

Please sign in to comment.