Split virtio-queue into multiple submodules for maintenance #120

jiangliu · 2021-11-21T03:37:52Z

Fix #87 by split the lib.rs of virtio-queue into multiple submodules:

split it into several submodules for maintenance
use setter/getter to avoid direct access to data structure fields
refine unit test cases to improve code coverage, now it reaches 90% code coverage.

Refine doc for constant definitions to make them symmetric. Signed-off-by: Liu Jiang <[email protected]>

Move Descriptor to a dedicated file as a submodule. Also avoid direct accesses to Descriptor's fields. Signed-off-by: Liu Jiang <[email protected]>

Move DescriptorChain/DescriptorChainRwIter to a dedicated file as a submodule. Signed-off-by: Liu Jiang <[email protected]>

Move VirtqUsedElem into descriptor.rs and add more unit test cases. Signed-off-by: Liu Jiang <[email protected]>

Split `struct AvialIter` into a submodule. Signed-off-by: Liu Jiang <[email protected]>

bonzini · 2021-11-21T11:09:06Z

The QueueStateSync API is absolutely not ready for being published.

Split `struct QueueState` into a submodule. Signed-off-by: Liu Jiang <[email protected]>

Split `struct QueueStateSync` into a submodule. Signed-off-by: Liu Jiang <[email protected]>

Split `struct Queue` into a submodule. Signed-off-by: Liu Jiang <[email protected]>

Change unit test cases to directly use Queue methods. Signed-off-by: Liu Jiang <[email protected]>

Add unit test cases for QueueStateSync to show the sample usage and improve code coverage. Signed-off-by: Liu Jiang <[email protected]>

Adjust code coverage for x86_64. Also modify git ignore file to ignore .idea directories. Signed-off-by: Liu Jiang <[email protected]>

jiangliu · 2021-11-21T15:35:12Z

The QueueStateSync API is absolutely not ready for being published.

Well formed API designs are always welcomed:)
But we hope we could publish the virtio-queue crate sooner than later, because several other projects depend on it. The situation has last more than one year. We have tried several times during the past year to migrate to the upstream vm-virtio implementation but failed. Then the cloud hypervisor, virtiofsd and our projects turned to maintain their own private versions, it's really a burden for us:(

jiangliu · 2021-11-21T16:14:30Z

The QueueStateSync API is absolutely not ready for being published.

BTW, I have one more patch to convert QueueStateGuard from an enum into a trait, just as one of your previous patch did. It does help to make the API clear and extensible:)

bonzini · 2021-11-21T21:18:32Z

The problem is that Queue is not solving the problem in a way that works for the vhost-backend crate for example.

I think we don't even need Queue or QueueSync. What we need is a good guard object so that one doesn't have to use the flexible API with GuestMemory arguments, when the easier one can be used.

In addition, signal_used_queue must definitely be a trait and the guard should know about it. I'll try to come up with something later this week.

jiangliu · 2021-11-22T02:28:56Z

The problem is that Queue is not solving the problem in a way that works for the vhost-backend crate for example.

Actually we need to support several use cases:

support normal virtio devices, such as virtio-blk/virtio-net
support vhost daemons with vhost-user-backend
support rust async io to improve performance

I think we don't even need Queue or QueueSync. What we need is a good guard object so that one doesn't have to use the flexible API with GuestMemory arguments, when the easier one can be used.

Is this something you are thinking of? #121 It introduces two changes:

convert enum QueueStateGuard to trait QueueStateGuard for extensibility and zero-cost abstraction.
introduce "struct QueueGuard` to encapsulate a QueueStateGuard object and associated guest memory object.

In addition, signal_used_queue must definitely be a trait and the guard should know about it. I'll try to come up with something later this week.

alexandruag

This def looks good as breaking up the code into separate modules is something that's been long overdue. Thanks Gerry! Left just one comment regarding some of the tests.

alexandruag · 2021-11-25T09:56:02Z

crates/virtio-queue/src/state_sync.rs

+}
+
+#[cfg(test)]
+mod tests {


Hmm, looks like most of the tests here would apply to other QueueStateT implementations just as well. It would actually be great to have a battery of tests for the QueueStateT interface that can then be reused to test all implementations (so far there's QueueState and QueueStateSync), and then separate tests for stuff that's specific to QueueStateSync in particular (altho there's not that much truly specific stuff IMO; even the test where multiple threads are spawned can be seen as generic due to lock being part of the high level interface).

Not sure exactly just how much effort that would require, so maybe doing it as part of this PR is not the best approach, but then I think we should at least have an issue and tackle it b4 publishing.

Will open one issue for it once the PR get merged:)

alexandruag

Cool, I think the refactoring here is def helpful while we work to wrap up the other ongoing discussions.

slp

LGTM. Thanks @jiangliu

queue: refine doc for constant definitions

fc7223b

Refine doc for constant definitions to make them symmetric. Signed-off-by: Liu Jiang <[email protected]>

jiangliu requested review from alexandruag and slp as code owners November 21, 2021 03:37

jiangliu force-pushed the refine branch from db2c9e0 to 9ecdcdc Compare November 21, 2021 05:08

jiangliu added 4 commits November 21, 2021 13:11

queue: move Descriptor to submodule

56552d2

Move Descriptor to a dedicated file as a submodule. Also avoid direct accesses to Descriptor's fields. Signed-off-by: Liu Jiang <[email protected]>

queue: move DescriptorChain to submodule

0840711

Move DescriptorChain/DescriptorChainRwIter to a dedicated file as a submodule. Signed-off-by: Liu Jiang <[email protected]>

queue: move VirtqUsedElem into descriptor.rs

85449b8

Move VirtqUsedElem into descriptor.rs and add more unit test cases. Signed-off-by: Liu Jiang <[email protected]>

queue: split struct AvailIter into submodule

e2b463e

Split `struct AvialIter` into a submodule. Signed-off-by: Liu Jiang <[email protected]>

jiangliu force-pushed the refine branch from 9ecdcdc to bb01ace Compare November 21, 2021 11:29

jiangliu added 4 commits November 21, 2021 21:24

queue: split struct QueueState into submodule

0a13baf

Split `struct QueueState` into a submodule. Signed-off-by: Liu Jiang <[email protected]>

queue: split struct QueueStateSync into submodule

4a5762a

Split `struct QueueStateSync` into a submodule. Signed-off-by: Liu Jiang <[email protected]>

queue: split struct Queue into submodule

7bc3e63

Split `struct Queue` into a submodule. Signed-off-by: Liu Jiang <[email protected]>

queue: change unit test cases to use Queue methods

d3bdd0b

Change unit test cases to directly use Queue methods. Signed-off-by: Liu Jiang <[email protected]>

jiangliu force-pushed the refine branch from bb01ace to d3bdd0b Compare November 21, 2021 13:25

queue: add unit test cases for QueueStateSync

9caaec5

Add unit test cases for QueueStateSync to show the sample usage and improve code coverage. Signed-off-by: Liu Jiang <[email protected]>

jiangliu force-pushed the refine branch 2 times, most recently from d22bcee to 1df099c Compare November 21, 2021 14:52

queue: adjust code coverage and git ignore file

d86793a

Adjust code coverage for x86_64. Also modify git ignore file to ignore .idea directories. Signed-off-by: Liu Jiang <[email protected]>

jiangliu force-pushed the refine branch from 1df099c to d86793a Compare November 21, 2021 15:09

jiangliu requested review from andreeaflorescu, bonzini and sboeuf November 21, 2021 15:36

jiangliu mentioned this pull request Nov 21, 2021

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

Closed

jiangliu changed the title ~~[WIP] Prepare virtio-queue for publishing~~ Split virtio-queue into multiple submodules for maintenance Nov 21, 2021

jiangliu force-pushed the refine branch from 465c773 to d86793a Compare November 22, 2021 02:18

alexandruag reviewed Nov 25, 2021

View reviewed changes

alexandruag approved these changes Nov 26, 2021

View reviewed changes

slp approved these changes Nov 26, 2021

View reviewed changes

slp merged commit d991cd2 into rust-vmm:main Nov 26, 2021

This was referenced Nov 26, 2021

Unify unit test cases of QueueState and QueueStateSync #122

Open

Refine QueueStateGuard and introduce QueueGuard #121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Split virtio-queue into multiple submodules for maintenance #120

Split virtio-queue into multiple submodules for maintenance #120

Uh oh!

jiangliu commented Nov 21, 2021 •

edited

Loading

Uh oh!

bonzini commented Nov 21, 2021

Uh oh!

jiangliu commented Nov 21, 2021

Uh oh!

jiangliu commented Nov 21, 2021

Uh oh!

bonzini commented Nov 21, 2021

Uh oh!

jiangliu commented Nov 22, 2021

Uh oh!

alexandruag left a comment

Uh oh!

alexandruag Nov 25, 2021

Uh oh!

jiangliu Nov 25, 2021

Uh oh!

alexandruag left a comment

Uh oh!

slp left a comment

Uh oh!

Uh oh!

Split virtio-queue into multiple submodules for maintenance #120

Split virtio-queue into multiple submodules for maintenance #120

Uh oh!

Conversation

jiangliu commented Nov 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bonzini commented Nov 21, 2021

Uh oh!

jiangliu commented Nov 21, 2021

Uh oh!

jiangliu commented Nov 21, 2021

Uh oh!

bonzini commented Nov 21, 2021

Uh oh!

jiangliu commented Nov 22, 2021

Uh oh!

alexandruag left a comment

Choose a reason for hiding this comment

Uh oh!

alexandruag Nov 25, 2021

Choose a reason for hiding this comment

Uh oh!

jiangliu Nov 25, 2021

Choose a reason for hiding this comment

Uh oh!

alexandruag left a comment

Choose a reason for hiding this comment

Uh oh!

slp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jiangliu commented Nov 21, 2021 •

edited

Loading