> Date: Wed, 26 Dec 2018 10:03:15 +0900
> From: Shoichi Yamaguchi
>
> > 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 virtqueuetxq_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/ 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/
> +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_ctrlq;
> +
> + mutex_enter(>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
> +++
