Re: RFC: vioif(4) multiqueue support

[Shoichi Yamaguchi <yamaguchi%netbsd.org@localhost>]

> > > > https://ftp.netbsd.org/pub/NetBSD/misc/yamaguchi/vioif_mutilq.patch

I have updated the patch.


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