IRC channel logs
2022-02-27.log
back to list of logs
<damo22>youpi: elements of bd are never removed, so i dont think search_name needs to be protected by the lock <youpi>damo22: unless you use proper barriers, you have *no* guarantess <youpi>notabley that block_head = bd is actually visible *before* bd->next = block_heax <youpi>if the visibility is inverted, the linked list is potentially incoherent <damo22>yes i agree but i think we have another bug <damo22>bug: that device_close does not remove bd from the list? <youpi>device_close should either remove the entry or mark it as unusable <damo22>should search_name instead check the taken refcount? <damo22>since that will be zero when closed <damo22>and i can put the locks around the critical paths <youpi>using a refcount is a possibility indeed <youpi>locks around the criticazl paths: note that you need to keep some lockig around the whole read/write operation <youpi>thus the idea of using an rwlock <youpi>yes, I meant using the existing refcount <youpi>that can be used instead of dropping the entry from the list <damo22>i dont know what a rwlock is, from the man page it seems to be 2 mutexes fused? <youpi>it's just a mutex that allow only one writer at a time, and serveral readers at a time <youpi>and not writer/reader at the same time <youpi>that's exactly matching the open/close vs read/write semantic <youpi>as in: open/close write to the list, read/write just read it <damo22>ah so we dont restrict read/writes to be sequential <youpi>letting read/write be parallel <youpi>to benefit the most from whatever can be in parallel <damo22>i noticed there is only one unlock functoin <youpi>err, at best that'd be subjective: this is still using the monothread libports management, so it cannot be faster by just adding mutexes <damo22>its faster than my previous attempt making it serial <youpi>yes, the thread interface is supposed to remember whether that was an rd or wr lock <youpi>still, the monothread libports management already make it serial <damo22>but previously i had read/write being sequential and had to be interleaved <youpi>libports already make operations sequential <youpi>when using ports_manage_port_operations_one_thread <damo22>yea i didnt want to change that in libmachdev <damo22>because the arbiter uses that too <youpi>the whole point is adding a machdev_server_multithread variant that uses ports_manage_port_operations_multithread <youpi>so that the various request of ext2fs don't have to wait for each other <AwesomeAdam54321>I'm passively reading here, I don't think I can contribute to the HURD anytime soon <damo22>Jari--: not much, it takes a long time to make things happen in hurd because there arent many people actively working on it <Jari-->damo22 it progressed a lot since last time I checked, last time no ISO images at all, only a single .tar.gz <damo22>im pretty new to it, i only started contributing in around 2019 <damo22>im still learning new things every time i send in a patch <damo22>youpi is fantastic at code review <damo22>youpi: what needs to be protected in pci-arbiter to make libmachdev use the multithread libports management? <damo22>i am guessing a similar rwlock may be required <damo22>maybe the lock should have been made in libmachdev instead? <damo22>we could protect the device interface itself from concurrent open/close etc <damo22>or do we want to have single threaded libmachdev for some use cases <damo22>its a bit tricky because there are 32 device emulations, so it would need 32 rwlocks <damo22>im going to add a rwlock to pci-arbiter instead <youpi>damo22: I don't think we need to spend time on making pci-arbiter multi-threaded, it's not used often <damo22>and switched rump to multithreaded <youpi>damo22: perhaps pci-arbiter doesn't need locking because its state doesn't change at all? <youpi>ok there's a create_node at least <damo22>the pci_control_port gets overwritten <damo22>and used a rwlock so if we ever need device_read/write in the arbiter it will be a matter of adding it in <youpi>? pci_control_port is modified only at startup <damo22>you can drop my commit if its not needed, its a separate commit and i tested it in isolation <damo22>device_close releases that port though <youpi>the name of the port by itself doesn't change <youpi>actually I don't understand that code <youpi>pci_control_port is just a port name, why calling ports_port_deref on it <damo22>does device_close ever get called <damo22>pci-arbiter uses a different machdev_server function, so i changed the one in libmachdev for rumpdisk <damo22>maybe that bug we just found explains why joan was getting a crash in the arbiter after the port management timed out? <damo22>maybe it called device_close and died <youpi>check whether libpciaccess actually uses device_close <damo22>it calls device_close indeed but only if the device_open fails <youpi>device_close is needed to close a device successfully opened... <damo22>it closes it after a successful open <damo22>open; root=lookup_under; close; dealloc <damo22>pci_control_port = ports_get_send_right (newpi); <youpi>no, but that still makes it a port name, not a void* <youpi>it would be the newpi on which it could make sense to use a deref <youpi>but again, there's just pcifs_startup that creates a reference <youpi>so there's no reference to unref <damo22>so should i deref it after calling fsys_startup? <damo22>the libmachdev wont call pci_device_close if its not implemented