Page MenuHomeFreeBSD

bhyve: support for enabling/disabling the net backend
ClosedPublic

Authored by vmaffione on Jul 16 2019, 4:58 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 18 2024, 2:22 PM
Unknown Object (File)
Feb 10 2024, 3:12 PM
Unknown Object (File)
Jan 29 2024, 8:13 PM
Unknown Object (File)
Jan 29 2024, 8:13 PM
Unknown Object (File)
Jan 29 2024, 8:13 PM
Unknown Object (File)
Jan 29 2024, 8:13 PM
Unknown Object (File)
Jan 29 2024, 8:13 PM
Unknown Object (File)
Dec 20 2023, 3:20 AM

Details

Summary

Extend the net backend interface with two functions, namely netbe_rx_disable()
and netbe_rx_enable(), which can be used by the net device emulators to stop
the backend from invoking the receive callback. This is useful for the device emulators,
i.e. on hardware resets or to implement receive backpressure.
The mevent module has been extended to support the addition of a disabled event.

To prevent race conditions, the net backends will start with receive operation disabled.
A follow-up patch will use the new functionalities in the virtio-net device.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

pmooney_pfmooney.com added inline comments.
usr.sbin/bhyve/block_if.c
397 ↗(On Diff #59820)

Why not use an enum or #defines to provide names, rather than adding the accompanying comment to explain the param at all the call sites?

usr.sbin/bhyve/block_if.c
397 ↗(On Diff #59820)

No objections on that. Another options would be to add another function mevent_add_disabled(), to minimize the patch size.

afedorov added inline comments.
usr.sbin/bhyve/block_if.c
397 ↗(On Diff #59820)

+1 to mevent_add_disabled()

vmaffione edited the summary of this revision. (Show Details)

Start the backends with receive enabled for the moment being.

minimize patch by adding mevent_add_disabled().

I tested this patch in various configurations using iperf3:

  • VM (Freebsd-12) - vale - VM(Freebsd-12)
  • VM (Freebsd-13) - vale - VM(Freebsd-13)
  • VM (Ubuntu 16.04) - vale - VM(ubuntu 16.04)
  • VM (Ubuntu 16.04) - vale - VM(Freebsd-12)

The same variation in guest OS's, but using VM - vale - igb --- igb - vale - VM

And with if_bridge(4) + if_tuntap(4) with similar configuration on the same host and between two hosts.

I did not find any problems.

So I don't see any uses of mevent_add_disabled() in this change? If that is used in a future change it might be best to defer that to the future change and just add the new hook(s) in the backend interface for now?

usr.sbin/bhyve/mevent.c
257

Maybe 'mevent_add_state' and make 'int enabled' be 'int state' that you assign to me_state directly. This removes the need for the comments as you then have:

mevent_add()
{
   mevent_add_state(..., MEV_ADD)
}

mevent_add_disabled(...)
{
   mevent_add_state(..., MEV_ADD_DISABLED)
}
usr.sbin/bhyve/net_backends.c
131

Maybe use a 'bool enable' instead of 'int state' as then the code reads a bit more obviously in backends, e.g.

if (enable)
    mevent_enable(...);
else
    mevent_disable(...);

I kind of wish there was a better name of the function pointer than 'recv_enable' as it's also used to disable. I honestly wouldn't mind if you just had two callbacks 'recv_enable' and 'recv_disable' and dropped the 'state' parameter entirely. I think that would ultimately be the most readable.

In D20973#471364, @jhb wrote:

So I don't see any uses of mevent_add_disabled() in this change?

This review is related to D20987

vmaffione added inline comments.
usr.sbin/bhyve/mevent.c
257

Good idea, thanks.

usr.sbin/bhyve/net_backends.c
131

I agree it is more readable the way you suggest. Some people prefer the other way, though.

Regarding mevent_add_disabled, I can defer that to the next patch.

This revision is now accepted and ready to land.Sep 27 2019, 5:54 PM