Repository navigation
[bug] Release the pipeline lock before mgmt UART writes - #1206
Conversation
6c4faed to
3d694ff
Compare
3d694ff to
b875003
Compare
`handle_management_message` locked the P4 pipeline before dispatching the request. For dump and radix requests, for example, it kept that lock while waiting for the guest to drain the 1-byte UART FIFO. In turn, if a queue notify message could send a vCPU into the `process_guest_packet` fn, where it waited on the same lock, that vCPU would not finish its PIO exit. This would lead to the guest not draining the UART, and, boom, deadlock. ## The Fix Now, the dump path copies table state while under the lock and writes it after unlocking happens. Table updates still hold the lock while changing the pipeline. UART writes now have a deadline, where if a response times out partway through, the handler will try to write the missing newline while waiting for the next request. *Note*: I ran into this issue on a long-running voxel session. The pre-fix binary reproduces it within minutes under a scadm polling loop, while the fixed one does not (A/B testing).
b875003 to
c12da84
Compare
c12da84 to
fb8e165
Compare
iximeow
left a comment
iximeow
left a comment
There was a problem hiding this comment.
I'm not sure this is the change you'd want to make, but fundamentally I don't want to be (further) in the way of changes you need for softnpu or dev work etc.
I will admit upfront that the first time I looked at this change was before you'd edited the PR description to be not what is (afaict) an LLM's description of the problem. I think about LLMs as writers not infrequently. here, I'm also laughing a little because having Claude look at this diff, it said most of the same things I did but in several paragraphs more words. it's hard to convince myself to want to pick up the context around a bug, even if it's very clearly wrong to hold a mutex over a sched_yield(), when my impression is that this code is relegated to /goal make it work.
if this fixes a problem for you, go ahead I guess? I can easily +1 this I suppose. if I should read this as "there is a bug in softnpu.rs, fix plz", then that's also fine and I'd mostly just take out the resyncing idea. either way we go is fine here? but either way I should have made the time to read the context and at least get some review to you sooner.
| // scheduled again. | ||
| std::thread::yield_now(); | ||
| } | ||
| if write_management_response(&uart, &buf, needs_resync, &log) { |
There was a problem hiding this comment.
I assume this is all basically hand-driven dev environment stuff so this doesn't matter: does scadm have an expected semantic of the relationship between the loaded program and the table state it's reading here? if yes: dropping the lock does mean you can have situations like "started reading the table, loaded a new program, finished reading the table, interpreted the table w.r.t the wrong program since it changed out from under you".
There was a problem hiding this comment.
I wrote out a TODO here that we can come back to in the DumpRequest case.
|
@iximeow I'm going to simplify this change, based on our discussion, and just avoid the lock over the entire mgmt UART write(s). So, I'll remove all the deadlining, resyncing, etc. |
zeeshanlakhani
changed the title
The deadlock fix in place already releases the pipeline mutex before serializing and dumping out entries from all tables. A vCPU in the virtio queue notify path can then return to the guest and drain the UART. After discussion with @iximeow, this simplifies the goal here: we were yielding a thread while holding a mutex over the entire UART set of management writes, which is bad. This effort removes the deadlines and resync handling, restoring the yielding write loops we had in place. The lock stays scoped to just the table updates and dump capture.
0242d40 to
11cf098
Compare
|
@iximeow updated with the more focused fix. |
2 participants
This bug came up during a long-running voxel session that was constantly dumping table state (in my voxel watcher script). I was able to reproduce this within minutes while running a
scadmpolling loop.In the softnpu code, we were yielding a thread while also holding onto a mutex over the entire set of management UART writes. As is, this could lead to a deadlock situation in the guest if a vCPU handling a virtio queue notify blocks on the same pipeline mutex. In this case, the management thread spinning and waiting for FIFO space, with the mutex still being held, could keep the vCPU blocked, unable to return to the guest. If the guest needs this vCPU to drain the UART, then nothing will drain it.
This fix scopes the lock to the table updates and dump capture specifically, serializing and writing the owned snapshot only after the mutex is released.