bhyve: missing mutex initialisations

[andy_omniosce.org]

Summary

There are three PCI virtio modules which never initialise the mutex
that's passed back to virtio.

[chuck]

LGTM

[rew]

looking at the comment above the struct virtio_softc definition in usr.sbin/bhyve/virtio.h:

[ ... snipped ... ]
* Note: inside each hypervisor virtio driver, changes to these
* data structures must be locked against other threads, if any.
* Except for PCI config space register read/write, we assume each
* driver does the required locking, but we need a pointer to the
* lock (if there is one) for PCI config space read/write ops.
[ ... snipped ... ]

suggests that vs_mtx is not required to be passed back to virtio - so I'm curious why these ones were omitted, was it by accident or purpose?

looking at virtio-scsi it creates threads in the driver, but virtio-rnd doesn't. So looking at virtio-rnd, what threads does it need the mutex for? The vcpu threads?

[andy_omniosce.org]

Two more missing mutex initialisations.

[andy_omniosce.org]

In D34372#778732, @rew wrote:

suggests that vs_mtx is not required to be passed back to virtio - so I'm curious why these ones were omitted, was it by accident or purpose?

looking at virtio-scsi it creates threads in the driver, but virtio-rnd doesn't. So looking at virtio-rnd, what threads does it need the mutex for? The vcpu threads?

It was just the initialisation that was omitted. Each of these drivers has a mutex in their soft state and passes it back to virtio, which uses it around the bar reads and writes.
With the changes I'm making in illumos, locking an uninitialised mutex is fatal there, hence this showed up.

If the modules don't need the mutex, then removing it completely would work too.

[rew]

I didn't realize that these mutex's get initialized on FreeBSD, even without your patch.

so, yea, I think this looks good - for FreeBSD there is no functional change here.

usr.sbin/bhyve/pci_virtio_rnd.c

185

without your patch applied:

I thought that sc->vsrc_vs.vs_mtx == NULL after this line, but its not.

in usr.sbin/bhyve/virtio.c, we have checks like:

if (vs->vs_mtx) 
        pthread_mutex_lock(vs->vs_mtx);

These checks evaluate as true and looking at pthread_mutex_lock()...it looks like it will initialize the mutex if it isn't already.

[andy_omniosce.org]

In D34372#778858, @rew wrote:

I didn't realize that these mutex's get initialized on FreeBSD, even without your patch.

I didn't either, but it seems that the rest of bhyve is not relying on that behaviour and it's not defined by POSIX nor in the pthread_mutex_lock.3 man page.

That would explain why the other part of this change, pci_nvme.c failing to initialise the last queue mutexes, hasn't shown up earlier either. With these changes in place on illumos, and running with full mutex checking, I have now run a wide range of guests without hitting any more issues.

Thanks

[chuck]

In D34372#778949, @andy_omniosce.org wrote:

In D34372#778858, @rew wrote:

I didn't realize that these mutex's get initialized on FreeBSD, even without your patch.

I didn't either, but it seems that the rest of bhyve is not relying on that behaviour and it's not defined by POSIX nor in the pthread_mutex_lock.3 man page.

Perhaps I'm getting confused. Are we saying on FreeBSD mutex's are initialized automatically, or in this particular case? If so, where/how does the initialization happen?

That would explain why the other part of this change, pci_nvme.c failing to initialise the last queue mutexes, hasn't shown up earlier either. With these changes in place on illumos, and running with full mutex checking, I have now run a wide range of guests without hitting any more issues.

<smacks forehead/> yup, NVMe change looks good.

[rew]

In D34372#779008, @chuck wrote:

Perhaps I'm getting confused. Are we saying on FreeBSD mutex's are initialized automatically, or in this particular case? If so, where/how does the initialization happen?

yea, on FreeBSD, the mutexes without explicit initialization get initialized regardless.

The mutex initialization is happening where we make the checks in usr.sbin/bhyve/virtio.c:

if (vs->vs_mtx) 
        pthread_mutex_lock(vs->vs_mtx);

vs->vs_mtx evaluates to true (even when the mutex is not initialized) and then when pthread_mutex_lock() is called, it will initialize the mutex if it wasn't previously initialized.

If I'm not mistaken, we can toss those checks along with VS_LOCK()/VS_UNLOCK(), but that would be a separate change.

[jhb]

I suspect this is working by accident on FreeBSD as PTHREAD_MUTEX_INITIALIZER (which is in POSIX) happens to be 0 so the same thing you get from calloc().

[andy_omniosce.org]

Great, thanks all. Is there anything else I need to do to get this merged?

[rew]

I’ll commit this tomorrow.