Re: RFC: vioif(4) multiqueue support

[s ymgch <s.ymgch228%gmail.com@localhost>]

Thank you for your helpful comments!


2018年12月26日(水) 11:37 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.

Yes, indeed. I'll wait for other comments for more one or two week.

>
> > 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?

The reason why I don't use "struct virtqueue" but use "struct virtqueue *"
is to register a virtqueue array ("struct virtqueue []") to virtio(4)
as an argument
of virtio_child_attach_start() or virtio_child_attach_set_vqs(). Virtqueues used
in a child device of virtio(4) like vioif(4) must be array.

>
> 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.

It is for the error handling code.
Example:
    for (...) {
        txq[i]->txq_lock = mutex_obj_alloc();
        s = softint_establish()
        if (s == NULL)
            goto err;
    }
    ....
    err:
        for (...)
            if (txq[i]->txq_lock)
                mutex_obj_free(txq->txq_lock);

I use the pointer value as a flag weather the lock already allocated or not, and
I didn't want to add a field into vioif_txqueue to save it.

>
> 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?

I'll add comments for the locks.
Currently, the locks cover all fields in the structure, and two or more
than locks can't be held at once.

>
> > +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.)

nvq_pairs is always less than VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX(= 0x8000).
So, I'll add KASSERT(nvq_pairs <=  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX).

>
> > @@ -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.

I'll take in other comments to the patch.

Thanks,
Yamaguchi