IRC channel logs


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
<damo22>or we need to fix that
<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
<youpi>that *does* happen
<damo22>bug: that device_close does not remove bd from the list?
<youpi>yes, that's another issue
<youpi>device_close should either remove the entry or mark it as unusable
<youpi>but that's orthogonal
<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
<damo22>we have a refcount already
<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>no that's the whole ponit
<youpi>letting read/write be parallel
<youpi>to benefit the most from whatever can be in parallel
<damo22>ok good
<damo22>im working on it now
<damo22>it works, and is faster too
<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
<damo22>that was slow
<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
<damo22>that will make it faster
<damo22>and ready for multicore
<Jari-->so whats up guys
<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>it had images then
<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>the emulated one at least
<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
<damo22>it seems simpler
<youpi>damo22: I don't think we need to spend time on making pci-arbiter multi-threaded, it's not used often
<damo22>its already multithreaded
<damo22>but there is no lock
<damo22>i fixed that, sending patches
<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>so i protected that
<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
<damo22>i added comments
<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>sending in a sec
<damo22>device_close releases that port though
<youpi>the name of the port by itself doesn't change
<damo22>hmm yea
<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>a bug?
<damo22>does device_close ever get called
<youpi>probably a bug yes
<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>? that's meaningless
<youpi>device_close is needed to close a device successfully opened...
<damo22>oh no wrong way around
<damo22>my bad
<damo22>it closes it after a successful open
<damo22>and it gets the port
<damo22>open; root=lookup_under; close; dealloc
<damo22>pci_control_port = ports_get_send_right (newpi);
<damo22>its a send right
<damo22>maybe named badly?
<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
<damo22>oh right
<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?
<youpi>no, just not at all
<youpi>the port always exists
<youpi>no reason to unref it
<damo22>ok so close can go away
<damo22>the libmachdev wont call pci_device_close if its not implemented
*youpi off for the day