Make interrupt work thread reliable.

kernel/clock.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2016 Jonas 'Sortie' Termansen.
+ * Copyright (c) 2013, 2016, 2017 Jonas 'Sortie' Termansen.
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -30,16 +30,26 @@
 
 namespace Sortix {
 
+static void Clock__InterruptWork(void* context)
+{
+	((Clock*) context)->InterruptWork();
+}
+
 Clock::Clock()
 {
 	delay_timer = NULL;
 	absolute_timer = NULL;
+	first_interrupt_timer = NULL;
+	last_interrupt_timer = NULL;
+	interrupt_work.handler = Clock__InterruptWork;
+	interrupt_work.context = this;
 	current_time = timespec_nul();
 	current_advancement = timespec_nul();
 	resolution = timespec_nul();
 	clock_mutex = KTHREAD_MUTEX_INITIALIZER;
 	clock_callable_from_interrupt = false;
 	we_disabled_interrupts = false;
+	interrupt_work_scheduled = false;
 }
 
 Clock::~Clock()
@@ -385,9 +395,25 @@ static void Clock__FireTimer(void* timer_ptr)
 	timer->clock->UnlockClock();
 }
 
-static void Clock__FireTimer_InterruptWorker(void* timer_ptr, void*, size_t)
+void Clock::InterruptWork()
 {
-	Clock__FireTimer(timer_ptr);
+	Interrupt::Disable();
+	Timer* work = first_interrupt_timer;
+	first_interrupt_timer = NULL;
+	last_interrupt_timer = NULL;
+	Interrupt::Enable();
+	while ( work )
+	{
+		Timer* next_work = work->next_interrupt_timer;
+		Clock__FireTimer(work);
+		work = next_work;
+	}
+	Interrupt::Disable();
+	if ( first_interrupt_timer )
+		Interrupt::ScheduleWork(&interrupt_work);
+	else
+		interrupt_work_scheduled = false;
+	Interrupt::Enable();
 }
 
 void Clock::FireTimer(Timer* timer)
@@ -409,7 +435,16 @@ void Clock::FireTimer(Timer* timer)
 		{
 			if ( !may_deallocate )
 				timer->flags |= TIMER_FIRING;
-			Interrupt::ScheduleWork(Clock__FireTimer_InterruptWorker, timer, NULL, 0);
+			(last_interrupt_timer ?
+			 last_interrupt_timer->next_interrupt_timer :
+			 first_interrupt_timer) = timer;
+			timer->next_interrupt_timer = NULL;
+			last_interrupt_timer = timer;
+			if ( !interrupt_work_scheduled )
+			{
+				Interrupt::ScheduleWork(&interrupt_work);
+				interrupt_work_scheduled = true;
+			}
 		}
 	}
 

kernel/include/sortix/kernel/clock.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2016 Jonas 'Sortie' Termansen.
+ * Copyright (c) 2013, 2016, 2017 Jonas 'Sortie' Termansen.
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -25,6 +25,7 @@
 #include <sortix/timespec.h>
 
 #include <sortix/kernel/kthread.h>
+#include <sortix/kernel/interrupt.h>
 
 namespace Sortix {
 
@@ -40,12 +41,16 @@ public:
 public:
 	Timer* delay_timer;
 	Timer* absolute_timer;
+	Timer* first_interrupt_timer;
+	Timer* last_interrupt_timer;
+	struct interrupt_work interrupt_work;
 	struct timespec current_time;
 	struct timespec current_advancement;
 	struct timespec resolution;
 	kthread_mutex_t clock_mutex;
 	bool clock_callable_from_interrupt;
 	bool we_disabled_interrupts;
+	bool interrupt_work_scheduled;
 
 public:
 	void SetCallableFromInterrupts(bool callable_from_interrupts);
@@ -72,6 +77,9 @@ private: // These should only be called if the clock is locked.
 	void TriggerDelay(struct timespec unaccounted);
 	void TriggerAbsolute();
 
+public: // Only for use by Clock__InterruptWork.
+	void InterruptWork();
+
 };
 
 } // namespace Sortix

kernel/include/sortix/kernel/interrupt.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2012, 2013, 2014 Jonas 'Sortie' Termansen.
+ * Copyright (c) 2011, 2012, 2013, 2014, 2017 Jonas 'Sortie' Termansen.
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -37,10 +37,16 @@ struct interrupt_handler
 	struct interrupt_handler* prev;
 };
 
+struct interrupt_work
+{
+	struct interrupt_work* next;
+	void (*handler)(void*);
+	void* context;
+};
+
 } // namespace Sortix
 
 namespace Sortix {
-
 namespace Interrupt {
 
 #if defined(__i386__) || defined(__x86_64__)
@@ -112,15 +118,9 @@ inline bool SetEnabled(bool is_enabled)
 
 void RegisterHandler(unsigned int index, struct interrupt_handler* handler);
 void UnregisterHandler(unsigned int index, struct interrupt_handler* handler);
-
 void Init();
-void InitWorker();
 void WorkerThread(void* user);
-
-bool ScheduleWork(void (*handler)(void*, void*, size_t),
-                  void* handler_context,
-                  void* payload,
-                  size_t payload_size);
+void ScheduleWork(struct interrupt_work* work);
 
 } // namespace Interrupt
 } // namespace Sortix

kernel/include/sortix/kernel/timer.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2016 Jonas 'Sortie' Termansen.
+ * Copyright (c) 2013, 2016, 2017 Jonas 'Sortie' Termansen.
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -69,6 +69,7 @@ public:
 	Clock* clock;
 	Timer* prev_timer;
 	Timer* next_timer;
+	Timer* next_interrupt_timer;
 	void (*callback)(Clock* clock, Timer* timer, void* user);
 	void* user;
 	size_t num_firings_scheduled;

kernel/interrupt.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2012, 2013, 2014 Jonas 'Sortie' Termansen.
+ * Copyright (c) 2011, 2012, 2013, 2014, 2016, 2017 Jonas 'Sortie' Termansen.
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -21,138 +21,48 @@
 #include <errno.h>
 #include <string.h>
 
-#include <sortix/kernel/cpu.h>
 #include <sortix/kernel/interrupt.h>
 #include <sortix/kernel/kernel.h>
-#include <sortix/kernel/process.h>
-#include <sortix/kernel/scheduler.h>
-#include <sortix/kernel/signal.h>
-#include <sortix/kernel/syscall.h>
-#include <sortix/kernel/thread.h>
-
-// TODO: The interrupt worker isn't a reliable design.
+#include <sortix/kernel/kthread.h>
 
 namespace Sortix {
 namespace Interrupt {
 
-unsigned char* queue;
-volatile size_t queue_offset;
-volatile size_t queue_used;
-const size_t QUEUE_SIZE = 4096;
-
-struct worker_package
-{
-	size_t payload_size;
-	void (*handler)(void*, void*, size_t);
-	void* handler_context;
-};
-
-void InitWorker()
-{
-	queue = new unsigned char[QUEUE_SIZE];
-	if ( !queue )
-		Panic("Can't allocate interrupt worker queue");
-	queue_offset = 0;
-	queue_used = 0;
-}
-
-static void WriteToQueue(const void* src, size_t size)
-{
-	assert(size <= QUEUE_SIZE - queue_used);
-	const unsigned char* input = (const unsigned char*) src;
-	for ( size_t i = 0; i < size; i++ )
-	{
-		size_t index = (queue_offset + queue_used + i) % QUEUE_SIZE;
-		queue[index] = input[i];
-	}
-	queue_used += size;
-}
-
-static void ReadFromQueue(void* dst, size_t size)
-{
-	assert(size <= queue_used);
-	unsigned char* output = (unsigned char*) dst;
-	for ( size_t i = 0; i < size; i++ )
-	{
-		size_t index = (queue_offset + i) % QUEUE_SIZE;
-		output[i] = queue[index];
-	}
-	queue_offset = (queue_offset + size) % QUEUE_SIZE;
-	queue_used -= size;
-}
-
-static bool PopPackage(struct worker_package* package,
-                       unsigned char* payload,
-                       size_t payload_size)
-{
-	bool interrupts_was_enabled = Interrupt::SetEnabled(false);
-	if ( !queue_used )
-	{
-		Interrupt::SetEnabled(interrupts_was_enabled);
-		return false;
-	}
-
-	ReadFromQueue(package, sizeof(*package));
-	if ( !(package->payload_size <= payload_size) )
-	{
-		Interrupt::SetEnabled(interrupts_was_enabled);
-		assert(package->payload_size <= payload_size);
-		queue_offset = (queue_offset + package->payload_size) % QUEUE_SIZE;
-		return false;
-	}
-
-	ReadFromQueue(payload, package->payload_size);
-
-	Interrupt::SetEnabled(interrupts_was_enabled);
-	return true;
-}
+static struct interrupt_work* first;
+static struct interrupt_work* last;
 
 void WorkerThread(void* /*user*/)
 {
 	assert(Interrupt::IsEnabled());
-
-	struct worker_package package;
-	size_t storage_size = QUEUE_SIZE;
-	unsigned char* storage = new unsigned char[storage_size];
-	if ( !storage )
-		Panic("Can't allocate interrupt worker storage");
 	while ( true )
 	{
-		if ( !PopPackage(&package, storage, storage_size) )
+		struct interrupt_work* work;
+		Interrupt::Disable();
+		work = first;
+		first = NULL;
+		last = NULL;
+		Interrupt::Enable();
+		if ( !work )
 		{
-			Scheduler::Yield();
+			// TODO: Make this thread not run until work arrives.
+			kthread_yield();
 			continue;
 		}
-		unsigned char* payload = storage;
-		size_t payload_size = package.payload_size;
-		assert(package.handler);
-		package.handler(package.handler_context, payload, payload_size);
+		while ( work )
+		{
+			struct interrupt_work* next_work = work->next;
+			work->handler(work->context);
+			work = next_work;
+		}
 	}
 }
 
-bool ScheduleWork(void (*handler)(void*, void*, size_t),
-                  void* handler_context,
-                  void* payload,
-                  size_t payload_size)
+void ScheduleWork(struct interrupt_work* work)
 {
 	assert(!Interrupt::IsEnabled());
-
-	assert(handler);
-	assert(payload || !payload_size);
-
-	struct worker_package package;
-	memset(&package, 0, sizeof(package));
-	package.payload_size = payload_size;
-	package.handler = handler;
-	package.handler_context = handler_context;
-
-	if ( QUEUE_SIZE - queue_used < sizeof(package) + payload_size )
-		return false;
-
-	WriteToQueue(&package, sizeof(package));
-	WriteToQueue(payload, payload_size);
-
-	return true;
+	(last ? last->next : first) = work;
+	work->next = NULL;
+	last = work;
 }
 
 } // namespace Interrupt

kernel/kernel.cpp

@@ -313,9 +313,6 @@ extern "C" void KernelInit(unsigned long magic, multiboot_info_t* bootinfo_p)
 	// Initialize the interrupt handler table and enable interrupts.
 	Interrupt::Init();
 
-	// Initialize the interrupt worker (before scheduling is enabled).
-	Interrupt::InitWorker();
-
 	// Initialize the clocks.
 	Time::Init();
 

kernel/timer.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2016 Jonas 'Sortie' Termansen.
+ * Copyright (c) 2013, 2016, 2017 Jonas 'Sortie' Termansen.
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -37,6 +37,7 @@ Timer::Timer()
 	clock = NULL;
 	prev_timer = NULL;
 	next_timer = NULL;
+	next_interrupt_timer = NULL;
 	callback = NULL;
 	user = NULL;
 	num_firings_scheduled = 0;

kernel/x86-family/ps2.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015 Jonas 'Sortie' Termansen.
+ * Copyright (c) 2015, 2017 Jonas 'Sortie' Termansen.
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -177,39 +177,87 @@ static bool IsMouseResponse(uint8_t* response, size_t size)
 	return false;
 }
 
+static void IRQ1Work(void* /*context*/);
+static void IRQ12Work(void* /*context*/);
+
 static struct interrupt_handler irq1_registration;
 static struct interrupt_handler irq12_registration;
+static struct interrupt_work irq1_work = { NULL, IRQ1Work, NULL };
+static struct interrupt_work irq12_work = { NULL, IRQ12Work, NULL };
 static PS2Device* irq1_device;
 static PS2Device* irq12_device;
+static unsigned char irq1_buffer[128];
+static unsigned char irq12_buffer[128];
+static size_t irq1_offset;
+static size_t irq12_offset;
+static size_t irq1_used;
+static size_t irq12_used;
+static bool irq1_working;
+static bool irq12_working;
 
-static void IRQ1Work(void* ctx, void* payload, size_t size)
+static void IRQ1Work(void* /*context*/)
 {
-	(void) ctx;
-	assert(size == sizeof(unsigned char));
-	unsigned char* byteptr = (unsigned char*) payload;
-	unsigned char byte = *byteptr;
-	if ( irq1_device )
-		irq1_device->PS2DeviceOnByte(byte);
+	Interrupt::Disable();
+	size_t todo = irq1_used;
+	for ( size_t i = 0; i < todo; i++ )
+	{
+		unsigned char byte = irq1_buffer[irq1_offset++];
+		if ( sizeof(irq1_buffer) <= irq1_offset )
+			irq1_offset = 0;
+		irq1_used--;
+		Interrupt::Enable();
+		if ( irq1_device )
+			irq1_device->PS2DeviceOnByte(byte);
+		Interrupt::Disable();
+	}
+	if ( irq1_used )
+		Interrupt::ScheduleWork(&irq1_work);
+	else
+		irq1_working = false;
+	Interrupt::Enable();
 }
 
-static void IRQ12Work(void* ctx, void* payload, size_t size)
+static void IRQ12Work(void* /*context*/)
 {
-	(void) ctx;
-	assert(size == sizeof(unsigned char));
-	unsigned char* byteptr = (unsigned char*) payload;
-	unsigned char byte = *byteptr;
-	if ( irq12_device )
-		irq12_device->PS2DeviceOnByte(byte);
+	Interrupt::Disable();
+	size_t todo = irq12_used;
+	for ( size_t i = 0; i < todo; i++ )
+	{
+		unsigned char byte = irq12_buffer[irq12_offset++];
+		if ( sizeof(irq12_buffer) <= irq12_offset )
+			irq12_offset = 0;
+		irq12_used--;
+		Interrupt::Enable();
+		if ( irq12_device )
+			irq12_device->PS2DeviceOnByte(byte);
+		Interrupt::Disable();
+	}
+	if ( irq12_used )
+		Interrupt::ScheduleWork(&irq12_work);
+	else
+		irq12_working = false;
+	Interrupt::Enable();
 }
 
-static void OnIRQ1(struct interrupt_context* intctx, void* user)
+static void OnIRQ1(struct interrupt_context* /*intctx*/, void* /*user*/)
 {
-	(void) intctx;
-	(void) user;
 	if ( inport8(REG_STATUS) & REG_STATUS_OUTPUT )
 	{
 		uint8_t byte = inport8(REG_DATA);
-		Interrupt::ScheduleWork(IRQ1Work, NULL, &byte, sizeof(byte));
+		// TODO: This drops incoming bytes if the buffer is full. Instead make
+		// the device not send further interrupts until we have available bytes.
+		if ( irq1_used < sizeof(irq1_buffer) )
+		{
+			size_t index = irq1_offset + irq1_used++;
+			if ( sizeof(irq1_buffer) <= index )
+				index -= sizeof(irq1_buffer);
+			irq1_buffer[index] = byte;
+			if ( !irq1_working )
+			{
+				Interrupt::ScheduleWork(&irq1_work);
+				irq1_working = true;
+			}
+		}
 	}
 }
 
@@ -220,7 +268,20 @@ static void OnIRQ12(struct interrupt_context* intctx, void* user)
 	if ( inport8(REG_STATUS) & REG_STATUS_OUTPUT )
 	{
 		uint8_t byte = inport8(REG_DATA);
-		Interrupt::ScheduleWork(IRQ12Work, NULL, &byte, sizeof(byte));
+		// TODO: This drops incoming bytes if the buffer is full. Instead make
+		// the device not send further interrupts until we have available bytes.
+		if ( irq12_used < sizeof(irq12_buffer) )
+		{
+			size_t index = irq12_offset + irq12_used++;
+			if ( sizeof(irq12_buffer) <= index )
+				index -= sizeof(irq12_buffer);
+			irq12_buffer[index] = byte;
+			if ( !irq12_working )
+			{
+				Interrupt::ScheduleWork(&irq12_work);
+				irq12_working = true;
+			}
+		}
 	}
 }
 

kernel/x86-family/vbox.cpp

@@ -159,7 +159,7 @@ public:
 public:
 	bool Initialize();
 	void OnInterrupt();
-	void InterruptWork(void* payload, size_t size);
+	void InterruptWork();
 
 private:
 	void Request(void* bufptr, size_t size);
@@ -171,6 +171,7 @@ private:
 private:
 	struct vbox_host_version vbox_version;
 	struct interrupt_handler interrupt_registration;
+	struct interrupt_work interrupt_work;
 	kthread_mutex_t buffer_lock;
 	uint64_t video_device;
 	volatile struct registers* regs;
@@ -181,6 +182,7 @@ private:
 	uint32_t devaddr;
 	uint32_t capabilities;
 	uint32_t listening_events;
+	uint32_t interrupt_work_events;
 	addralloc_t mmio_alloc;
 	addralloc_t buffer1_alloc;
 	addralloc_t buffer2_alloc;
@@ -196,6 +198,11 @@ private:
 
 };
 
+void VBoxDevice__InterruptWork(void* context)
+{
+	((VBoxDevice*) context)->InterruptWork();
+}
+
 VBoxDevice::VBoxDevice(uint32_t devaddr)
 {
 	this->buffer_lock = KTHREAD_MUTEX_INITIALIZER;
@@ -209,6 +216,9 @@ VBoxDevice::VBoxDevice(uint32_t devaddr)
 	this->has_buffer2_mapped = false;
 	this->has_interrupt_registered = false;
 	this->has_video_device = false;
+	this->interrupt_work.handler = VBoxDevice__InterruptWork;
+	this->interrupt_work.context = this;
+	this->interrupt_work_events = 0;
 }
 
 VBoxDevice::~VBoxDevice()
@@ -236,11 +246,6 @@ void VBoxDevice__OnInterrupt(struct interrupt_context*, void* context)
 	((VBoxDevice*) context)->OnInterrupt();
 }
 
-void VBoxDevice__InterruptWork(void* context, void* payload, size_t size)
-{
-	((VBoxDevice*) context)->InterruptWork(payload, size);
-}
-
 bool VBoxDevice::Initialize()
 {
 	ScopedLock lock(&buffer_lock);
@@ -367,6 +372,9 @@ void VBoxDevice::OnInterrupt()
 
 	regs->guest_event_mask = 0;
 
+	assert(interrupt_work_events == 0);
+	interrupt_work_events = host_events;
+
 	// TODO: There's no protection that this interrupt handler isn't clobbering
 	//       the buffer because we don't have the lock.
 
@@ -378,15 +386,13 @@ void VBoxDevice::OnInterrupt()
 	ack_events.events = host_events; // TODO: Or?
 	RequestIRQ(&ack_events, sizeof(ack_events));
 
-	Interrupt::ScheduleWork(VBoxDevice__InterruptWork, this,
-	                        &host_events, sizeof(host_events));
+	Interrupt::ScheduleWork(&interrupt_work);
 }
 
-void VBoxDevice::InterruptWork(void* payload, size_t /*size*/)
+void VBoxDevice::InterruptWork()
 {
 	ScopedLock lock(&buffer_lock);
-	uint32_t host_events;
-	memcpy(&host_events, payload, sizeof(host_events));
+	uint32_t host_events = interrupt_work_events;
 
 	if ( host_events & VBOX_EVENT_DISPLAY_CHANGE_REQUEST )
 	{
@@ -405,6 +411,7 @@ void VBoxDevice::InterruptWork(void* payload, size_t /*size*/)
 			Video::ResizeDisplay(video_device, display, xres, yres, bpp);
 	}
 
+	interrupt_work_events = 0;
 	regs->guest_event_mask = listening_events;
 }