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

Multipage Deallocator #19

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
89 changes: 80 additions & 9 deletions fs/hayleyfs/balloc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -261,23 +261,48 @@ impl PageAllocator for Option<PerCpuPageAllocator> {
fn dealloc_data_page_list(&self, pages: &DataPageListWrapper<Clean, Free>) -> Result<()> {
if let Some(allocator) = self {
let mut page_list = pages.get_page_list_cursor();
let mut page = page_list.current();

// RBTree to store the free list for every cpu
let mut cpu_free_list_map : RBTree<usize, Vec<PageNum>> = RBTree::new();

let mut page = page_list.current(); // head of page list

// get a list of pages #s for each cpu
while page.is_some() {
// janky syntax to deal with the fact that page_list.current() returns an Option
if let Some(page) = page {
// TODO: refactor to avoid acquiring lock on every iteration
allocator.dealloc_page(page.get_page_no())?;
// pr_info!("Page: {}", page.get_page_no());

let cpu : usize = allocator.pageno2cpuid(page.get_page_no())?;

if cpu_free_list_map.get(&cpu).is_none() {
cpu_free_list_map.try_insert(cpu, Vec::new())?;
}

// add cpu page to vector (vector is mutable)
let cpu_page_vec_option : Option<&mut Vec<PageNum>> = cpu_free_list_map.get_mut(&cpu);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cleaner (and potentially more efficient) to take out the if statement that checks if cpu is in cpu_free_list_map; instead, you can obtain cpu_page_vec_option as you currently do, and then rather than returning an error if cpu_page_vec_option is None below, create a new vector with the page number and insert it into the map.

Also -- the convention here would be to just name the vector cpu_page_vec; it doesn't have to reflect the fact that it's an Option.


if cpu_page_vec_option.is_some() {
let cpu_page_vec : &mut Vec<PageNum> = cpu_page_vec_option.unwrap(); // safe unwrap
cpu_page_vec.try_push(page.get_page_no())?;
} else {
pr_info!("CPU not added to RBTree\n");
return Err(EINVAL);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another Rust idiom -- you can structure this code as follows:

if let Some(cpu_page_vec) = cpu_page_vec_option() {
    cpu_page_vec.try_push(page.get_page_no())?;
} else {
    . . . 
}

It's a bit more concise. If you rename cpu_page_vec_option to cpu_page_vec, Rust will also let you write the if condition like this:

if let Some(cpu_page_vec) = cpu_page _vec 

Within the if case, cpu_page_vec will refer to the vector itself; outside of that, the variable name will still refer to the Option<Vec<>>.


page_list.move_next();
} else {
unreachable!()
}
page = page_list.current();
page = page_list.current();
}

// pr_info!("Num Pages in List: {}", num_pages);

allocator.dealloc_multiple_page(cpu_free_list_map)?;
Ok(())

} else {
pr_info!("ERROR: page allocator is uninitialized\n");
Err(EINVAL)
}
}
}

fn dealloc_dir_page<'a>(&self, page: &DirPageWrapper<'a, Clean, Dealloc>) -> Result<()> {
Expand Down Expand Up @@ -343,6 +368,52 @@ impl PerCpuPageAllocator {
Ok(())
}
}


fn dealloc_multiple_page(&self, cpu_free_list_map : RBTree<usize, Vec<PageNum>>) -> Result<()> {
// add pages to corresponding free list in ascending cpu # order
for (cpu, page_nos) in cpu_free_list_map.iter() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure -- have you confirmed/checked that this always iterates over the CPU keys in ascending order?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've confirmed through dmesg that this iterates over CPU keys in ascending order (I think the RB tree default is ascending), but I don't know how to formally test it, so I'll remove the comment.


let free_list = Arc::clone(&self.free_lists[*cpu]);
let mut free_list = free_list.lock();

for page_no in page_nos.iter() {

free_list.free_pages += 1;
let res = free_list.list.try_insert(*page_no, ());

// unwrap the error so we can get at the option
let res = match res {
Ok(res) => res,
Err(e) => {
pr_info!(
"ERROR: failed to insert {:?} into page allocator at CPU {:?}, error {:?}\n",
page_no,
cpu,
e
);
return Err(e);
}
};

if res.is_some() {
pr_info!(
"ERROR: page {:?} was already in the allocator at CPU {:?}\n",
page_no,
cpu
);
return Err(EINVAL);
}
}
}
Ok(())
}

fn pageno2cpuid(&self, page_no : PageNum) -> Result<usize> {
let cpu: usize = ((page_no - self.start) / self.pages_per_cpu).try_into()?;
Ok(cpu)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there not already a function for this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't found one that does this in balloc.rs. I noticed in dealloc_page the same formula is used to calculate the cpu.


}

// placeholder page descriptor that can represent either a dir or data page descriptor
Expand Down Expand Up @@ -2365,4 +2436,4 @@ impl<Op> DataPageListWrapper<InFlight, Op> {
pages: self.pages,
}
}
}
}
12 changes: 12 additions & 0 deletions reconfig.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#!/bin/bash
sudo umount /dev/pmem0
sudo rmmod hayleyfs
make LLVM=-14 fs/hayleyfs/hayleyfs.ko
sudo insmod fs/hayleyfs/hayleyfs.ko
sudo mount -o init -t hayleyfs /dev/pmem0 /mnt/pmem
touch test.txt
echo "Hello World" > test.txt
sudo mv test.txt /mnt/pmem
rm test.txt
cd /mnt/pmem
pwd
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you written any tests beyond this? I think it would be good to add (in a tests/ directory at the top level) more cases, e.g. larger files, checking that everything works properly when there are multiple files in the system, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added two tests - one testing a large file and one testing a series of small files.