fix(pipe_read): reads don't need to be atomic

change(ahci): spin for a short time first before switching
              to interrupt wait

src/driver/ahci/mod.rs

@@ -1,12 +1,14 @@
 use crate::{
+    fs::procfs,
     kernel::{
         block::{make_device, BlockDevice},
         interrupt::register_irq_handler,
+        mem::paging::PageBuffer,
     },
     prelude::*,
 };
 
-use alloc::sync::Arc;
+use alloc::{format, sync::Arc};
 use bindings::{
     kernel::hw::pci::{self, pci_device},
     EIO,
@@ -87,6 +89,14 @@ impl Device {
             if let Err(e) = (|| -> KResult<()> {
                 port.init()?;
 
+                {
+                    let port = port.clone();
+                    let name = format!("ahci-p{}-stats", port.nport);
+                    procfs::populate_root(name.into_bytes().into(), move |buffer| {
+                        writeln!(buffer, "{:?}", port.stats.lock().as_ref()).map_err(|_| EIO)
+                    })?;
+                }
+
                 let port = BlockDevice::register_disk(
                     make_device(8, nport * 16),
                     2147483647, // TODO: get size from device
@@ -106,13 +116,10 @@ impl Device {
     }
 
     fn handle_interrupt(&self) {
-        println_debug!("ahci interrupt fired");
-
         // Safety
         // `self.ports` is accessed inside irq handler
         let ports = self.ports.lock();
         for nport in self.control.pending_interrupts() {
-            println_debug!("processing port {nport}");
             if let None = ports[nport as usize] {
                 println_warn!("port {nport} not found");
                 continue;
@@ -130,7 +137,6 @@ impl Device {
             vwrite(port.interrupt_status(), PORT_IS_DHRS);
 
             self.control.clear_interrupt(nport);
-            println_debug!("clear port {nport} interrupt flags");
 
             port.handle_interrupt();
         }
@@ -157,9 +163,7 @@ impl Device {
         device.control.enable_interrupts();
 
         let device_irq = device.clone();
-        register_irq_handler(irqno as i32, move || {
-            device_irq.handle_interrupt()
-        })?;
+        register_irq_handler(irqno as i32, move || device_irq.handle_interrupt())?;
 
         device.probe_ports()?;
 
@@ -179,9 +183,7 @@ unsafe extern "C" fn probe_device(pcidev: *mut pci_device) -> i32 {
 }
 
 pub fn register_ahci_driver() {
-    let ret = unsafe {
-        pci::register_driver_r(VENDOR_INTEL, DEVICE_AHCI, Some(probe_device))
-    };
+    let ret = unsafe { pci::register_driver_r(VENDOR_INTEL, DEVICE_AHCI, Some(probe_device)) };
 
     assert_eq!(ret, 0);
 }

src/driver/ahci/port.rs

@@ -11,8 +11,8 @@ use crate::sync::condvar::CondVar;
 
 use super::command::{Command, IdentifyCommand, ReadLBACommand};
 use super::{
-    vread, vwrite, CommandHeader, PRDTEntry, FISH2D, PORT_CMD_CR, PORT_CMD_FR,
-    PORT_CMD_FRE, PORT_CMD_ST, PORT_IE_DEFAULT,
+    vread, vwrite, CommandHeader, PRDTEntry, FISH2D, PORT_CMD_CR, PORT_CMD_FR, PORT_CMD_FRE,
+    PORT_CMD_ST, PORT_IE_DEFAULT,
 };
 
 fn spinwait_clear(refval: *const u32, mask: u32) -> KResult<()> {
@@ -135,13 +135,28 @@ impl FreeList {
     }
 }
 
+#[derive(Default, Debug)]
+pub struct AdapterPortStats {
+    /// Number of commands sent
+    cmd_sent: u64,
+
+    /// Number of transmission errors
+    cmd_error: u64,
+
+    /// Number of interrupts fired
+    int_fired: u64,
+}
+
 pub struct AdapterPort {
-    nport: u32,
+    pub nport: u32,
     regs: *mut (),
     page: Page,
     slots: [CommandSlot; 32],
     free_list: Spin<FreeList>,
     free_list_cv: CondVar,
+
+    /// Statistics for this port
+    pub stats: Spin<AdapterPortStats>,
 }
 
 /// # Safety
@@ -158,13 +173,12 @@ impl AdapterPort {
             nport,
             regs: NoCachePP::new(base + 0x100 + 0x80 * nport as usize).as_ptr(),
             slots: core::array::from_fn(|index| {
-                CommandSlot::new(unsafe {
-                    cmdheaders_start.offset(index as isize)
-                })
+                CommandSlot::new(unsafe { cmdheaders_start.offset(index as isize) })
             }),
             free_list: Spin::new(FreeList::new()),
             free_list_cv: CondVar::new(),
             page,
+            stats: Spin::default(),
         }
     }
 }
@@ -202,15 +216,12 @@ impl AdapterPort {
         vread(self.sata_status()) & 0xf == 0x3
     }
 
-    fn get_free_slot(&self) -> usize {
+    fn get_free_slot(&self) -> u32 {
         let mut free_list = self.free_list.lock_irq();
 
         loop {
             match free_list.free.pop_front() {
-                Some(slot) => {
-                    free_list.working.push_back(slot);
-                    break slot as usize;
-                }
+                Some(slot) => break slot,
                 None => {
                     self.free_list_cv.wait(&mut free_list, false);
                 }
@@ -218,6 +229,10 @@ impl AdapterPort {
         }
     }
 
+    fn save_working(&self, slot: u32) {
+        self.free_list.lock().working.push_back(slot);
+    }
+
     fn release_free_slot(&self, slot: u32) {
         self.free_list.lock().free.push_back(slot);
         self.free_list_cv.notify_one();
@@ -236,11 +251,12 @@ impl AdapterPort {
 
             let slot = &self.slots[n as usize];
 
-            println_debug!("slot{n} finished");
-
             // TODO: check error
-            slot.inner.lock().state = SlotState::Finished;
+            let mut slot_inner = slot.inner.lock();
+            debug_assert_eq!(slot_inner.state, SlotState::Working);
+            slot_inner.state = SlotState::Finished;
             slot.cv.notify_all();
+            self.stats.lock().int_fired += 1;
 
             false
         });
@@ -278,15 +294,15 @@ impl AdapterPort {
         let command_fis: &mut FISH2D = cmdtable_page.as_cached().as_mut();
         command_fis.setup(cmd.cmd(), cmd.lba(), cmd.count());
 
-        let prdt: &mut [PRDTEntry; 248] =
-            cmdtable_page.as_cached().offset(0x80).as_mut();
+        let prdt: &mut [PRDTEntry; 248] = cmdtable_page.as_cached().offset(0x80).as_mut();
 
         for (idx, page) in pages.iter().enumerate() {
             prdt[idx].setup(page);
         }
 
-        let slot_index = self.get_free_slot();
+        let slot_index = self.get_free_slot() as usize;
         let slot_object = &self.slots[slot_index];
+
         let mut slot = slot_object.inner.lock_irq();
 
         slot.setup(
@@ -298,25 +314,37 @@ impl AdapterPort {
 
         // should we clear received fis here?
         debug_assert!(vread(self.command_issue()) & (1 << slot_index) == 0);
-
-        println_debug!("slot{slot_index} working");
         vwrite(self.command_issue(), 1 << slot_index);
 
-        while slot.state == SlotState::Working {
-            slot_object.cv.wait(&mut slot, false);
+        if spinwait_clear(self.command_issue(), 1 << slot_index).is_err() {
+            let mut saved = false;
+            while slot.state == SlotState::Working {
+                if !saved {
+                    saved = true;
+                    self.save_working(slot_index as u32);
+                }
+                slot_object.cv.wait(&mut slot, false);
+            }
+        } else {
+            // TODO: check error
+            slot.state = SlotState::Finished;
         }
 
         let state = slot.state;
         slot.state = SlotState::Idle;
 
+        debug_assert_ne!(state, SlotState::Working);
         self.release_free_slot(slot_index as u32);
 
-        drop(slot);
-        println_debug!("slot{slot_index} released");
-
         match state {
-            SlotState::Finished => Ok(()),
-            SlotState::Error => Err(EIO),
+            SlotState::Finished => {
+                self.stats.lock().cmd_sent += 1;
+                Ok(())
+            }
+            SlotState::Error => {
+                self.stats.lock().cmd_error += 1;
+                Err(EIO)
+            }
             _ => panic!("Invalid slot state"),
         }
     }
@@ -343,7 +371,7 @@ impl AdapterPort {
         match self.identify() {
             Err(err) => {
                 self.stop_command()?;
-                return Err(err);
+                Err(err)
             }
             Ok(_) => Ok(()),
         }
@@ -361,8 +389,7 @@ impl BlockRequestQueue for AdapterPort {
             return Err(EINVAL);
         }
 
-        let command =
-            ReadLBACommand::new(req.buffer, req.sector, req.count as u16)?;
+        let command = ReadLBACommand::new(req.buffer, req.sector, req.count as u16)?;
 
         self.send_command(&command)
     }

src/fs/procfs.rs

@@ -226,7 +226,7 @@ pub fn root() -> ProcFsNode {
 
 pub fn creat(
     parent: &ProcFsNode,
-    name: &Arc<[u8]>,
+    name: Arc<[u8]>,
     file: Box<dyn ProcFsFile>,
 ) -> KResult<ProcFsNode> {
     let parent = match parent {
@@ -244,7 +244,7 @@ pub fn creat(
         parent
             .entries
             .access_mut(lock.as_mut())
-            .push((name.clone(), ProcFsNode::File(inode.clone())));
+            .push((name, ProcFsNode::File(inode.clone())));
     }
 
     Ok(ProcFsNode::File(inode))
@@ -287,8 +287,44 @@ pub fn init() {
 
     creat(
         &root(),
-        &Arc::from(b"mounts".as_slice()),
+        Arc::from(b"mounts".as_slice()),
         Box::new(DumpMountsFile),
     )
     .unwrap();
 }
+
+pub struct GenericProcFsFile<ReadFn>
+where
+    ReadFn: Send + Sync + Fn(&mut PageBuffer) -> KResult<()>,
+{
+    read_fn: Option<ReadFn>,
+}
+
+impl<ReadFn> ProcFsFile for GenericProcFsFile<ReadFn>
+where
+    ReadFn: Send + Sync + Fn(&mut PageBuffer) -> KResult<()>,
+{
+    fn can_read(&self) -> bool {
+        self.read_fn.is_some()
+    }
+
+    fn read(&self, buffer: &mut PageBuffer) -> KResult<usize> {
+        self.read_fn.as_ref().ok_or(EACCES)?(buffer).map(|_| buffer.len())
+    }
+}
+
+pub fn populate_root<F>(name: Arc<[u8]>, read_fn: F) -> KResult<()>
+where
+    F: Send + Sync + Fn(&mut PageBuffer) -> KResult<()> + 'static,
+{
+    let root = root();
+
+    creat(
+        &root,
+        name,
+        Box::new(GenericProcFsFile {
+            read_fn: Some(read_fn),
+        }),
+    )
+    .map(|_| ())
+}

src/kernel.ld

@@ -122,6 +122,7 @@ SECTIONS
         *(.got)
         *(.got.plt)
 
+        . = . + 4;
         . = ALIGN(0x1000) - 4;
         LONG(KERNEL_MAGIC);
 

src/kernel/syscall/procops.cc

@@ -59,12 +59,8 @@ execve_retval kernel::syscall::do_execve(const std::string& exec,
 
     current_process->files.onexec();
 
-    async::preempt_disable();
-
     // TODO: set cs and ss to compatibility mode
     if (int ret = types::elf::elf32_load(d); ret != 0) {
-        async::preempt_enable();
-
         if (ret == types::elf::ELF_LOAD_FAIL_NORETURN)
             kill_current(SIGSEGV);
 
@@ -72,8 +68,6 @@ execve_retval kernel::syscall::do_execve(const std::string& exec,
     }
 
     current_thread->signals.on_exec();
-    async::preempt_enable();
-
     return {d.ip, d.sp, 0};
 }
 

src/kernel/task/thread.cc

@@ -28,14 +28,10 @@ struct PACKED tss64_t {
 };
 constexpr physaddr<tss64_t> tss{0x00000070};
 
-thread::thread(std::string name, pid_t owner)
-    : owner{owner}, attr{READY | SYSTEM}, name{name} {}
+thread::thread(std::string name, pid_t owner) : owner{owner}, attr{READY | SYSTEM}, name{name} {}
 
 thread::thread(const thread& val, pid_t owner)
-    : owner{owner}
-    , attr{val.attr}
-    , name{val.name}
-    , tls_desc32{val.tls_desc32} {}
+    : owner{owner}, attr{val.attr}, name{val.name}, tls_desc32{val.tls_desc32} {}
 
 tid_t thread::tid() const {
     return (tid_t)kstack.pfn;
@@ -50,8 +46,7 @@ bool thread::operator==(const thread& rhs) const {
 }
 
 static inline uintptr_t __stack_bottom(pfn_t pfn) {
-    return (uintptr_t)(void*)kernel::mem::physaddr<void>{
-        pfn + (1 << KERNEL_STACK_ORDER) * 0x1000};
+    return (uintptr_t)(void*)kernel::mem::physaddr<void>{pfn + (1 << KERNEL_STACK_ORDER) * 0x1000};
 }
 
 thread::kernel_stack::kernel_stack() {
@@ -112,20 +107,17 @@ void thread::set_attr(thd_attr_t new_attr, bool forced) {
             break;
         case READY:
             if (attr & ZOMBIE) {
-                kmsgf("[kernel:warn] zombie process pid%d tries to wake up",
-                      owner);
+                kmsgf("[kernel:warn] zombie process pid%d tries to wake up", owner);
                 break;
             }
 
-            if (attr & READY) {
-                kmsgf("[kernel:warn] trying to wake up %d from USLEEP",
-                      this->owner);
-
+            if (attr & READY)
                 break;
-            }
 
-            if (!forced && attr & USLEEP)
+            if (!forced && attr & USLEEP) {
+                kmsgf("[kernel:warn] trying to wake up %d from USLEEP", this->owner);
                 break;
+            }
 
             attr &= SYSTEM;
             attr |= READY;

src/kernel/vfs.cpp

@@ -267,15 +267,10 @@ int fs::pipe::read(char* buf, size_t n) {
     kernel::async::lock_guard lck{mtx};
     size_t orig_n = n;
 
-    if (n <= PIPE_SIZE || this->buf.empty()) {
-        while (is_writeable() && this->buf.size() < n) {
-            bool interrupted = waitlist_r.wait(mtx);
-            if (interrupted)
-                return -EINTR;
-
-            if (n > PIPE_SIZE)
-                break;
-        }
+    while (is_writeable() && this->buf.size() == 0) {
+        bool interrupted = waitlist_r.wait(mtx);
+        if (interrupted)
+            return -EINTR;
     }
 
     while (!this->buf.empty() && n)

src/kernel/vfs/filearr.cc

@@ -188,8 +188,6 @@ static inline std::pair<dentry_pointer, int> _open_file(const fs_context& contex
 // TODO: file opening permissions check
 int filearray::open(const dentry_pointer& cwd, types::string_view filepath, int flags,
                     mode_t mode) {
-    lock_guard lck{pimpl->mtx};
-
     auto [dent, ret] = _open_file(*pimpl->context, cwd, filepath, flags, mode);
 
     assert(dent || ret != 0);
@@ -223,6 +221,8 @@ int filearray::open(const dentry_pointer& cwd, types::string_view filepath, int
         }
     }
 
+    lock_guard lck{pimpl->mtx};
+
     return pimpl->place_new_file(std::make_shared<regular_file>(fflags, 0, d_get(dent)), fdflag);
 }
 

src/sync/lock.rs

@@ -23,6 +23,15 @@ impl<Value, Strategy: LockStrategy> Lock<Value, Strategy> {
     }
 }
 
+impl<Value: Sized + Default, Strategy: LockStrategy> Default for Lock<Value, Strategy> {
+    fn default() -> Self {
+        Self {
+            strategy_data: Strategy::data(),
+            value: Default::default(),
+        }
+    }
+}
+
 impl<Value: ?Sized, Strategy: LockStrategy> Lock<Value, Strategy> {
     #[inline(always)]
     pub fn lock<'lt>(&'lt self) -> Guard<'lt, Value, Strategy> {