From 87b3bef06c13847a971aefd811a46dffa89adf46 Mon Sep 17 00:00:00 2001 From: Jeremy Soller Date: Wed, 30 Mar 2022 14:54:49 +0000 Subject: [PATCH] Revert "Merge branch 'clone_grant_using_fmap' into 'master'" This reverts merge request !190 --- src/context/file.rs | 2 +- src/context/memory.rs | 22 ++++++++------------- src/scheme/user.rs | 7 +++---- src/syscall/fs.rs | 18 +++++++++++------ src/syscall/process.rs | 45 ++++++++++++++---------------------------- 5 files changed, 39 insertions(+), 55 deletions(-) diff --git a/src/context/file.rs b/src/context/file.rs index 86ebf25..a274ef7 100644 --- a/src/context/file.rs +++ b/src/context/file.rs @@ -7,7 +7,7 @@ use crate::scheme::{self, SchemeNamespace, SchemeId}; use crate::syscall::error::{Result, Error, EBADF}; /// A file description -#[derive(Clone, Copy, Debug)] +#[derive(Debug)] pub struct FileDescription { /// The namespace the file was opened from (used for debugging) pub namespace: SchemeNamespace, diff --git a/src/context/memory.rs b/src/context/memory.rs index 2174e25..cfac011 100644 --- a/src/context/memory.rs +++ b/src/context/memory.rs @@ -290,15 +290,7 @@ pub struct Grant { mapped: bool, owned: bool, //TODO: This is probably a very heavy way to keep track of fmap'd files, perhaps move to the context? - pub desc_opt: Option, -} -#[derive(Clone, Debug)] -pub struct GrantFileRef { - pub desc: FileDescriptor, - pub offset: usize, - // TODO: Can the flags maybe be stored together with the page flags. Should some flags be kept, - // and others discarded when re-fmapping on clone? - pub flags: MapFlags, + pub desc_opt: Option, } impl Grant { @@ -371,7 +363,7 @@ impl Grant { } } - pub fn map_inactive(src: VirtualAddress, dst: VirtualAddress, size: usize, flags: PageFlags, desc_opt: Option, inactive_table: &mut InactivePageTable) -> Grant { + pub fn map_inactive(src: VirtualAddress, dst: VirtualAddress, size: usize, flags: PageFlags, desc_opt: Option, inactive_table: &mut InactivePageTable) -> Grant { let active_table = unsafe { ActivePageTable::new(src.kind()) }; let mut inactive_mapper = inactive_table.mapper(); @@ -515,9 +507,10 @@ impl Grant { flush_all.flush(); - if let Some(file_ref) = self.desc_opt.take() { + if let Some(desc) = self.desc_opt.take() { + println!("Grant::unmap: close desc {:?}", desc); //TODO: This imposes a large cost on unmapping, but that cost cannot be avoided without modifying fmap and funmap - let _ = file_ref.desc.close(); + let _ = desc.close(); } self.mapped = false; @@ -540,9 +533,10 @@ impl Grant { ipi(IpiKind::Tlb, IpiTarget::Other); - if let Some(file_ref) = self.desc_opt.take() { + if let Some(desc) = self.desc_opt.take() { + println!("Grant::unmap_inactive: close desc {:?}", desc); //TODO: This imposes a large cost on unmapping, but that cost cannot be avoided without modifying fmap and funmap - let _ = file_ref.desc.close(); + let _ = desc.close(); } self.mapped = false; diff --git a/src/scheme/user.rs b/src/scheme/user.rs index 5d7f9c6..6035bd5 100644 --- a/src/scheme/user.rs +++ b/src/scheme/user.rs @@ -8,7 +8,7 @@ use spin::{Mutex, RwLock}; use crate::context::{self, Context}; use crate::context::file::FileDescriptor; -use crate::context::memory::{DANGLING, page_flags, round_down_pages, Grant, Region, GrantFileRef}; +use crate::context::memory::{DANGLING, page_flags, round_down_pages, Grant, Region}; use crate::event; use crate::paging::{PAGE_SIZE, InactivePageTable, VirtualAddress}; use crate::scheme::{AtomicSchemeId, SchemeId}; @@ -123,7 +123,7 @@ impl UserInner { ).map(|addr| addr.data()) } - fn capture_inner(context_weak: &Weak>, dst_address: usize, address: usize, size: usize, flags: MapFlags, desc_opt: Option) + fn capture_inner(context_weak: &Weak>, dst_address: usize, address: usize, size: usize, flags: MapFlags, desc_opt: Option) -> Result { // TODO: More abstractions over grant creation! @@ -233,8 +233,7 @@ impl UserInner { if address % PAGE_SIZE > 0 { println!("scheme returned unaligned address, causing extra frame to be allocated"); } - let file_ref = GrantFileRef { desc, offset: map.offset, flags: map.flags }; - let res = UserInner::capture_inner(&context_weak, map.address, address, map.size, map.flags, Some(file_ref)); + let res = UserInner::capture_inner(&context_weak, map.address, address, map.size, map.flags, Some(desc)); if let Ok(grant_address) = res { if let Some(context_lock) = context_weak.upgrade() { let context = context_lock.read(); diff --git a/src/syscall/fs.rs b/src/syscall/fs.rs index 642a80a..7052c0f 100644 --- a/src/syscall/fs.rs +++ b/src/syscall/fs.rs @@ -489,8 +489,11 @@ pub fn funmap_old(virtual_address: usize) -> Result { } } - if let Some(file_ref) = desc_opt { - let scheme_id = { file_ref.desc.description.read().scheme }; + if let Some(desc) = desc_opt { + let scheme_id = { + let description = desc.description.read(); + description.scheme + }; let scheme = { let schemes = scheme::schemes(); @@ -499,7 +502,7 @@ pub fn funmap_old(virtual_address: usize) -> Result { }; let res = scheme.funmap_old(virtual_address); - let _ = file_ref.desc.close(); + let _ = desc.close(); res } else { @@ -552,8 +555,11 @@ pub fn funmap(virtual_address: usize, length: usize) -> Result { } } - for (file_ref, intersection) in notify_files { - let scheme_id = { file_ref.desc.description.read().scheme }; + for (desc, intersection) in notify_files { + let scheme_id = { + let description = desc.description.read(); + description.scheme + }; let scheme = { let schemes = scheme::schemes(); @@ -562,7 +568,7 @@ pub fn funmap(virtual_address: usize, length: usize) -> Result { }; let res = scheme.funmap(intersection.start_address().data(), intersection.size()); - let _ = file_ref.desc.close(); + let _ = desc.close(); res?; } diff --git a/src/syscall/process.rs b/src/syscall/process.rs index fd0d055..3ef08d6 100644 --- a/src/syscall/process.rs +++ b/src/syscall/process.rs @@ -10,9 +10,9 @@ use core::ops::DerefMut; use core::{intrinsics, mem, str}; use spin::RwLock; -use crate::context::file::{FileDescription, FileDescriptor}; +use crate::context::file::FileDescriptor; use crate::context::{ContextId, WaitpidKey}; -use crate::context::memory::{Grant, UserGrants, Region}; +use crate::context::memory::{UserGrants, Region}; use crate::context; #[cfg(not(feature="doc"))] use crate::elf::{self, program_header}; @@ -267,38 +267,23 @@ pub fn clone(flags: CloneFlags, stack_base: usize) -> Result { // If not cloning virtual memory, use fmap to re-obtain every grant where possible if !flags.contains(CLONE_VM) { let mut grants = grants.write(); - let old_grants = mem::take(&mut grants.inner); - // TODO: Find some way to do this without having to allocate. + let mut to_remove = BTreeSet::new(); - // TODO: Check that the current process is not allowed to serve any scheme this logic - // could interfere with. Deadlocks would otherwise seem inevitable. + // TODO: Use drain_filter if possible - for mut grant in old_grants.into_iter() { - let address = grant.start_address().data(); - let size = grant.size(); - - if let Some(ref mut file_ref) = grant.desc_opt { - - let FileDescription { scheme, number, .. } = { *file_ref.desc.description.read() }; - let scheme_arc = match crate::scheme::schemes().get(scheme) { - Some(s) => Arc::clone(s), - None => continue, - }; - let map = crate::syscall::data::Map { - address, - size, - offset: file_ref.offset, - flags: file_ref.flags, - }; - grant.unmap(); - match scheme_arc.fmap(number, &map) { - Ok(_) => (), - Err(_) => continue, - }; - } else { - grants.insert(grant); + for grant in grants.iter() { + let remove = false; + if let Some(ref _desc) = grant.desc_opt { + println!("todo: clone grant using fmap: {:?}", grant); } + if remove { + to_remove.insert(Region::from(grant)); + } + } + + for region in to_remove { + grants.remove(®ion); } }