enhance virtio_queue to better support multi-threading #76

jiangliu · 2021-08-18T16:31:29Z

Refactor virtio_queue::Queue to better support multi-threading, and also ease migration of existing consumers of the crate. The main changes are:

introduce trait QueueStateT<M: GuestAddressSpace>, so we could use different concrete queue state objects for different queues.
split QueueState out of Queue structure, now struct Queue is a simple wrapper over QueueState with an associated memory object.
introduce struct QueueStateSync for multi-threaded context.
move go_to_previous_position() to struct AvailIter().

Fixes: #89 #90

andreeaflorescu · 2021-08-19T07:24:42Z

@jiangliu this is a super interesting PR! We are working now on a design to support save/restore state in rust-vmm components (while keeping the serialization/deserialization product specific), and we came up with a design that is quite similar to what you are proposing here with the QueueState. In a nutshell, the idea is to have a state state structure in all components for which we need the state, and always initialize the component from the state. @lauralt is working on a design document that we should be able to share with the community next week.

Looking high level, there are also a few differences between the proposals, so I would need to do an in-depth review. For example, we did not deem necessary a QueueStateTrait, so I'd like to understand if that is needed (or relevant) for the save/restore implementation.

jiangliu · 2021-09-01T09:19:46Z

@jiangliu this is a super interesting PR! We are working now on a design to support save/restore state in rust-vmm components (while keeping the serialization/deserialization product specific), and we came up with a design that is quite similar to what you are proposing here with the QueueState. In a nutshell, the idea is to have a state state structure in all components for which we need the state, and always initialize the component from the state. @lauralt is working on a design document that we should be able to share with the community next week.

Looking high level, there are also a few differences between the proposals, so I would need to do an in-depth review. For example, we did not deem necessary a QueueStateTrait, so I'd like to understand if that is needed (or relevant) for the save/restore implementation.

hi @andreeaflorescu , any update on this topic? The vm-virtio/virtio-queue becomes a blocking point for several of our projects:(

lauralt · 2021-09-01T15:13:58Z

@jiangliu this is a super interesting PR! We are working now on a design to support save/restore state in rust-vmm components (while keeping the serialization/deserialization product specific), and we came up with a design that is quite similar to what you are proposing here with the QueueState. In a nutshell, the idea is to have a state state structure in all components for which we need the state, and always initialize the component from the state. @lauralt is working on a design document that we should be able to share with the community next week.
Looking high level, there are also a few differences between the proposals, so I would need to do an in-depth review. For example, we did not deem necessary a QueueStateTrait, so I'd like to understand if that is needed (or relevant) for the save/restore implementation.

hi @andreeaflorescu , any update on this topic? The vm-virtio/virtio-queue becomes a blocking point for several of our projects:(

Hi, I just opened an RFC with our proposal, sorry for the delay and please have a look! We spent some time thinking whether we should have the (de)ser and versionize support in rust-vmm or let the VMM completely handle that, would be great to hear your feedback on this as well.

slp

Thanks for working on this. I think this is a step in the right direction, that tackles both #89 and #90. It's also consistent with the direction of vhost and vhost-user-backend.

Of course, this is going to require changes in the crate consumers, but I checked them with vhost-user-backend and found them to be reasonable and quite straightforward.

alexandruag

Hi, left some high level comments for now. Also, would be really nice to add the new abstractions (i.e. QueueState, QueueStateSync, etc.) in their own files/modules once we're happy with all the other details.

alexandruag · 2021-09-15T22:11:46Z

crates/virtio-device/src/lib.rs

@@ -284,7 +284,7 @@ mod tests {
            assert_eq!(d.cfg.device_features & (1 << VIRTIO_F_RING_EVENT_IDX), 0);

            for q in d.cfg.queues.iter() {
-                assert_eq!(q.event_idx_enabled, false);
+                assert_eq!(q.state.event_idx_enabled, false);


Just as a mention, the config in devices should start holding QueueState objects instead of Queuess, but we can apply this change later.

Sure, let's cover it by a dedicated MR, so we could keep this MR easy to review:)

alexandruag · 2021-09-15T22:17:57Z

crates/virtio-queue/src/lib.rs

-pub struct Queue<M: GuestAddressSpace> {
-    mem: M,
-
+pub struct QueueState<M: GuestAddressSpace> {


Hmm, I think a QueueState object doesn't need to be parametrized with anything related to GuestMemory or GuestAddressSpace traits. Instead, we can keep GuestState with any type parameters, and add generic parameters to the individual methods which expect to work with a GuestMemory handle. For example, this commit also showcases separating the state into a distinct object, but without any type parameters.

I have tried to follow the suggestion, but the result is neutral, the QueueState has been simplified by removing the type parameter, the callers of QueueState's. All QueueState's methods take a mem: &GuestAddressSpace::M, so when calling those methods, you need to queuestate.avail_idx::<M>(&self.mem.memory(), order).
So I suggestion to keep the type parameter the ease the method invoke.

Hmm, so I think most (if not all) methods can work with a M: GuestMemory generic type parameter and a mem: &M argument, instead of having a M: GuestAddress space generic type parameter and mem: &GuestAddressSpace::M argument. This way we can get the best of both worlds, as we both remove the type parameter from QueueState, and don't need to explicitly type any additional qualifications when calling the methods. If this still doesn't work for the current approach, please nudge me and I'll give it a go as well, since removing the generic from QueueState can improve readability and conciseness by quite a lot.

Seems we also need <M: GuestAddressSpace> for

impl<M: GuestAddressSpace> QueueStateT<M> for QueueState<M>

Hmm, I think that can be worked around as well; I'll quickly try to better illustrate what I have in mind by adding an example commit on top of your changes in a branch somewhere, and ping here with a link when it's done a bit later.

I experimented some more with this, and I pushed an example to this branch. The last two commits are placed on top of the changes you introduce in this PR, and we can just take and use them if they make sense. Can you please have a look and share your thoughts? I'll also propose a small change to how we offer the lock semantics that you mentioned are required already; will ping back soon about that.

Btw I just found out why that weird Sized trait bound had to appear in the above example; it's because of the impl Bytes for T where T: GuestMemory auto implementation from vm-memory, which implies a Sized bound for T. I think we'll have to get rid of the auto impl at some point because it prevents a couple of useful scenarios, but for now I'll add a small change to relax the bounds there to include ?Sized as well (since there's no reason really not to).

I experimented some more with this, and I pushed an example to this branch. The last two commits are placed on top of the changes you introduce in this PR, and we can just take and use them if they make sense. Can you please have a look and share your thoughts? I'll also propose a small change to how we offer the lock semantics that you mentioned are required already; will ping back soon about that.

Thanks for your work to simplify the interface, it looks attractive. One drawback with the simplification is that we can't make trait object of QueueStateT anymore. I'm not sure whether there's any case needing QueueStateT trait object.

alexandruag · 2021-09-15T22:33:34Z

crates/virtio-queue/src/lib.rs

-    /// Goes back one position in the available descriptor chain offered by the driver.
-    /// Rust does not support bidirectional iterators. This is the only way to revert the effect
-    /// of an iterator increment on the queue.
-    pub fn go_to_previous_position(&mut self) {


We can add a go_to_previous_position method to AvailIter also, but I think should keep the methods on QueueState and Queue as well. AvailIter takes some mut references, which causes issues with the borrow checker in some scenarios (which can be addressed, for example, by adding a pop_descriptor_chain method to Queue/QueueState in the future).

The go_to_previous_position() method assumes there's only one thread consuming descriptor chain from the available ring, so it would rewind. With the new Queue and QueueState, the assumption is not true anymore. That's why I have moved go_to_previous_position() to AvailIter because AvailIter is protected by a lock logically.

Let's take an example:

thread A locks the queue, consume a descriptor head 1, advance next to 2, unlock

thread B locks the queue, consume a descriptor head 2, advance next to 3, unlock

thread A locks the queue, rewind the head to 1 by go_to_previous_position(), unlock.
Then descriptor 2 will be consumed twice.

Thanks for the explanation; let's suspend this discussion a bit until we get to the bottom of the one related to the lock method.

alexandruag · 2021-09-15T22:57:29Z

crates/virtio-queue/src/lib.rs

+    ///
+    /// Logically this method will acquire the underlying lock protecting the `QueueState` Object.
+    /// The lock will be released when the returned object gets dropped.
+    fn lock(&mut self) -> QueueStateGuard<'_, M>;


Hmm, I was just wondering, what's the purpose of this method? Since it takes a &mut self, we're already in a context where he have exclusive access to the object, and it looks like we can already perform all the operations using the methods from this trait.

If there's a good reason to have the lock method, then maybe we can get rid of QueueStateGuard by using a signature like fn lock(&mut self) -> impl DerefMut<Target=QueueState>, or have an associated type with a : DerefMut<Target=QueueState> bound (if the former doesn't actually work). The downside to doing either of these is (IIRC) not being able to have QueueStateT trait objects, but that doesn't seem like a use case we're trying to meet right now.

My first trial is to implement fn lock(&mut self) -> impl DerefMut<Target=QueueState> but it's really to hard to deal with the lifetime parameter needed. The I used the ugly QueueStateGuard to simplify the implementation.

The total goal of lock() method is to support following pattern:

fn process(queue: &mut Queue, eventfd: EventFd) -> Result<()> { loop { eventfd.read().unwrap(); // Lock the queue, get the next descriptor chain, and unlock let chain = queue.lock().iter().next(); ... // Lock the queue, add used descriptor, unlock. queue.add_used(chain.head_index); } }

Ok so after thinking a bit in circles about this :D, it looks like the lock method ultimately adds to the interface some semantics that should probably be handled behind the scenes by specific implementations. For example, given that QueueStateSync is added in this PR as well (and it also implements QueueStateT), don't we get proper synchronized behavior implicitly whenever we're (or a Queue is) using that as the underlying state, without the need for an explicit lock method?

That's the real art part:)
We are trying to balance between flexibility and performance. If we support multi-threading by simple replace single-threaded QueueState with multi-threaded versionQueueStateSync, we may not get too much because QueueStateSync causes too many lock/unlock operations. So we want fine-granularity control of the lock policy. For casual users, it may just replace QueueState with QueueStateSync, and everything should work as expected. For advanced users, it may call QueueStateSync::lock() to get the underlying guard object, and then optimize its lock policy.

You're definitely right that we should keep in mind that different consumers might need different policies with varying degrees of granularity. In my mind this can be achieved by having different backends for QueueStateT, instead of also adding lock to the QueueStateT interface explicitly. If, at some point, a new use case appears that cannot be efficiently addressed via either QueueState or QueueStateSync (it looks like they're enough for now, please correct if I'm wrong), then a new backend can be plugged in (and also upstreamed if not super-specific to a particular project). Do you think this more incremental approach works based on what we know so far?

We already have some clients needs the flexibility. The virtiofsd project and the nydus image service project also help to optimize performance by multi-threading. Our first trial has achieved some performance improvements, but it's still limited by the vm-queue interfaces. So I proposed this change to alleviate it.

Cool, so here's a simple pseudo-example of how we could redefine the presentation of lock semantics (there are a couple of comments with bits of additional context in there as well). Basically I think we can make things a bit clearer by defining a separate trait for sync semantics, and remove lock from the regular QueueState interface.

This gives consumers of the abstractions (and other abstractions that are built on top) the ability to clearly represent their expectations; for example something that will work with state in a context that needs synchronization will demand a QueueStateSyncT bound, whereas something that works with any QueueStateT makes it clear it's doesn't have synchronization concerns (also, we can move the go to previous position op back to regular state as well, which is actually surprisingly useful to continue to have).

Seems we have two possible way to build the interface here.
One way is to let the virtio driver claims that "I'm a superman to support multi-threading concurrence, but you must supply a special queue with multi-threading capability to me, otherwise I will refuse to work".
Another way is that the virtio driver is optimized for multi-threading. When a multi-threading capable queue is passed in, the driver works in multi-threading mode. When a single-threaded queue is passed in, the driver works too.
The current interface is designed for the latter case.

Also it may also affect the implementation of device manager in vmm, I need more time to figure it out:)

Hi again, I gave some more thought and I'm personally starting to lean towards a strong preference to move the lock semantics outside of QueueState into another abstraction. Let's say there's some component/consumer/code A that uses queue states without having multithreading concerns, and another B that does need to leverage MT for performance or any other reasons. A won't need lock (and might want to use go_to_previous position as part of the state interface), whereas B will explicitly call lock in different places.

The issues I see here is that we're keeping some functionality away from A, and then B has no way of ensuring that lock (which has to be provided by any QueueStateT implementation, since it's always a part of the interface) actually provides synchronization. Moreover, since consumers such as B call lock explicitly, they can also explicitly request a trait bound for the object. With this in mind, I think the approach where we have an additional trait (i.e. QueueStateSyncT or something along those lines) lifts the limitations on QueueStateT itself (because it no longer carries synchronization semantics of its own), and removes some of the ambiguity and potential pitfalls for use cases where sync is required. Also, we can add QueueStateSyncT: QueueStateT such that it can seamlessly be used for implicit synchronization where appropriate.

That being said, this is prob better discussed over an actual proposal/PR. So far, the bigger discussion points are removing the M generic parameter from QueueState and the sync story. Should we just merge the current PR with the understanding that discussions will continue and changes may happen via subsequent PRs?

Hi again, I gave some more thought and I'm personally starting to lean towards a strong preference to move the lock semantics outside of QueueState into another abstraction. Let's say there's some component/consumer/code A that uses queue states without having multithreading concerns, and another B that does need to leverage MT for performance or any other reasons. A won't need lock (and might want to use go_to_previous position as part of the state interface), whereas B will explicitly call lock in different places.

The issues I see here is that we're keeping some functionality away from A, and then B has no way of ensuring that lock (which has to be provided by any QueueStateT implementation, since it's always a part of the interface) actually provides synchronization. Moreover, since consumers such as B call lock explicitly, they can also explicitly request a trait bound for the object. With this in mind, I think the approach where we have an additional trait (i.e. QueueStateSyncT or something along those lines) lifts the limitations on QueueStateT itself (because it no longer carries synchronization semantics of its own), and removes some of the ambiguity and potential pitfalls for use cases where sync is required. Also, we can add QueueStateSyncT: QueueStateT such that it can seamlessly be used for implicit synchronization where appropriate.

That being said, this is prob better discussed over an actual proposal/PR. So far, the bigger discussion points are removing the M generic parameter from QueueState and the sync story. Should we just merge the current PR with the understanding that discussions will continue and changes may happen via subsequent PRs?

Sounds like a plan, let's do it step by step. It's ok to reach a relative stable interface before publishing v0.1. I will rebase it tomorrow.

alexandruag · 2021-09-15T22:59:26Z

crates/virtio-queue/src/lib.rs

    pub fn set_next_avail(&mut self, next_avail: u16) {
        self.state.set_next_avail(next_avail);
    }
 }

+impl<M: GuestAddressSpace> Queue<M, QueueState<M>> {
+    /// A consuming iterator over all available descriptor chain heads offered by the driver.
+    pub fn iter(&mut self) -> Result<AvailIter<'_, M>, Error> {


What prevents this from being part of the more generic impl Queue above this one?

For QueueStateSync to implement the iter() method, it needs to lock the underlying Mutex, which means the concurrence has actually been disabled by iter(). And to support QueueStateSync::iter(), we need to change prototype of iter() to something like:

pub fn iter(&mut self) -> Result<(GuardForAvailIter, AvailIter<'_, M>), Error>;

A guard is needed to protect the returned AvailIter object.

jiangliu · 2021-09-16T01:04:08Z

Hi, left some high level comments for now. Also, would be really nice to add the new abstractions (i.e. QueueState, QueueStateSync, etc.) in their own files/modules once we're happy with all the other details.

I plan to do the split the a dedicated MR:)

alexandruag

Hi Gerry, left a couple of additional comments and some small examples. Looking forward to hearing your thoughts.

I guess one last high level comment is whether we should actually split the new interfaces into non-mut and mut parts (i.e. QueueStateT and QueueStateMutT depending on whether the methods need &self or &mut self), as this makes things easier with certain bounds and implementations for smart pointer-like objects (for example). It's prob not super important right now, but it will become more complicated to change as time goes by.

Also, we can ultimately merge this as is and add changes on top afterwards; I think the important thing right now is to get aligned on the direction, and then we can get there in as many steps as we need.

alexandruag · 2021-10-04T13:03:02Z

crates/virtio-queue/src/lib.rs

-pub struct Queue<M: GuestAddressSpace> {
-    mem: M,
-
+pub struct QueueState<M: GuestAddressSpace> {


Btw I just found out why that weird Sized trait bound had to appear in the above example; it's because of the impl Bytes for T where T: GuestMemory auto implementation from vm-memory, which implies a Sized bound for T. I think we'll have to get rid of the auto impl at some point because it prevents a couple of useful scenarios, but for now I'll add a small change to relax the bounds there to include ?Sized as well (since there's no reason really not to).

alexandruag · 2021-10-04T13:09:20Z

crates/virtio-queue/src/lib.rs

+    ///
+    /// Logically this method will acquire the underlying lock protecting the `QueueState` Object.
+    /// The lock will be released when the returned object gets dropped.
+    fn lock(&mut self) -> QueueStateGuard<'_, M>;


Cool, so here's a simple pseudo-example of how we could redefine the presentation of lock semantics (there are a couple of comments with bits of additional context in there as well). Basically I think we can make things a bit clearer by defining a separate trait for sync semantics, and remove lock from the regular QueueState interface.

This gives consumers of the abstractions (and other abstractions that are built on top) the ability to clearly represent their expectations; for example something that will work with state in a context that needs synchronization will demand a QueueStateSyncT bound, whereas something that works with any QueueStateT makes it clear it's doesn't have synchronization concerns (also, we can move the go to previous position op back to regular state as well, which is actually surprisingly useful to continue to have).

jiangliu · 2021-10-05T12:34:39Z

Hi Gerry, left a couple of additional comments and some small examples. Looking forward to hearing your thoughts.

I guess one last high level comment is whether we should actually split the new interfaces into non-mut and mut parts (i.e. QueueStateT and QueueStateMutT depending on whether the methods need &self or &mut self), as this makes things easier with certain bounds and implementations for smart pointer-like objects (for example). It's prob not super important right now, but it will become more complicated to change as time goes by.

Also, we can ultimately merge this as is and add changes on top afterwards; I think the important thing right now is to get aligned on the direction, and then we can get there in as many steps as we need.

I think it's ok to split the trait into non-mut and mut parts, but I'm not sure about the use cases for a readonly queue:)

For the way to move forward, I prefer to merge this first and then add more changes piece by piece:)

Split out `struct QueueState` from `struct Queue`, which contains all the fields related to virtio queue state. It will be reused to support concurent operations for multi-threading context later. Signed-off-by: Liu Jiang <[email protected]>

The `AvailIter` structure is an iterator to consume all avialable descriptors on the available ring. And Queue::go_to_previous_position() is to go back one step on the available ring, or essentially undo the effect of AvailIter::next(). So let's move go_to_previous_position() to AvailIter. It helps to support multi-threading in future. Signed-off-by: Liu Jiang <[email protected]>

Abstract queue state operations as `trait QueueStateT`, so we could provide different implementations for single-threaded context and multi-threaded context. Signed-off-by: Liu Jiang <[email protected]>

Introduce QueueStateSync, which is essentially Arc<Mutex<QueueState>>, for multi-threaded context. The QueueStateSync object could be cloned and sent to different threads for concurrently processing available descriptors. Signed-off-by: Liu Jiang <[email protected]>

Use atomic operation to access queue.head, to avoid reading inconsistent values. Signed-off-by: Liu Jiang <[email protected]>

Refine documentation a bit. Signed-off-by: Liu Jiang <[email protected]>

jiangliu · 2021-10-13T07:14:26Z

@slp @alexandruag I have rebased to the latest base, so let's move on and get things done step by step:)

lauralt · 2021-10-13T08:25:49Z

Is it possible to add some sort of test for the multi-threading part?

alexandruag

Cool let's merge this and continue to iterate on top of it. From my point of view I'd like to continue with the changes that remove the generic type parameter from QueueState and the proposal to refactor how the sync semantics are provided (i.e. vs the current lock method of QueueStateT). We'll also have to add more testing (like the tests Laura mentioned) after things settle down more, and we'll see at that point how to split the work.

bonzini · 2021-10-26T10:36:46Z

This API makes very little sense, since the lock() method takes a &mut self. If you have a &mut self there's no need to lock anything. You might as well make it get_mut() instead.

bonzini · 2021-10-26T10:42:09Z

Plus, it is horribly inefficient. The synchronization should have been done at the Queue level, not at the QueueState level.

There were a couple of changes in rust-vmm#76 that are not yet tested. PR that covers that functionality is in review here: rust-vmm#111. Until that one is merged, the coverage needs to be decreased. Signed-off-by: Laura Loghin <[email protected]>

There were a couple of changes in #76 that are not yet tested. PR that covers that functionality is in review here: #111. Until that one is merged, the coverage needs to be decreased. Signed-off-by: Laura Loghin <[email protected]>

jiangliu requested review from alexandruag and slp as code owners August 18, 2021 16:31

jiangliu force-pushed the refactor branch 2 times, most recently from 6f42186 to ccd3ae2 Compare August 19, 2021 03:35

jiangliu requested review from acatangiu and sboeuf August 19, 2021 06:09

jiangliu force-pushed the refactor branch from ccd3ae2 to 166b275 Compare August 19, 2021 06:51

alexandruag mentioned this pull request Sep 13, 2021

virtio-queue: Handle address translations #75

Closed

jiangliu force-pushed the refactor branch 4 times, most recently from 22335e7 to f2c196a Compare September 14, 2021 02:34

jiangliu mentioned this pull request Sep 15, 2021

[virtio-queue] Split the code in multiple modules #87

Closed

slp previously approved these changes Sep 15, 2021

View reviewed changes

alexandruag reviewed Sep 15, 2021

View reviewed changes

jiangliu requested a review from alexandruag September 16, 2021 03:10

jiangliu dismissed slp’s stale review via dc0ad32 September 23, 2021 08:41

jiangliu force-pushed the refactor branch from f2c196a to dc0ad32 Compare September 23, 2021 08:41

alexandruag reviewed Oct 4, 2021

View reviewed changes

lauralt mentioned this pull request Oct 12, 2021

move checks from Queue::is_valid to setters #94

Closed

jiangliu added 5 commits October 13, 2021 14:33

queue: split out struct QueueState

8c76c8a

Split out `struct QueueState` from `struct Queue`, which contains all the fields related to virtio queue state. It will be reused to support concurent operations for multi-threading context later. Signed-off-by: Liu Jiang <[email protected]>

queue: introduce traits queue state operations

ca9f381

Abstract queue state operations as `trait QueueStateT`, so we could provide different implementations for single-threaded context and multi-threaded context. Signed-off-by: Liu Jiang <[email protected]>

queue: use atomic to access queue head field

be4f85d

Use atomic operation to access queue.head, to avoid reading inconsistent values. Signed-off-by: Liu Jiang <[email protected]>

queue: refine documentation a bit

9b985ef

Refine documentation a bit. Signed-off-by: Liu Jiang <[email protected]>

jiangliu force-pushed the refactor branch 2 times, most recently from a935d0d to 9b985ef Compare October 13, 2021 06:54

jiangliu requested review from alexandruag and slp October 13, 2021 07:14

alexandruag approved these changes Oct 14, 2021

View reviewed changes

slp approved these changes Oct 15, 2021

View reviewed changes

slp merged commit d5cfd8b into rust-vmm:main Oct 15, 2021

lauralt mentioned this pull request Oct 21, 2021

Remove generic type parameter M from QueueState #104

Merged

bonzini mentioned this pull request Oct 26, 2021

Replace QueueStateSync with QueueSync #111

Closed

jiangliu mentioned this pull request Nov 26, 2021

[virtio-queue] Support multiple working threads per queue #90

Closed

enhance virtio_queue to better support multi-threading #76

enhance virtio_queue to better support multi-threading #76

Conversation

jiangliu commented Aug 18, 2021 • edited Loading

andreeaflorescu commented Aug 19, 2021

jiangliu commented Sep 1, 2021

lauralt commented Sep 1, 2021

slp left a comment

Choose a reason for hiding this comment

alexandruag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangliu commented Sep 16, 2021

alexandruag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangliu commented Oct 5, 2021

jiangliu commented Oct 13, 2021

lauralt commented Oct 13, 2021

alexandruag left a comment

Choose a reason for hiding this comment

bonzini commented Oct 26, 2021

bonzini commented Oct 26, 2021

jiangliu commented Aug 18, 2021 •

edited

Loading