Skip to content

Commit

Permalink
[storm/mailbox] Add reader_thread_id field
Browse files Browse the repository at this point in the history
This makes it easier to debug scenarios where multiple thread are simultaneously attempting to read from a mailbox, which is a scenario that is unsupported.

Also includes some other minor kernel improvements.
  • Loading branch information
perlun committed Oct 13, 2018
1 parent 0c6040a commit 60ec03f
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 18 deletions.
2 changes: 1 addition & 1 deletion storm/generic/debug.c
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ static int print_simple(const char *string)
// Handle the NULL string.
if (string == NULL)
{
debug_print_simple("(null)");
print_simple("(null)");

return 0;
}
Expand Down
30 changes: 22 additions & 8 deletions storm/generic/mailbox.c
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ return_type mailbox_send(mailbox_id_type mailbox_id, message_parameter_type *mes
if (mailbox == NULL)
{
mutex_kernel_signal(&tss_tree_mutex);
DEBUG_SDB(DEBUG, "mailbox == NULL.");
DEBUG_SDB(DEBUG, "mailbox %u could not be found", mailbox_id);

return STORM_RETURN_MAILBOX_UNAVAILABLE;
}
Expand Down Expand Up @@ -387,10 +387,13 @@ return_type mailbox_receive(mailbox_id_type mailbox_id,
return STORM_RETURN_MAILBOX_UNAVAILABLE;
}

// We would *like* to add current_thread_id == mailbox->owner_thread_id)
// to the check below, but the IPC library currently passes the reply
// mailbox ID from the service client to the process that is
// listening for connections on the service, making that check
// impossible for now.
if (!(current_process_id == mailbox->owner_process_id &&
current_cluster_id == mailbox->owner_cluster_id /* &&
current_thread_id == mailbox->owner_thread_id*/
))
current_cluster_id == mailbox->owner_cluster_id))
{
// We don't have read-access to this mailbox, since we are not the owner.
DEBUG_MESSAGE(DEBUG, "Access denied for process/thread %u/%u (should have been %u/%u) mailbox ID %u",
Expand All @@ -409,26 +412,37 @@ return_type mailbox_receive(mailbox_id_type mailbox_id,
{
if (message_parameter->block)
{
// This _should_ never happen, but we've seen it in cases where multiple threads are (incorrectly) reading from
// the same mailbox => chaos ensues.
assert(!mailbox->reader_blocked, "Attempting to block on mailbox_receive for mailbox ID %d, but mailbox is already blocked by thread %d", mailbox_id, mailbox->reader_thread_id);

// Block ourselves until the mailbox gets populated.
mailbox->reader_blocked = TRUE;
mailbox->reader_thread_id = current_tss->thread_id;

DEBUG_MESSAGE(VERBOSE_DEBUG, "Blocking ourselves.");

mutex_kernel_signal(&tss_tree_mutex);

// Modify this task's entry in the TSS structure.
mutex_kernel_wait(&tss_tree_mutex);
current_tss->state = STATE_MAILBOX_RECEIVE;
current_tss->mailbox_id = mailbox_id;
current_tss->mutex_time = timeslice;
mutex_kernel_signal(&tss_tree_mutex);

// Pass on control to the task switcher, since we are now
// blocked on MAILBOX_RECEIVE.
dispatch_next();

mutex_kernel_wait(&tss_tree_mutex);
mailbox->reader_blocked = FALSE;
mailbox->reader_thread_id = PROCESS_ID_NONE;

assert(mailbox->number_of_messages != 0,
"Was unblocked but number_of_messages == 0 in mailbox %d", mailbox_id)
assert(mailbox->first_message != NULL,
"Was unblocked but first_message == NULL in mailbox %d", mailbox_id)

// A message has arrived. We open the mailbox again, so that we can read out the message.
// A message has arrived. We open the mailbox again, so that
// we can read out the message.
DEBUG_MESSAGE(VERBOSE_DEBUG,
"mailbox_id = %u, mailbox->messages = %u, mailbox->first_message = %x",
mailbox_id, mailbox->number_of_messages,
Expand Down
4 changes: 2 additions & 2 deletions storm/include/storm/generic/debug.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,10 @@ extern void debug_run(void);
debug_run(); \
while (TRUE) ;

#define assert(condition, message) \
#define assert(condition, message, ...) \
if (!(condition)) \
{ \
DEBUG_HALT(message); \
DEBUG_HALT(message, ## __VA_ARGS__); \
}

#else // OPTION_RELEASE
Expand Down
6 changes: 5 additions & 1 deletion storm/include/storm/generic/mailbox.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,13 @@ typedef struct
// Number of messages in the mailbox.
unsigned int number_of_messages;

// Is the owner of this mailbox blocked on reading?
// Is a thread blocked on reading from this mailbox?
bool reader_blocked;

// The thread ID of the blocked reader, or THREAD_NONE if no thread
// blocked on this mailbox.
thread_id_type reader_thread_id;

// Start of the first message in the mailbox.
message_type *first_message;
message_type *last_message;
Expand Down
4 changes: 1 addition & 3 deletions storm/include/storm/generic/thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@
// Authors: Henrik Hallin <[email protected]>
// Per Lundberg <[email protected]>
//
// © Copyright 1999-2000 chaos development
// © Copyright 2013 chaos development
// © Copyright 2015-2016 chaos development
// © Copyright 1999 chaos development

#pragma once

Expand Down
2 changes: 0 additions & 2 deletions storm/include/storm/x86/thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// Author: Per Lundberg <[email protected]>
//
// © Copyright 2000 chaos development
// © Copyright 2013 chaos development
// © Copyright 2015-2016 chaos development

#pragma once

Expand Down
2 changes: 1 addition & 1 deletion storm/x86/memory_physical.c
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ return_type memory_physical_allocate(uint32_t *page, unsigned int length, const
else
{
debug_print("%u/%u pages allocated.\n", physical_pages - pages_left, physical_pages);
DEBUG_HALT("Failed to allocate a page.");
DEBUG_HALT("Failed to allocate %d pages - couldn't find a node with enough free pages left.", length);
}
}

Expand Down
5 changes: 5 additions & 0 deletions storm/x86/memory_virtual.c
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ page_directory_entry_page_table *kernel_page_directory;
page_directory_entry_page_table *process_page_directory = (page_directory_entry_page_table *) BASE_PROCESS_PAGE_DIRECTORY;
page_table_entry *shared_page_tables;

// Convenience variable which can be used to access the paging structures from a process' own
// virtual memory context. Not currently used by the kernel, but could be utilized if we want
// to simplify after-thread-creation mapping of memory etc.
page_table_entry *process_page_tables_flat = (page_table_entry *) BASE_PROCESS_PAGE_TABLES;

// FIXME: We should have one mutex per cluster/thread/process, or whatever seems best.

//mutex_kernel_type memory_map_mutex = MUTEX_UNLOCKED;
Expand Down

0 comments on commit 60ec03f

Please sign in to comment.