When read thread is enabled, perform all the writes in the thread
as well. Most importantly, this ensures that we do not attempt to
simultaneously read and write the same fd from different threads.
Sponsored by: The FreeBSD Foundation
Differential D132578
[lldb] [Core] Use thread for Communication::Write() as well mgorny on Aug 24 2022, 11:06 AM. Authored by
Details
When read thread is enabled, perform all the writes in the thread Sponsored by: The FreeBSD Foundation
Diff Detail Event TimelineComment Actions Can you explain where you're heading with this? I can imagine that something like this might be necessary in some cases, but I also don't see anything inherently wrong with concurrent reads and writes to the same connection. Comment Actions Well, back when working on the async thread, you've indicated that it's a bad idea to read from one thread while writing from another, so I've basically focused on getting to the point when all reads and writes are guaranteed to happen from a single thread. Comment Actions (with the final goal of using StartReadThread() inside gdb-remote comms to take care of reading the data for concurrent process plugin instances) Comment Actions Ok, I think I know what you mean. That comment was made in the context of the GDB communication class, whose functionality is more complex (sending a packet involves both reading and writing) than the plain Communication. Sending packets from multiple threads is more complex that a hypothetical simple setup where you have a single thread just doing reading, and another thread just for writing... ... but we don't have such a case, so doing this might be fine. I sort of understand the direction you're going with this, but I can't really visualize the final state. Could you give a short summary of the desired final architecture. FWIW, my idea was to use the gdb-remote "async" thread for this centralized sort of reading(sending)/writing(receiving). The async thread serves a similar purpose to the communication read thread, but is completely unrelated to it. I thought that would be the easiest to implement, since we already kind of have a synchronization mechanism (the "continue" lock) between these thread and other threads. However, I don't have a complete picture of how would this work (I'd guess we have a single "async" thread for all of the processes, and it would somehow synchronize/arbitrate between them), so I am open to other ideas as well. Comment Actions Well, my "baseline" idea was something like that: having a shared (e.g. via shared_ptr) m_gdb_comm, and running async thread as part of that. The thread would read packets from remote and split them into two groups: asynchronous events (i.e. stop reasons and related packets) and synchronous replies. This is reasonably easy for non-stop protocol since the former are always sent as % notification, while the latter as regular packets. For non-nonstop case, I suppose we would just trigger some explicit switch on resuming/leaving resume. Asynchronous events would be passed (e.g. via callback) to process instances that are currently resumed. Synchronous replies would be just passed to whichever process is currently waiting for the reply. However, the problem with that design is that it requires a lot of different changes, and the gdb-remote plugin code is not exactly friendly to that. So I was trying to split it into smaller steps that would be testable and reviewable on their own, i.e. basically refactor, refactor, get multiprocess working, cleanup, cleanup. What these changes were aiming at was basically moving all the reads and writes from gdb-remote into a dedicated thread reusing the code already in LLDB, so I could initially add support for the new "async" thread and handling the resume-related packets there without having to immediately replace the existing code for synchronous packets that reads and writes to m_gdb_comm directly from a lot of places. However, now that I think of it, I didn't really think it through and using read thread along with some trivial locking and queues won't be sufficient to be able to cleanly supports reading these two kinds of packets from different threads. At this point I'm basically lost in the woods and I'm not really sure how to proceed, or how much of this code is actually worth further consideration. Comment Actions Ok, so I guess I'm back the original plan of using async thread to do the packet exchanges. Right now, the rough plan is to:
Exact details TBD. It's possible that initially I'll leave the per-process "old" async threads as well in order to reduce the diffs. Comment Actions Seems reasonable. Just be aware that the packet RTT is very important for some, so we will either make sure that the extra thread is not used in the base case (no processes running), or somehow ensure that the extra thread does not introduce delays. Comment Actions I'll bear that in mind. Worst case, the wrapper class can take care of conditionally enabling the extra thread and falling back to invoking comms directly. |