From 57c167d2fa0790115c49586f73cfe3538beda67c Mon Sep 17 00:00:00 2001 From: jD91mZM2 Date: Wed, 22 Jul 2020 15:09:28 +0200 Subject: [PATCH] Make grants be a BTreeSet --- src/context/context.rs | 6 +- src/context/memory.rs | 176 ++++++++++++++++++++++++++++++++++------- src/scheme/memory.rs | 26 ++---- src/scheme/user.rs | 28 ++++--- src/syscall/driver.rs | 30 ++++--- src/syscall/fs.rs | 25 +++--- src/syscall/process.rs | 50 +++++++----- syscall | 2 +- 8 files changed, 240 insertions(+), 103 deletions(-) diff --git a/src/context/context.rs b/src/context/context.rs index 0814ee6..4b15280 100644 --- a/src/context/context.rs +++ b/src/context/context.rs @@ -11,7 +11,7 @@ use crate::arch::{interrupt::InterruptStack, paging::PAGE_SIZE}; use crate::common::unique::Unique; use crate::context::arch; use crate::context::file::{FileDescriptor, FileDescription}; -use crate::context::memory::{Grant, Memory, SharedMemory, Tls}; +use crate::context::memory::{UserGrants, Memory, SharedMemory, Tls}; use crate::ipi::{ipi, IpiKind, IpiTarget}; use crate::scheme::{SchemeNamespace, FileHandle}; use crate::sync::WaitMap; @@ -230,7 +230,7 @@ pub struct Context { /// User Thread local storage pub tls: Option, /// User grants - pub grants: Arc>>, + pub grants: Arc>, /// The name of the context pub name: Arc>>, /// The current working directory @@ -289,7 +289,7 @@ impl Context { stack: None, sigstack: None, tls: None, - grants: Arc::new(Mutex::new(Vec::new())), + grants: Arc::new(Mutex::new(UserGrants::default())), name: Arc::new(Mutex::new(Vec::new().into_boxed_slice())), cwd: Arc::new(Mutex::new(Vec::new())), files: Arc::new(Mutex::new(Vec::new())), diff --git a/src/context/memory.rs b/src/context/memory.rs index 554fefc..9a3e83b 100644 --- a/src/context/memory.rs +++ b/src/context/memory.rs @@ -1,6 +1,9 @@ +use alloc::collections::{BTreeSet, VecDeque}; use alloc::sync::{Arc, Weak}; -use alloc::collections::VecDeque; +use core::borrow::Borrow; +use core::cmp::{Eq, Ordering, PartialEq, PartialOrd}; use core::intrinsics; +use core::ops::{Deref, DerefMut}; use spin::Mutex; use crate::arch::paging::PAGE_SIZE; @@ -12,10 +15,70 @@ use crate::paging::entry::EntryFlags; use crate::paging::mapper::MapperFlushAll; use crate::paging::temporary_page::TemporaryPage; -#[derive(Debug)] -pub struct Grant { +/// Round up to the nearest multiple of page size +pub fn round_pages(size: usize) -> usize { + let rounded_up = size + PAGE_SIZE - 1; + rounded_up - rounded_up % PAGE_SIZE +} + +#[derive(Debug, Default)] +pub struct UserGrants { + pub inner: BTreeSet, +} +impl Deref for UserGrants { + type Target = BTreeSet; + fn deref(&self) -> &Self::Target { + &self.inner + } +} +impl DerefMut for UserGrants { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.inner + } +} + +#[derive(Debug, Clone, Copy, PartialOrd, Ord, PartialEq, Eq)] +pub struct Region { start: VirtualAddress, size: usize, +} +impl Region { + pub fn start_address(&self) -> VirtualAddress { + self.start + } + pub unsafe fn set_start_address(&mut self, start: VirtualAddress) { + self.start = start; + } + + /// Return the exact size of the grant + pub fn size(&self) -> usize { + self.size + } + pub unsafe fn set_size(&mut self, size: usize) { + self.size = size; + } + + /// Return the size of the grant in multiples of the page size + pub fn full_size(&self) -> usize { + round_pages(self.size) + } + + /// Returns true if the address is within the grant's requested range + pub fn contains(&self, address: VirtualAddress, length: usize) -> bool { + self.start <= address && (address.get() - self.start.get() + length) <= self.size + } + /// Returns true if the address is within the grant's actual range (so, + /// rounded up to the page size) + pub fn occupies(&self, address: VirtualAddress, length: usize) -> bool { + self.start <= address && (address.get() - self.start.get() + length) <= self.full_size() + } +} + +#[derive(Debug)] +pub struct Grant { + // TODO: Use region here instead of start/end separately + region: Region, + flags: EntryFlags, mapped: bool, owned: bool, @@ -40,8 +103,10 @@ impl Grant { flush_all.flush(&mut active_table); Grant { - start: to, - size, + region: Region { + start: to, + size, + }, flags, mapped: true, owned: false, @@ -64,8 +129,10 @@ impl Grant { flush_all.flush(&mut active_table); Grant { - start: to, - size, + region: Region { + start: to, + size, + }, flags, mapped: true, owned: true, @@ -100,8 +167,10 @@ impl Grant { ipi(IpiKind::Tlb, IpiTarget::Other); Grant { - start: to, - size, + region: Region { + start: to, + size, + }, flags, mapped: true, owned: false, @@ -117,14 +186,14 @@ impl Grant { let mut flush_all = MapperFlushAll::new(); - let start_page = Page::containing_address(self.start); - let end_page = Page::containing_address(VirtualAddress::new(self.start.get() + self.size - 1)); + let start_page = Page::containing_address(self.region.start); + let end_page = Page::containing_address(VirtualAddress::new(self.region.start.get() + self.region.size - 1)); for page in Page::range_inclusive(start_page, end_page) { //TODO: One function to do both? let flags = active_table.translate_page_flags(page).expect("grant references unmapped memory"); let frame = active_table.translate_page(page).expect("grant references unmapped memory"); - let new_page = Page::containing_address(VirtualAddress::new(page.start_address().get() - self.start.get() + new_start.get())); + let new_page = Page::containing_address(VirtualAddress::new(page.start_address().get() - self.region.start.get() + new_start.get())); if self.owned { let result = active_table.map(new_page, EntryFlags::PRESENT | EntryFlags::WRITABLE | EntryFlags::NO_EXECUTE); flush_all.consume(result); @@ -138,7 +207,7 @@ impl Grant { if self.owned { unsafe { - intrinsics::copy(self.start.get() as *const u8, new_start.get() as *mut u8, self.size); + intrinsics::copy(self.region.start.get() as *const u8, new_start.get() as *mut u8, self.region.size); } let mut flush_all = MapperFlushAll::new(); @@ -147,7 +216,7 @@ impl Grant { //TODO: One function to do both? let flags = active_table.translate_page_flags(page).expect("grant references unmapped memory"); - let new_page = Page::containing_address(VirtualAddress::new(page.start_address().get() - self.start.get() + new_start.get())); + let new_page = Page::containing_address(VirtualAddress::new(page.start_address().get() - self.region.start.get() + new_start.get())); let result = active_table.remap(new_page, flags); flush_all.consume(result); } @@ -156,8 +225,10 @@ impl Grant { } Grant { - start: new_start, - size: self.size, + region: Region { + start: new_start, + size: self.region.size, + }, flags: self.flags, mapped: true, owned: self.owned, @@ -172,8 +243,8 @@ impl Grant { let mut flush_all = MapperFlushAll::new(); - let start_page = Page::containing_address(self.start); - let end_page = Page::containing_address(VirtualAddress::new(self.start.get() + self.size - 1)); + let start_page = Page::containing_address(self.region.start); + let end_page = Page::containing_address(VirtualAddress::new(self.region.start.get() + self.region.size - 1)); for page in Page::range_inclusive(start_page, end_page) { //TODO: One function to do both? let flags = active_table.translate_page_flags(page).expect("grant references unmapped memory"); @@ -181,7 +252,7 @@ impl Grant { flush_all.consume(result); active_table.with(new_table, temporary_page, |mapper| { - let new_page = Page::containing_address(VirtualAddress::new(page.start_address().get() - self.start.get() + new_start.get())); + let new_page = Page::containing_address(VirtualAddress::new(page.start_address().get() - self.region.start.get() + new_start.get())); let result = mapper.map_to(new_page, frame, flags); // Ignore result due to mapping on inactive table unsafe { result.ignore(); } @@ -190,22 +261,48 @@ impl Grant { flush_all.flush(&mut active_table); - self.start = new_start; + self.region.start = new_start; } + pub fn region(&self) -> Region { + self.region + } + + // ------------------------ + // Delegate to region + // ------------------------ + pub fn start_address(&self) -> VirtualAddress { - self.start + self.region.start } pub unsafe fn set_start_address(&mut self, start: VirtualAddress) { - self.start = start; + self.region.start = start; } + /// Return the exact size of the grant pub fn size(&self) -> usize { - self.size + self.region.size() } pub unsafe fn set_size(&mut self, size: usize) { - self.size = size; + self.region.set_size(size) } + /// Return the size of the grant in multiples of the page size + pub fn full_size(&self) -> usize { + self.region.full_size() + } + /// Returns true if the address is within the grant's requested range + pub fn contains(&self, address: VirtualAddress, length: usize) -> bool { + self.region.contains(address, length) + } + /// Returns true if the address is within the grant's actual range (so, + /// rounded up to the page size) + pub fn occupies(&self, address: VirtualAddress, length: usize) -> bool { + self.region.occupies(address, length) + } + + // ------------------------ + // End TODO + // ------------------------ pub fn flags(&self) -> EntryFlags { self.flags @@ -226,8 +323,8 @@ impl Grant { let mut flush_all = MapperFlushAll::new(); - let start_page = Page::containing_address(self.start); - let end_page = Page::containing_address(VirtualAddress::new(self.start.get() + self.size - 1)); + let start_page = Page::containing_address(self.region.start); + let end_page = Page::containing_address(VirtualAddress::new(self.region.start.get() + self.region.size - 1)); for page in Page::range_inclusive(start_page, end_page) { let (result, _frame) = active_table.unmap_return(page, false); flush_all.consume(result); @@ -253,8 +350,8 @@ impl Grant { let mut active_table = unsafe { ActivePageTable::new() }; active_table.with(new_table, temporary_page, |mapper| { - let start_page = Page::containing_address(self.start); - let end_page = Page::containing_address(VirtualAddress::new(self.start.get() + self.size - 1)); + let start_page = Page::containing_address(self.region.start); + let end_page = Page::containing_address(VirtualAddress::new(self.region.start.get() + self.region.size - 1)); for page in Page::range_inclusive(start_page, end_page) { let (result, _frame) = mapper.unmap_return(page, false); // This is not the active table, so the flush can be ignored @@ -273,6 +370,29 @@ impl Grant { } } +impl PartialOrd for Grant { + fn partial_cmp(&self, other: &Self) -> Option { + self.region.partial_cmp(&other.region) + } +} +impl Ord for Grant { + fn cmp(&self, other: &Self) -> Ordering { + self.region.cmp(&other.region) + } +} +impl PartialEq for Grant { + fn eq(&self, other: &Self) -> bool { + self.region.eq(&other.region) + } +} +impl Eq for Grant {} + +impl Borrow for Grant { + fn borrow(&self) -> &Region { + &self.region + } +} + impl Drop for Grant { fn drop(&mut self) { assert!(!self.mapped); diff --git a/src/scheme/memory.rs b/src/scheme/memory.rs index 5efb025..b6f94e6 100644 --- a/src/scheme/memory.rs +++ b/src/scheme/memory.rs @@ -1,6 +1,6 @@ use core::cmp; use crate::context; -use crate::context::memory::Grant; +use crate::context::memory::{round_pages, Grant}; use crate::memory::{free_frames, used_frames, PAGE_SIZE}; use crate::paging::{ActivePageTable, Page, VirtualAddress}; use crate::paging::entry::EntryFlags; @@ -47,7 +47,7 @@ impl Scheme for MemoryScheme { let mut grants = context.grants.lock(); - let full_size = ((map.size + PAGE_SIZE - 1) / PAGE_SIZE) * PAGE_SIZE; + let full_size = round_pages(map.size); let mut to_address = if map.address == 0 { crate::USER_GRANT_OFFSET } else { if @@ -70,16 +70,12 @@ impl Scheme for MemoryScheme { entry_flags |= EntryFlags::WRITABLE; } - let mut i = 0; - - while i < grants.len() { - let grant = &mut grants[i]; - + for grant in grants.iter() { let grant_start = grant.start_address().get(); - let grant_len = ((grant.size() + PAGE_SIZE - 1) / PAGE_SIZE) * PAGE_SIZE; + let grant_len = grant.full_size(); let grant_end = grant_start + grant_len; - if to_address < grant_start || grant_end <= to_address { + if !grant.occupies(VirtualAddress::new(to_address), full_size) { // grant has nothing to do with the memory to map, and thus we can safely just // go on to the next one. @@ -90,14 +86,9 @@ impl Scheme for MemoryScheme { to_address, ); } - i += 1; continue; - } - - // check whether this grant overlaps with the memory range to use, by checking that - // the start and end of the grant is not within the memory range to map - if grant_start <= to_address && grant_end > to_address || grant_start <= to_address + full_size && grant_end > to_address + full_size { + } else { // the range overlaps, thus we'll have to continue to the next grant, or to // insert a new grant at the end (if not MapFlags::MAP_FIXED). @@ -110,7 +101,7 @@ impl Scheme for MemoryScheme { // changed at all when mapping to a fixed address, we can just continue to // the next grant and shrink or remove that one if it was also overlapping. if to_address + full_size > grant_start { - let new_start = core::cmp::min(grant_end, to_address + full_size); + let new_start = core::cmp::min(grant_end, to_address + full_size); let new_size = grant.size() - (new_start - grant_start); unsafe { grant.set_size(new_size) }; @@ -127,7 +118,6 @@ impl Scheme for MemoryScheme { return Err(Error::new(EOPNOTSUPP)); } else { to_address = grant_end; - i += 1; } continue; } @@ -145,7 +135,7 @@ impl Scheme for MemoryScheme { } } - grants.insert(i, Grant::map(start_address, full_size, entry_flags)); + grants.insert(Grant::map(start_address, full_size, entry_flags)); Ok(to_address) } diff --git a/src/scheme/user.rs b/src/scheme/user.rs index 1c083bc..1b01d73 100644 --- a/src/scheme/user.rs +++ b/src/scheme/user.rs @@ -138,21 +138,19 @@ impl UserInner { entry_flags |= EntryFlags::WRITABLE; } - let mut i = 0; - while i < grants.len() { - let start = grants[i].start_address().get(); + for grant in grants.iter() { + let start = grant.start_address().get(); if to_address + full_size < start { break; } - let pages = (grants[i].size() + 4095) / 4096; + let pages = (grant.size() + 4095) / 4096; let end = start + pages * 4096; to_address = end; - i += 1; } //TODO: Use syscall_head and syscall_tail to avoid leaking data - grants.insert(i, Grant::map_inactive( + grants.insert(Grant::map_inactive( VirtualAddress::new(from_address), VirtualAddress::new(to_address), full_size, @@ -178,14 +176,18 @@ impl UserInner { let mut grants = context.grants.lock(); - for i in 0 .. grants.len() { - let start = grants[i].start_address().get(); - let end = start + grants[i].size(); - if address >= start && address < end { - grants.remove(i).unmap_inactive(&mut new_table, &mut temporary_page); + // TODO Implementation can now use the powers of BTreeSet - return Ok(()); - } + let grant = grants.iter().map(|grant| grant.region()).find(|grant| { + let start = grant.start_address().get(); + let end = start + grant.size(); + + address >= start && address < end + }); + + if let Some(grant) = grant { + grants.take(&grant).unwrap().unmap_inactive(&mut new_table, &mut temporary_page); + return Ok(()); } Err(Error::new(EFAULT)) diff --git a/src/syscall/driver.rs b/src/syscall/driver.rs index 6318426..af0eec1 100644 --- a/src/syscall/driver.rs +++ b/src/syscall/driver.rs @@ -93,20 +93,20 @@ pub fn inner_physmap(physical_address: usize, size: usize, flags: PhysmapFlags) entry_flags |= EntryFlags::NO_CACHE; } - let mut i = 0; - while i < grants.len() { - let start = grants[i].start_address().get(); + // TODO: Make this faster than Sonic himself by using le superpowers of BTreeSet + + for grant in grants.iter() { + let start = grant.start_address().get(); if to_address + full_size < start { break; } - let pages = (grants[i].size() + 4095) / 4096; + let pages = (grant.size() + 4095) / 4096; let end = start + pages * 4096; to_address = end; - i += 1; } - grants.insert(i, Grant::physmap( + grants.insert(Grant::physmap( PhysicalAddress::new(from_address), VirtualAddress::new(to_address), full_size, @@ -131,14 +131,18 @@ pub fn inner_physunmap(virtual_address: usize) -> Result { let mut grants = context.grants.lock(); - for i in 0 .. grants.len() { - let start = grants[i].start_address().get(); - let end = start + grants[i].size(); - if virtual_address >= start && virtual_address < end { - grants.remove(i).unmap(); + // TODO Implementation can now use the powers of BTreeSet - return Ok(0); - } + let grant = grants.iter().map(|grant| grant.region()).find(|grant| { + let start = grant.start_address().get(); + let end = start + grant.size(); + + virtual_address >= start && virtual_address < end + }); + + if let Some(grant) = grant { + grants.take(&grant).unwrap().unmap(); + return Ok(0); } Err(Error::new(EFAULT)) diff --git a/src/syscall/fs.rs b/src/syscall/fs.rs index 00276f7..fb9cc6c 100644 --- a/src/syscall/fs.rs +++ b/src/syscall/fs.rs @@ -453,15 +453,22 @@ pub fn funmap(virtual_address: usize) -> Result { let mut grants = context.grants.lock(); - for i in 0 .. grants.len() { - let start = grants[i].start_address().get(); - let end = start + grants[i].size(); - if virtual_address >= start && virtual_address < end { - let mut grant = grants.remove(i); - desc_opt = grant.desc_opt.take(); - grant.unmap(); - break; - } + // TODO: Make BTreeSet roll around at the speed of sound, + // I mean, its got places to go, gotta follow its rainbow. + // Can't keep around, gotta moving on. + // Guess what lies ahead, only one way to find oooouuuut. + + let grant = grants.iter().map(|grant| grant.region()).find(|grant| { + let start = grant.start_address().get(); + let end = start + grant.size(); + + virtual_address >= start && virtual_address < end + }); + + if let Some(grant) = grant { + let mut grant = grants.take(&grant).unwrap(); + desc_opt = grant.desc_opt.take(); + grant.unmap(); } } diff --git a/src/syscall/process.rs b/src/syscall/process.rs index cc3bce3..e469689 100644 --- a/src/syscall/process.rs +++ b/src/syscall/process.rs @@ -1,13 +1,15 @@ -use alloc::sync::Arc; use alloc::boxed::Box; +use alloc::collections::BTreeSet; +use alloc::sync::Arc; use alloc::vec::Vec; use core::alloc::{GlobalAlloc, Layout}; -use core::{intrinsics, mem}; use core::ops::DerefMut; +use core::{intrinsics, mem}; use spin::Mutex; use crate::context::file::FileDescriptor; use crate::context::{ContextId, WaitpidKey}; +use crate::context::memory::UserGrants; use crate::context; #[cfg(not(feature="doc"))] use crate::elf::{self, program_header}; @@ -280,12 +282,12 @@ pub fn clone(flags: CloneFlags, stack_base: usize) -> Result { if flags.contains(CLONE_VM) { grants = Arc::clone(&context.grants); } else { - let mut grants_vec = Vec::new(); + let mut grants_set = UserGrants::default(); for grant in context.grants.lock().iter() { let start = VirtualAddress::new(grant.start_address().get() + crate::USER_TMP_GRANT_OFFSET - crate::USER_GRANT_OFFSET); - grants_vec.push(grant.secret_clone(start)); + grants_set.insert(grant.secret_clone(start)); } - grants = Arc::new(Mutex::new(grants_vec)); + grants = Arc::new(Mutex::new(grants_set)); } if flags.contains(CLONE_VM) { @@ -332,20 +334,25 @@ 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 i = 0; - while i < grants.lock().len() { + let mut grants = grants.lock(); + + let mut to_remove = BTreeSet::new(); + + // TODO: Use drain_filter if possible + + for grant in grants.iter() { let remove = false; - if let Some(grant) = grants.lock().get(i) { - if let Some(ref _desc) = grant.desc_opt { - println!("todo: clone grant {} using fmap: {:?}", i, grant); - } + if let Some(ref _desc) = grant.desc_opt { + println!("todo: clone grant using fmap: {:?}", grant); } if remove { - grants.lock().remove(i); - } else { - i += 1; + to_remove.insert(grant.region()); } } + + for region in to_remove { + grants.remove(®ion); + } } // If vfork, block the current process @@ -510,9 +517,15 @@ pub fn clone(flags: CloneFlags, stack_base: usize) -> Result { } // Move grants - for grant in grants.lock().iter_mut() { - let start = VirtualAddress::new(grant.start_address().get() + crate::USER_GRANT_OFFSET - crate::USER_TMP_GRANT_OFFSET); - grant.move_to(start, &mut new_table, &mut temporary_page); + { + let mut grants = grants.lock(); + let old_grants = mem::replace(&mut *grants, UserGrants::default()); + + for mut grant in old_grants.inner.into_iter() { + let start = VirtualAddress::new(grant.start_address().get() + crate::USER_GRANT_OFFSET - crate::USER_TMP_GRANT_OFFSET); + grant.move_to(start, &mut new_table, &mut temporary_page); + grants.insert(grant); + } } context.grants = grants; } @@ -626,7 +639,8 @@ fn empty(context: &mut context::Context, reaping: bool) { let mut grants = context.grants.lock(); if Arc::strong_count(&context.grants) == 1 { - for grant in grants.drain(..) { + let grants = mem::replace(&mut *grants, UserGrants::default()); + for grant in grants.inner.into_iter() { if reaping { println!("{}: {}: Grant should not exist: {:?}", context.id.into(), unsafe { ::core::str::from_utf8_unchecked(&context.name.lock()) }, grant); diff --git a/syscall b/syscall index 6346fd6..c4ddf3f 160000 --- a/syscall +++ b/syscall @@ -1 +1 @@ -Subproject commit 6346fd671ef6e1650062a8cd3097c0b0d17c92cb +Subproject commit c4ddf3fff4b216364f062eca28f62a133350652a