Skip to content

Commit

Permalink
Fix Pipe API.
Browse files Browse the repository at this point in the history
  • Loading branch information
@travis1829
travis1829 committed Jan 27, 2021
1 parent 2fec36d commit 81972b3
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 52 deletions.
2 changes: 1 addition & 1 deletion kernel-rs/src/file.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,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
118 changes: 67 additions & 51 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};

const PIPESIZE: usize = 512;

Expand Down Expand Up @@ -40,8 +40,8 @@ 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, ()> {
//TODO(https://github.com/kaist-cp/rv6/issues/366) : `n` should be u32.
pub fn read(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {
let mut inner = self.inner.lock();
loop {
match inner.try_read(addr, n) {
Expand All @@ -61,7 +61,7 @@ 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, ()> {
pub fn write(&self, addr: UVAddr, n: usize) -> Result<usize, ()> {
let mut written = 0;
let mut inner = self.inner.lock();
loop {
Expand All @@ -84,7 +84,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 @@ -95,73 +95,73 @@ 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`.
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 = NonNull::new(page.into_usize() as *mut Pipe).expect("AllocatedPipe alloc");

// `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 (*self.ptr).close(writable) {
kernel().free(Page::from_usize(self.ptr as *mut Pipe as _));
pub fn close(self, writable: bool) {
unsafe {
// Safe since `ptr` holds a `Pipe` stored in a valid page allocated from `kernel().alloc()`.
if self.ptr.as_ref().close(writable) {
kernel().free(Page::from_usize(self.ptr.as_ptr() as _));
}
}
}
}
Expand All @@ -173,13 +173,22 @@ pub enum PipeError {
}

impl PipeInner {
unsafe fn try_write(&mut self, addr: UVAddr, n: usize) -> Result<usize, PipeError> {
fn try_write(&mut self, addr: UVAddr, n: usize) -> Result<usize, PipeError> {
let mut ch = [0 as u8];
let proc = myproc();
if !self.readopen || (*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 = &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 @@ -194,18 +203,25 @@ impl PipeInner {
Ok(n)
}

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

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 (*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 81972b3

Please sign in to comment.