Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dragonball: Support vhost-user-net device #8503

Merged
merged 3 commits into from Dec 25, 2023
Merged

dragonball: Support vhost-user-net device #8503

merged 3 commits into from Dec 25, 2023

Conversation

justxuewei
Copy link
Member

This PR introduces vhost-user-net devices to Dragonball. The devices are
allowed to run as server on the VMM side.

Fixes: #8502

Signed-off-by: Eric Ren renzhen@linux.alibaba.com
Signed-off-by: Liu Jiang gerry@linux.alibaba.com
Signed-off-by: Zha Bin zhabin@linux.alibaba.com
Signed-off-by: Chao Wu chaowu@linux.alibaba.com
Signed-off-by: Zizheng Bian zizheng.bian@linux.alibaba.com
Signed-off-by: Xuewei Niu niuxuewei.nxw@antgroup.com

@katacontainersbot katacontainersbot added the size/huge Largest and most complex task (probably needs breaking into small pieces) label Nov 23, 2023
@justxuewei
Copy link
Member Author

Hi @studychao @ZizhengBian @lifupan, vhost-user-net device has been tested with DPDK. Please take a look at this. Thanks!

liubogithub
liubogithub reviewed Dec 8, 2023
View reviewed changes
src/dragonball/src/dbs_virtio_devices/src/vhost/net.rs
match mem.read_slice(&mut buf, desc.addr()) {
Ok(_) => unsafe { Ok(std::ptr::read_volatile(&buf[..] as *const _ as *const T)) },
Err(err) => {
error!("Failed to read from memory, {}", err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the error msg here too generic? It's difficult to tell if it's vhost-net related.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is used to read an object from memory, but its type is generic without implementing Debug trait. No more details are able to retrieve. To get more info, a possible way is to add a context("extra info here") when calling this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UPDATE 1: anyhow crate isn't used by Dragonball. So the context is unavailable. Then extra info is required to pass from parameters, which I think it isn't a elegant way.

liubogithub
liubogithub reviewed Dec 8, 2023
View reviewed changes
src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_user/net.rs Outdated Show resolved Hide resolved
src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_user/net.rs Show resolved Hide resolved
src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_user/net.rs
}
}
Err(err) => {
error!("{}: failed to setup connection: {}", self.id, err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above two error messages are identical, making it difficult to figure out where goes wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VhostUserProtocol error branch is to try to reconnect. Others should be one type of errors: print error log and stop performing this function. Sharing one error message is acceptable in my perspective.

src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_user/net.rs Show resolved Hide resolved
src/dragonball/src/device_manager/vhost_user_net_dev_mgr.rs Outdated Show resolved Hide resolved
@justxuewei justxuewei mentioned this pull request Dec 11, 2023
Merged
@justxuewei justxuewei force-pushed the vhost-user-net branch 4 times, most recently from 0543fcb to a353a2d Compare December 14, 2023 06:25
@justxuewei
Copy link
Member Author

Hi @liubogithub, I've pushed a new commit which resolve issues you mentioned in your code review. PTAL. Thanks!

@justxuewei justxuewei force-pushed the vhost-user-net branch 9 times, most recently from 47439af to 6e2d8fb Compare December 20, 2023 07:08
studychao
studychao reviewed Dec 20, 2023
View reviewed changes
Copy link
Member

@studychao studychao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, a few comments

src/dragonball/src/dbs_virtio_devices/src/lib.rs Outdated
@@ -364,6 +367,10 @@ pub mod tests {
pub const VIRTQ_DESC_F_NEXT: u16 = 0x1;
pub const VIRTQ_DESC_F_WRITE: u16 = 0x2;

pub(crate) const GUEST_PHYS_END: u64 = (1 << 46) - 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you use GUEST_PHYS_END, GUEST_MEM_START and GUEST_MEM_END defined in dbs_boot?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

src/dragonball/src/device_manager/vhost_user_net_dev_mgr.rs
Arc::new(cfg.queue_sizes()),
epoll_mgr.clone(),
)?)),
None => Err(VirtioError::InvalidInput),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about making this error information more explicit like NoEpollMgr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's great. However, other device managers, e.g. VirtioNetDeviceMgr, are still using the InvalidInput as well. Shall we file an issue to track this down and fix this in another PR?

src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_user/net.rs Outdated Show resolved Hide resolved
src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_user/net.rs Outdated Show resolved Hide resolved
src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_user/net.rs Outdated Show resolved Hide resolved
src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_user/net.rs Outdated Show resolved Hide resolved
src/dragonball/src/dbs_virtio_devices/src/vhost/vhost_user/net.rs Outdated Show resolved Hide resolved
studychao
studychao reviewed Dec 20, 2023
View reviewed changes
src/dragonball/src/dbs_virtio_devices/src/balloon.rs
let config = VirtioDeviceConfig::<Arc<GuestMemoryMmap<()>>>::new(
Arc::new(mem),
address_space,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you separate all addreess_space related change to a different commit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've slipt that changes into a separate commit.

@justxuewei justxuewei force-pushed the vhost-user-net branch 2 times, most recently from 34866a8 to 0d8312e Compare December 20, 2023 10:13
@justxuewei justxuewei force-pushed the vhost-user-net branch 2 times, most recently from c1a1c72 to de9af1b Compare December 21, 2023 02:54
@justxuewei justxuewei mentioned this pull request Dec 21, 2023
Closed
@justxuewei justxuewei force-pushed the vhost-user-net branch 3 times, most recently from 8cceae0 to 104d751 Compare December 21, 2023 10:42
@justxuewei
Copy link
Member Author

Hi @studychao, I've addressed the issues mentioned in your comments. Could you take a look at this again? Thanks!

studychao
studychao approved these changes Dec 21, 2023
View reviewed changes
Copy link
Member

@studychao studychao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@justxuewei
Copy link
Member Author

/test

@justxuewei
dragonball: Introduce address space for MmioV2DeviceState
1f21d3c 
Vhost-user-net has a dependency on address space from `MmioV2DeviceState`.
The addition of the address space is introduced in this patch. Plus, it
makes sure all unit tests have the according parameter as well.

Fixes: kata-containers#8502

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
@justxuewei
dragonball: Support vhost-user-net devices
beadce5 
This PR introduces vhost-user-net devices to Dragonball. The devices are
allowed to run as server on the VMM side.

Fixes: kata-containers#8502

Signed-off-by: Eric Ren <renzhen@linux.alibaba.com>
Signed-off-by: Liu Jiang <gerry@linux.alibaba.com>
Signed-off-by: Zha Bin <zhabin@linux.alibaba.com>
Signed-off-by: Chao Wu <chaowu@linux.alibaba.com>
Signed-off-by: Zizheng Bian <zizheng.bian@linux.alibaba.com>
Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
@justxuewei justxuewei force-pushed the vhost-user-net branch 3 times, most recently from 88d797d to e6c1868 Compare December 22, 2023 07:14
@justxuewei justxuewei changed the title dragonball: Support vhost-user-net devices dragonball: Support vhost-user-net device Dec 22, 2023
@justxuewei
Copy link
Member Author

/test

@justxuewei
dragonball: Wrap config space into set_config_space
4c5de72 
Config space of network device is shared and accord with virtio 1.1 spec.
It is a good way to abstract the common part into one function.
`set_config_space()` implements this.

Plus, this patch removes `vq_pairs` from vhost-net devices, since there is
a possibility of data inconsistency. For example, some places read that
from `self.vq_pairs`, others read from `queue_sizes.len() / 2`.

Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
@justxuewei
Copy link
Member Author

/test

@justxuewei justxuewei merged commit 38eb407 into kata-containers:main Dec 25, 2023
166 of 176 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liubogithub liubogithub liubogithub left review comments

@lifupan lifupan lifupan approved these changes

@studychao studychao studychao approved these changes

Assignees
No one assigned
Labels
ok-to-test size/huge Largest and most complex task (probably needs breaking into small pieces)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dragonball: Support vhost-user-net devices
5 participants
@justxuewei @liubogithub @lifupan @studychao @katacontainersbot