Re: RFC: vioif(4) multiqueue support

[Taylor R Campbell <campbell+netbsd-tech-net%mumble.net@localhost>]

> Date: Wed, 26 Dec 2018 10:03:15 +0900
> From: Shoichi Yamaguchi <yamaguchi%netbsd.org@localhost>
> 
> > I implemented a patch that make vioif(4) support multi-queue. And I have put
> > the patch on ftp.n.o. I used vioif(4) multiqueue on qemu-kvm on Linux kernel
> > 4.19.5. And It seems to be working fine.
> >
> > https://ftp.netbsd.org/pub/NetBSD/misc/yamaguchi/vioif_mutilq.patch
> 
> Do you have any comments?
> I would like to going to commit the patch if there is no comment until tomorrow.

Hi, Yamaguchi-san!  A lot of Americans and Europeans are on vacation
this time of year, so it might be better to hold off for another week
or two.  Here's a quick review -- I don't know anything about virtio,
so this is just about use of kernel APIs and abstractions.  Someone
who knows something about virtio should take a look too.

> diff --git a/sys/dev/pci/if_vioif.c b/sys/dev/pci/if_vioif.c
> index 3bbd300e88e..769b108e793 100644
> --- a/sys/dev/pci/if_vioif.c
> +++ b/sys/dev/pci/if_vioif.c
>  
>  /* Feature bits */
> -#define VIRTIO_NET_F_CSUM	(1<<0)
> [...]
> +#define VIRTIO_NET_F_MQ			(1<<22)

If you're going to modify all of the lines here, maybe take the
opportunity to convert them to use __BIT?

> @@ -171,73 +184,110 @@ struct virtio_net_ctrl_vlan {
> [...]
>  /*
>   * if_vioifvar.h:
>   */
> +struct vioif_txqueue {
> +	struct virtqueue	*txq_vq;

Why not make this an embedded structure?

struct vioif_txqueue {
	struct virtqueue	txq_vq;
	...
};

struct vioif_softc {
	struct vioif_txqueue	*sc_txq;
	struct vioif_rxqueue	*sc_rxq;
	struct vioif_ctrlqueue	*sc_ctrlq;
	...
};

> +	kmutex_t		*txq_lock;

Why is this a pointer to kmutex_t with mutex_obj_alloc/free and not
just a kmutex_t with mutex_init/destroy?  Is it reused anywhere?  If
it is reused, this needs explanation in the comments.  If it is not,
just use kmutex_t.

Can you write a comment summarizing what locks cover what fields, and,
if more than one lock can be held at once, what the lock order is?

> +struct vioif_rxqueue {
> +	struct virtqueue	*rxq_vq;
> +	kmutex_t		*rxq_lock;

Likewise.

> -#define VIOIF_TX_LOCK(_sc)	mutex_enter(&(_sc)->sc_tx_lock)
> -#define VIOIF_TX_UNLOCK(_sc)	mutex_exit(&(_sc)->sc_tx_lock)
> -#define VIOIF_TX_LOCKED(_sc)	mutex_owned(&(_sc)->sc_tx_lock)
> -#define VIOIF_RX_LOCK(_sc)	mutex_enter(&(_sc)->sc_rx_lock)
> -#define VIOIF_RX_UNLOCK(_sc)	mutex_exit(&(_sc)->sc_rx_lock)
> -#define VIOIF_RX_LOCKED(_sc)	mutex_owned(&(_sc)->sc_rx_lock)
> +#define VIOIF_TXQ_LOCK(_q)	mutex_enter((_q)->txq_lock)
> +#define VIOIF_TXQ_TRYLOCK(_q)	mutex_tryenter((_q)->txq_lock)
> +#define VIOIF_TXQ_UNLOCK(_q)	mutex_exit((_q)->txq_lock)
> +#define VIOIF_TXQ_LOCKED(_q)	mutex_owned((_q)->txq_lock)
> +
> +#define VIOIF_RXQ_LOCK(_q)	mutex_enter((_q)->rxq_lock)
> +#define VIOIF_RXQ_UNLOCK(_q)	mutex_exit((_q)->rxq_lock)
> +#define VIOIF_RXQ_LOCKED(_q)	mutex_owned((_q)->rxq_lock)

Can we just use mutex_enter/exit/&c. without the macros?  Sometimes we
use macros where they are conditional, depending on NET_MPSAFE, but if
there's no need for that, I would prefer to read direct calls to
mutex_enter/exit/&c.

> +static int
> +vioif_alloc_queues(struct vioif_softc *sc)
> +{
> +	int nvq_pairs = sc->sc_max_nvq_pairs;
> +	int nvqs = nvq_pairs * 2;
> +	int i;
> +
> +	sc->sc_rxq = kmem_zalloc(sizeof(sc->sc_rxq[0]) * nvq_pairs,
> +	    KM_NOSLEEP);
> +	if (sc->sc_rxq == NULL)
> +		return -1;
> +
> +	sc->sc_txq = kmem_zalloc(sizeof(sc->sc_txq[0]) * nvq_pairs,
> +	    KM_NOSLEEP);
> +	if (sc->sc_txq == NULL)
> +		return -1;

Check to avoid arithmetic overflow here:

	if (nvq_pairs > INT_MAX/2 - 1 ||
	    nvq_pairs > SIZE_MAX/sizeof(sc->sc_rxq[0]))
		return -1;
	nvqs = nvq_pairs * 2;
	if (...) nvqs++;
	sc->sc_rxq = kmem_zalloc(sizeof(sc->sc_rxq[0]) * nvq_pairs, ...);

Same in all the other allocations like this.  (We should have a
kmem_allocarray -- I have a draft somewhere.)

> @@ -586,69 +759,109 @@ vioif_attach(device_t parent, device_t self, void *aux)
> [...]
> +		/* Limit the number of queue pairs to use */
> +		if (sc->sc_max_nvq_pairs <= ncpu)
> +			sc->sc_req_nvq_pairs = sc->sc_max_nvq_pairs;
> +		else
> +			sc->sc_req_nvq_pairs = ncpu;

How about sc->sc_req_nvq_pairs = MIN(sc->sc_max_nvq_pairs, ncpu)?

> +static void
> +vioif_ctrl_release(struct vioif_softc *sc)
> +{
> +	struct vioif_ctrlqueue *ctrlq = &sc->sc_ctrlq;
> +
> +	mutex_enter(&ctrlq->ctrlq_wait_lock);

KASSERT(ctrlq->ctrlq_inuse != FREE)

Might be helpful to record the lwp that owns this ctrlq, too, for
diagnostics: KASSERT(ctrlq->ctrlq_owner == curlwp).

> diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c
> index 65c5222b774..bb972997be2 100644
> --- a/sys/dev/pci/virtio_pci.c
> +++ b/sys/dev/pci/virtio_pci.c
> @@ -604,8 +677,14 @@ virtio_pci_setup_interrupts(struct virtio_softc *sc)
> [...]
>  	if (pci_intr_type(pc, psc->sc_ihp[0]) == PCI_INTR_TYPE_MSIX) {
> -		psc->sc_ihs = kmem_alloc(sizeof(*psc->sc_ihs) * 2,
> +		psc->sc_ihs = kmem_alloc(sizeof(*psc->sc_ihs) * nmsix,
>  		    KM_SLEEP);

Check for arithmetic overflow here.