Imperial-MEng/pintos_22
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge branch 'task2/refactoring/saleh' into 'master'

Browse Source 
Refactor System Calls and Process for Readability; Change FD to local counter and use it as hash

See merge request lab2425_autumn/pintos_22!47
This commit is contained in:
Gleb Koval
2024-11-15 16:41:12 +00:00
parent 0e50603eef ea3b3594ea
commit 222aeff90f
6 changed files with 130 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
src/threads/thread.c
View File

@@ -688,6 +688,7 @@ init_thread (struct thread *t, const char *name, int nice, int priority,
t->recent_cpu = recent_cpu;
t->priority = t->base_priority;
t->fd_counter = MINIMUM_USER_FD;
t->exit_status = -1;
list_init (&t->child_results);

7
src/threads/thread.h
View File

@@ -32,6 +32,9 @@ typedef int tid_t;
#define NICE_DEFAULT 0 /* Default niceness. */
#define NICE_MAX 20 /* Highest niceness. */
/* File Descriptors. */
#define MINIMUM_USER_FD 2 /* Minimum file descriptor for user programs. */
/* A process result, synchronised between parent and child. */
struct process_result
{
@@ -137,7 +140,9 @@ struct thread
#ifdef USERPROG
/* Owned by userprog/process.c. */
uint32_t *pagedir; /* Page directory. */
struct hash open_files; /* Hash Table of FD -> Struct File */
unsigned int fd_counter; /* File descriptor counter for thread's
open files. */
struct hash open_files; /* Hash Table of FD -> Struct File. */
#endif
/* Owned by thread.c. */

66
src/userprog/process.c
View File

@@ -84,9 +84,9 @@ process_execute (const char *cmd)
/* Validates that the current file to be executed can be opened/exists. */
lock_acquire (&filesys_lock);
bool valid_file = filesys_open (file_name) != NULL;
struct file *file = filesys_open (file_name);
lock_release (&filesys_lock);
if (!valid_file)
if (file == NULL)
return TID_ERROR;
/* Create a new thread to execute the command, by initializing
@@ -128,7 +128,6 @@ static void
start_process (void *proc_start_data)
{
struct intr_frame if_;
struct process_start_data *data = proc_start_data;
/* Initialize interrupt frame and load executable. */
@@ -137,40 +136,45 @@ start_process (void *proc_start_data)
if_.cs = SEL_UCSEG;
if_.eflags = FLAG_IF | FLAG_MBS;
/* Acquire the file system lock to prevent race conditions. */
lock_acquire (&filesys_lock);
/* Prevent writing to the file being executed. */
struct file *exec_file = filesys_open (data->file_name);
if (exec_file == NULL)
{
/* If the executable file cannot be opened, free resources and quit. */
lock_release (&filesys_lock);
sema_up (&data->loaded);
thread_exit ();
}
/* Deny write to the executable file to prevent writing to it and release the
file system lock. */
file_deny_write (exec_file);
lock_release (&filesys_lock);
thread_current ()->exec_file = exec_file;
/* Load the ELF executable file, and store the success of the operation in
the 'success' variable in data. */
data->success = load (data->file_name, &if_.eip, &if_.esp);
/* If load failed, quit. */
if (!data->success)
/* If load was sucessful, initialize user process stack and free page used
to store the command that executed the process. */
if (data->success)
{
sema_up (&data->loaded);
thread_exit ();
data->success =
process_init_stack (data->cmd_saveptr, &if_.esp, data->file_name);
}
/* Initialize user process stack and free page used to store the
command that executed the process. */
data->success = process_init_stack (data->cmd_saveptr, &if_.esp, data->file_name);
/* Signal that the process has finished attempting to load. */
bool success = data->success;
sema_up (&data->loaded);
/* If stack initialization failed, free resources and quit. */
/* If the load was unsuccessful or if it was but the stack initialization
failed, exit the thread. */
if (!success)
{
thread_exit ();
}
/* Start the user process by simulating a return from an
interrupt, implemented by intr_exit (in
@@ -187,6 +191,10 @@ start_process (void *proc_start_data)
static bool
process_init_stack (char *cmd_saveptr, void **esp, char *file_name)
{
ASSERT (cmd_saveptr != NULL);
ASSERT (esp != NULL);
ASSERT (file_name != NULL);
/* Load command line argument *data* to user process stack.
This can't cause overflow due to enforcing that the size of
command line input must fit in a page. Also keep track
@@ -198,8 +206,12 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name)
int arg_count = 0;
while (arg != NULL)
{
/* filename has already been validated to be a safe-to-access string,
so we can safely use strlen here. Filename has already been
split from the command line arguments. */
push_to_stack (esp, arg, (strlen (arg) + 1) * sizeof (char));
/* Try to allocate memory for the argument pointer. */
struct arg_elem *arg_elem = malloc (sizeof (struct arg_elem));
if (arg_elem == NULL)
{
@@ -208,9 +220,11 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name)
return false;
}
/* Store the argument pointer in the linked list. */
arg_elem->arg = *esp;
list_push_front (&arg_list, &arg_elem->elem);
/* Increment the argument count and get the next argument. */
arg_count++;
arg = strtok_r (NULL, " ", &cmd_saveptr);
}
@@ -231,7 +245,10 @@ process_init_stack (char *cmd_saveptr, void **esp, char *file_name)
int overflow_bytes = (PHYS_BASE - *esp) + remaining_size - PGSIZE;
if (overflow_bytes > 0)
{
/* Calculate the number of pages needed to allocate. */
int pages_needed = DIV_CEIL (overflow_bytes, PGSIZE);
/* Allocate the pages and map them to the user process. */
for (int i = 1; i < pages_needed + 1; i++)
{
uint8_t *kpage = palloc_get_page (PAL_USER | PAL_ZERO);
@@ -296,11 +313,12 @@ push_to_stack (void **esp, void *data, size_t data_size)
* This function will be implemented in task 2.
* For now, it does nothing. */
int
process_wait (tid_t child_tid UNUSED)
process_wait (tid_t child_tid)
{
struct process_result *child_result = NULL;
struct list_elem *e;
struct thread *cur = thread_current ();
for (e = list_begin (&cur->child_results);
e != list_end (&cur->child_results); e = list_next (e))
{
@@ -308,27 +326,40 @@ process_wait (tid_t child_tid UNUSED)
= list_entry (e, struct process_result, elem);
if (result->tid == child_tid)
{
/* Found the child process. */
child_result = result;
break;
}
/* List is ordered, allowing us to break early. */
/* List is ordered, allowing us to break early if the child_tid is
greater than the current result's tid. */
else if (result->tid > child_tid)
break;
}
/* If the child process was not found, return -1. */
if (child_result == NULL)
return -1;
/* Wait for child to die. */
sema_down (&child_result->sema);
/* We need lock release in process_exit, so we need to acquire (and possibly
wait) for it here to ensure we don't free the lock memory before it is
released in process_exit. */
lock_acquire (&child_result->lock);
/* To prevent waiting for child twice, remove it from the list.
No need to use lock since this is the only thread with access to
the struct process_result now. */
list_remove (&child_result->elem);
/* Get the exit status of the child */
int exit_status = child_result->exit_status;
/* Release the lock */
lock_release (&child_result->lock);
free (child_result);
return exit_status;
}
@@ -345,9 +376,10 @@ process_exit (void)
/* Clean up all open files */
hash_destroy (&cur->open_files, fd_cleanup);
/* Close the executable file. */
/* Close the executable file, implicitly allowing it to be written to. */
if (cur->exec_file != NULL)
{
/* Acquire the file system lock to prevent race conditions. */
lock_acquire (&filesys_lock);
file_close (cur->exec_file);
lock_release (&filesys_lock);

63
src/userprog/syscall.c
View File

@@ -16,8 +16,6 @@
#define MAX_SYSCALL_ARGS 3
#define EXIT_FAILURE -1
static unsigned fd_counter = MIN_USER_FD;
struct open_file
{
int fd; /* File Descriptor / Identifier */
@@ -142,6 +140,7 @@ syscall_exit (int status)
static pid_t
syscall_exec (const char *cmd_line)
{
/* Validate the user string before executing the process. */
validate_user_string (cmd_line);
return process_execute (cmd_line); /* Returns the PID of the new process */
@@ -161,12 +160,15 @@ syscall_wait (pid_t pid)
static bool
syscall_create (const char *file, unsigned initial_size)
{
/* Validate the user string before creating the file. */
validate_user_string (file);
/* Acquire the file system lock to prevent race conditions. */
lock_acquire (&filesys_lock);
bool status = filesys_create (file, initial_size);
lock_release (&filesys_lock);
/* Return the status of the file creation. */
return status;
}
@@ -176,12 +178,15 @@ syscall_create (const char *file, unsigned initial_size)
static bool
syscall_remove (const char *file)
{
/* Validate the user string before removing the file. */
validate_user_string (file);
/* Acquire the file system lock to prevent race conditions. */
lock_acquire (&filesys_lock);
bool status = filesys_remove (file);
lock_release (&filesys_lock);
/* Return the status of the file removal. */
return status;
}
@@ -192,11 +197,15 @@ syscall_remove (const char *file)
static int
syscall_open (const char *file)
{
/* Validate the user string before opening the file. */
validate_user_string (file);
/* Acquire the file system lock to prevent race conditions. */
lock_acquire (&filesys_lock);
struct file *ptr = filesys_open (file);
lock_release (&filesys_lock);
/* If the file could not be opened, return failure. */
if (ptr == NULL)
return EXIT_FAILURE;
@@ -206,12 +215,14 @@ syscall_open (const char *file)
= (struct open_file*) malloc (sizeof (struct open_file));
if (file_info == NULL)
{
/* If we could not allocate memory for the file_info struct, close the
file and return failure. */
file_close (ptr);
return EXIT_FAILURE;
}
/* Populate the above struct, with a unique FD and the current open file */
file_info->fd = fd_counter++;
file_info->fd = thread_current ()->fd_counter++;
file_info->file = ptr;
/* Add the new FD->file mapping to the hashtable for the current thread */
@@ -227,14 +238,17 @@ syscall_open (const char *file)
static int
syscall_filesize (int fd)
{
/* Try to get the file from the FD. If it does not exist, return failure. */
struct open_file *file_info = fd_get_file (fd);
if (file_info == NULL)
return EXIT_FAILURE;
/* Acquire the file system lock to prevent any race conditions. */
lock_acquire (&filesys_lock);
int bytes = file_length (file_info->file);
lock_release (&filesys_lock);
/* Return the number of bytes in the file. */
return bytes;
}
@@ -247,9 +261,10 @@ syscall_read (int fd, void *buffer, unsigned size)
{
/* Only console (fd = 0) or other files, not including STDOUT, (fd > 1) are
allowed. */
if (fd < 0 || fd == STDOUT_FILENO)
if (fd < STDIN_FILENO || fd == STDOUT_FILENO)
return EXIT_FAILURE;
/* Validate the user buffer for the provided size before reading. */
validate_user_pointer (buffer, size);
if (fd == STDIN_FILENO)
@@ -259,18 +274,24 @@ syscall_read (int fd, void *buffer, unsigned size)
for (int i = 0; i < size; i++)
write_buffer[i] = input_getc ();
/* In case of console, read is always (eventually) successful. So return
the size for the number of bytes read. */
return size;
}
else
{
/* Reading from a file. */
/* Find the file from the FD. If it does not exist, return failure. */
struct open_file *file_info = fd_get_file (fd);
if (file_info == NULL)
return EXIT_FAILURE;
/* Acquire the file system lock to prevent race-conditions. */
lock_acquire (&filesys_lock);
int bytes_written = file_read (file_info->file, buffer, size);
lock_release (&filesys_lock);
/* Return the number of bytes read. */
return bytes_written;
}
}
@@ -287,25 +308,32 @@ syscall_write (int fd, const void *buffer, unsigned size)
if (fd <= 0)
return 0;
/* Validate the user buffer for the provided size before writing. */
validate_user_pointer (buffer, size);
if (fd == STDOUT_FILENO)
{
/* Writing to the console. */
putbuf (buffer, size);
/* In case of console, write is always successful. So return the size for
the number of bytes written. */
return size;
}
else
{
/* Writing to a file. */
/* Find the file from the FD. If it does not exist, return failure. */
struct open_file *file_info = fd_get_file (fd);
if (file_info == NULL)
return 0;
/* Acquire the file system lock to prevent race conditions. */
lock_acquire (&filesys_lock);
int bytes = file_write (file_info->file, buffer, size);
lock_release (&filesys_lock);
/* Return the number of bytes written. */
return bytes;
}
}
@@ -317,9 +345,11 @@ syscall_write (int fd, const void *buffer, unsigned size)
static void
syscall_seek (int fd, unsigned position)
{
/* Find the file from the FD. If it does not exist, do nothing. */
struct open_file *file_info = fd_get_file (fd);
if (file_info != NULL)
{
/* File exists: Acquire the file system lock to prevent race conditions. */
lock_acquire (&filesys_lock);
file_seek (file_info->file, position);
lock_release (&filesys_lock);
@@ -332,14 +362,17 @@ syscall_seek (int fd, unsigned position)
static unsigned
syscall_tell (int fd)
{
/* Find the file from the FD. If it does not exist, return 0. */
struct open_file *file_info = fd_get_file (fd);
if (file_info == NULL)
return 0;
/* Acquire the file system lock to prevent race conditions. */
lock_acquire (&filesys_lock);
unsigned pos = file_tell (file_info->file);
lock_release (&filesys_lock);
/* Return the current position in the file. */
return pos;
}
@@ -349,14 +382,21 @@ syscall_tell (int fd)
static void
syscall_close (int fd)
{
/* Find the file from the FD. If it does not exist, do nothing. */
struct open_file *file_info = fd_get_file (fd);
if (file_info != NULL)
{
/* File exists */
/* First, remove the file from the hash table of open files. */
hash_delete (&thread_current ()->open_files, &file_info->elem);
/* Then, close the file, acquiring the file system lock to prevent race
conditions. */
lock_acquire (&filesys_lock);
file_close (file_info->file);
lock_release (&filesys_lock);
/* Free the memory allocated for the file_info struct. */
free (file_info);
}
}
@@ -366,7 +406,12 @@ syscall_close (int fd)
unsigned
fd_hash (const struct hash_elem *element, void *aux UNUSED)
{
return hash_int (hash_entry (element, struct open_file, elem)->fd);
/* We use the FD as the hash value. This is because the FD is incremented
sequentially and is therefore unique for each file. It positively affects
the performance of the hash table: 1. It is unique so no need to call
expensive hash functions. 2. It being sequential means that the hash table
is more likely to be weight balanced. */
return hash_entry (element, struct open_file, elem)->fd;
}
/* Comparator function for the open_file table. Compares two entries based on
@@ -421,17 +466,19 @@ fd_get_file (int fd)
static void
validate_user_pointer (const void *start, size_t size)
{
/* If the size is 0, we do not need to check anything. */
if (size == 0)
return;
const void *end = start + size - 1;
/* Check if the start and end pointers are valid user virtual addresses. */
if (start == NULL || !is_user_vaddr (start) || !is_user_vaddr (end))
syscall_exit (EXIT_FAILURE);
/* We now need to check if the entire memory block is mapped to physical
memory by the page table. */
for (const void *ptr = start; ptr <= end; ptr += PGSIZE)
for (const void *ptr = pg_round_down (start); ptr <= end; ptr += PGSIZE)
if (pagedir_get_page (thread_current ()->pagedir, ptr) == NULL)
syscall_exit (EXIT_FAILURE);
}
@@ -442,9 +489,11 @@ validate_user_pointer (const void *start, size_t size)
static void
validate_user_string (const char *str)
{
/* Check if the string pointer is a valid user virtual address. */
if (str == NULL || !is_user_vaddr (str))
syscall_exit (EXIT_FAILURE);
/* Calculate the offset of the string within the (first) page. */
size_t offset = (uintptr_t) str % PGSIZE;
/* We move page by page, checking if the page is mapped to physical memory. */
@@ -452,6 +501,8 @@ validate_user_string (const char *str)
{
void *page = pg_round_down (str);
/* If we reach addresses that are not mapped to physical memory before the
end of the string, the thread is terminated. */
if (!is_user_vaddr(page) ||
pagedir_get_page (thread_current ()->pagedir, page) == NULL)
syscall_exit (EXIT_FAILURE);

2
src/userprog/syscall.h
View File

@@ -4,8 +4,6 @@
#include <hash.h>
#include "threads/synch.h"
#define MIN_USER_FD 2
typedef int pid_t;
struct lock filesys_lock;