On Tue, Nov 22, 2016 at 09:45:30PM +0100, Mike Belopuhov wrote:
> vmd hackers and users seem to be able to trigger the assertion
> below every time they do down and then up (with a dhclient for
> instance):
>
> panic: kernel diagnostic assertion "m != NULL" failed: file
> "/usr/src/sys/dev/pci/if_vio.c", line 1008
> Starting stack trace...
> panic() at panic+0x10b
> __assert() at __assert+0x25
> vio_rxeof() at vio_rxeof+0x1db
> vio_rx_intr() at vio_rx_intr+0x28
> virtio_check_vqs() at virtio_check_vqs+0x8c
> virtio_pci_legacy_intr() at virtio_pci_legacy_intr+0x71
> intr_handler() at intr_handler+0x67
> Xintr_legacy7() at Xintr_legacy7+0xdd
> --- interrupt ---
>
> The culprit is that vio_rx_drain and vio_tx_drain functions
> are not properly implemented: they m_freem mbufs attached to
> ring slots but do nothing about unconfiguring the backend
> virtqueue. This results in us thinking that the ring is
> empty when we UP the interface the next time, but then a
> completion event arrives for the slot that doesn't have an
> associated mbuf anymore.
>
> To fix this properly a virtqueue draining code has to be
> implemented, possibly based on what FreeBSD has. However,
> there's nothing really wrong with leaving the rings the
> way they are and "resuming" on UP, vio doesn't destroy
> them anyway.
>
> The other part of the diff is removing virtio_reinit_start
> and virtio_negotiate_features. I'm not sure why does it
> make sense to re-negotiate features on stop, but reinit
> functions mess around the receive ring and if I just leave
> them there, the interface will not be operational on UP.
>
> So far this has been tested under vmd by rzalamena@ and
> reyk@ but it would be nice to get it tested under the KVM.
>
This has been on my to-do list to look at for a while, thanks a ton
mikeb for tackling this.
-ml
>
>
> diff --git sys/dev/pci/if_vio.c sys/dev/pci/if_vio.c
> index 97af2a2..96b5fdf 100644
> --- sys/dev/pci/if_vio.c
> +++ sys/dev/pci/if_vio.c
> @@ -596,10 +596,13 @@ vio_attach(struct device *parent, struct device *self,
> void *aux)
> ifmedia_set(&sc->sc_media, IFM_ETHER | IFM_AUTO);
> vsc->sc_config_change = vio_config_change;
> timeout_set(&sc->sc_txtick, vio_txtick, &sc->sc_vq[VQTX]);
> timeout_set(&sc->sc_rxtick, vio_rxtick, &sc->sc_vq[VQRX]);
>
> + if_rxr_init(&sc->sc_rx_ring, 2 * ((ifp->if_hardmtu / MCLBYTES) + 1),
> + sc->sc_vq[VQRX].vq_num);
> +
> if_attach(ifp);
> ether_ifattach(ifp);
>
> return;
>
> @@ -664,12 +667,10 @@ int
> vio_init(struct ifnet *ifp)
> {
> struct vio_softc *sc = ifp->if_softc;
>
> vio_stop(ifp, 0);
> - if_rxr_init(&sc->sc_rx_ring, 2 * ((ifp->if_hardmtu / MCLBYTES) + 1),
> - sc->sc_vq[VQRX].vq_num);
> vio_populate_rx_mbufs(sc);
> ifp->if_flags |= IFF_RUNNING;
> ifq_clr_oactive(&ifp->if_snd);
> vio_iff(sc);
> vio_link_state(ifp);
> @@ -689,21 +690,15 @@ vio_stop(struct ifnet *ifp, int disable)
> /* only way to stop I/O and DMA is resetting... */
> virtio_reset(vsc);
> vio_rxeof(sc);
> if (vsc->sc_nvqs >= 3)
> vio_ctrleof(&sc->sc_vq[VQCTL]);
> - vio_tx_drain(sc);
> - if (disable)
> - vio_rx_drain(sc);
>
> - virtio_reinit_start(vsc);
> - virtio_negotiate_features(vsc, vsc->sc_features, NULL);
> virtio_start_vq_intr(vsc, &sc->sc_vq[VQRX]);
> virtio_stop_vq_intr(vsc, &sc->sc_vq[VQTX]);
> if (vsc->sc_nvqs >= 3)
> virtio_start_vq_intr(vsc, &sc->sc_vq[VQCTL]);
> - virtio_reinit_end(vsc);
> if (vsc->sc_nvqs >= 3) {
> if (sc->sc_ctrl_inuse != FREE)
> sc->sc_ctrl_inuse = RESET;
> wakeup(&sc->sc_ctrl_inuse);
> }
>
