Page MenuHomeFreeBSD

bhyve: support for enabling/disabling the net backend
ClosedPublic

Authored by vmaffione on Jul 16 2019, 4:58 PM.

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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vmaffione created this revision.Jul 16 2019, 4:58 PM
vmaffione edited the summary of this revision. (Show Details)Jul 16 2019, 5:01 PM
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?

vmaffione added inline comments.Jul 16 2019, 5:32 PM
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.

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

+1 to mevent_add_disabled()

vmaffione updated this revision to Diff 59826.Jul 16 2019, 7:16 PM
vmaffione edited the summary of this revision. (Show Details)

Start the backends with receive enabled for the moment being.

vmaffione updated this revision to Diff 59827.Jul 16 2019, 7:28 PM

minimize patch by adding mevent_add_disabled().

vmaffione updated this revision to Diff 59828.Jul 16 2019, 7:31 PM

Upload the right patch

vmaffione updated this revision to Diff 59892.Jul 18 2019, 8:21 PM

Removed traling space.

Any opinions on this patch?

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.

Thanks a lot!

jhb added a comment.Sep 13 2019, 4:26 AM

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 ↗(On Diff #59892)

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
120 ↗(On Diff #59892)

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 updated this revision to Diff 62123.Sep 15 2019, 2:08 PM

Addressed jhb's comments.

vmaffione marked 2 inline comments as done.Sep 15 2019, 2:10 PM
vmaffione added inline comments.
usr.sbin/bhyve/mevent.c
257 ↗(On Diff #59892)

Good idea, thanks.

usr.sbin/bhyve/net_backends.c
120 ↗(On Diff #59892)

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

vmaffione marked 2 inline comments as done.Sep 15 2019, 2:10 PM

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

Does it look good now?

jhb accepted this revision.Sep 27 2019, 5:54 PM

Thanks

This revision is now accepted and ready to land.Sep 27 2019, 5:54 PM
This revision was automatically updated to reflect the committed changes.