On Wed, 23 Nov 2016, Mike Belopuhov wrote:
> This fixes up tsleep priorities in vio(4) and makes sleeps
> interruptable so that ifconfig can be killed with ^C. Tested
> on qemu with a previous diff that makes vio(4) hang there.
>
> OK?
I think the tsleep stuff is ok, but the vio_iff() changes are not.
>
> diff --git sys/dev/pci/if_vio.c sys/dev/pci/if_vio.c
> index 97af2a2..d8c9798 100644
> --- sys/dev/pci/if_vio.c
> +++ sys/dev/pci/if_vio.c
> @@ -281,11 +281,11 @@ int vio_ctrl_rx(struct vio_softc *, int, int);
> int vio_set_rx_filter(struct vio_softc *);
> void vio_iff(struct vio_softc *);
> int vio_media_change(struct ifnet *);
> void vio_media_status(struct ifnet *, struct ifmediareq *);
> int vio_ctrleof(struct virtqueue *);
> -void vio_wait_ctrl(struct vio_softc *sc);
> +int vio_wait_ctrl(struct vio_softc *sc);
> int vio_wait_ctrl_done(struct vio_softc *sc);
> void vio_ctrl_wakeup(struct vio_softc *, enum vio_ctrl_state);
> int vio_alloc_mem(struct vio_softc *);
> int vio_alloc_dmamem(struct vio_softc *);
> void vio_free_dmamem(struct vio_softc *);
> @@ -1219,11 +1219,12 @@ vio_ctrl_rx(struct vio_softc *sc, int cmd, int onoff)
>
> if (vsc->sc_nvqs < 3)
> return ENOTSUP;
>
> splassert(IPL_NET);
> - vio_wait_ctrl(sc);
> + if ((r = vio_wait_ctrl(sc)) != 0)
> + return r;
>
> sc->sc_ctrl_cmd->class = VIRTIO_NET_CTRL_RX;
> sc->sc_ctrl_cmd->command = cmd;
> sc->sc_ctrl_rx->onoff = onoff;
>
> @@ -1246,14 +1247,12 @@ vio_ctrl_rx(struct vio_softc *sc, int cmd, int onoff)
> sizeof(*sc->sc_ctrl_rx), 1);
> VIO_DMAMEM_ENQUEUE(sc, vq, slot, sc->sc_ctrl_status,
> sizeof(*sc->sc_ctrl_status), 0);
> virtio_enqueue_commit(vsc, vq, slot, 1);
>
> - if (vio_wait_ctrl_done(sc)) {
> - r = EIO;
> + if ((r = vio_wait_ctrl_done(sc)) != 0)
> goto out;
> - }
>
> VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_cmd,
> sizeof(*sc->sc_ctrl_cmd), BUS_DMASYNC_POSTWRITE);
> VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_rx,
> sizeof(*sc->sc_ctrl_rx), BUS_DMASYNC_POSTWRITE);
> @@ -1271,28 +1270,38 @@ vio_ctrl_rx(struct vio_softc *sc, int cmd, int onoff)
> out:
> vio_ctrl_wakeup(sc, FREE);
> return r;
> }
>
> -void
> +int
> vio_wait_ctrl(struct vio_softc *sc)
> {
> - while (sc->sc_ctrl_inuse != FREE)
> - tsleep(&sc->sc_ctrl_inuse, IPL_NET, "vio_wait", 0);
> + int r = 0;
> +
> + while (sc->sc_ctrl_inuse != FREE) {
> + r = tsleep(&sc->sc_ctrl_inuse, PRIBIO|PCATCH, "viowait", 0);
> + if (r == EINTR)
> + return r;
> + }
> sc->sc_ctrl_inuse = INUSE;
> +
> + return r;
> }
>
> int
> vio_wait_ctrl_done(struct vio_softc *sc)
> {
> int r = 0;
> +
> while (sc->sc_ctrl_inuse != DONE && sc->sc_ctrl_inuse != RESET) {
> if (sc->sc_ctrl_inuse == RESET) {
> - r = 1;
> + r = EIO;
> break;
> }
> - tsleep(&sc->sc_ctrl_inuse, IPL_NET, "vio_wait", 0);
> + r = tsleep(&sc->sc_ctrl_inuse, PRIBIO|PCATCH, "viodone", 0);
> + if (r == EINTR)
> + break;
> }
> return r;
> }
>
> void
> @@ -1334,11 +1343,12 @@ vio_set_rx_filter(struct vio_softc *sc)
> splassert(IPL_NET);
>
> if (vsc->sc_nvqs < 3)
> return ENOTSUP;
>
> - vio_wait_ctrl(sc);
> + if ((r = vio_wait_ctrl(sc)) != 0)
> + return r;
>
> sc->sc_ctrl_cmd->class = VIRTIO_NET_CTRL_MAC;
> sc->sc_ctrl_cmd->command = VIRTIO_NET_CTRL_MAC_TABLE_SET;
>
> VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_cmd,
> @@ -1364,14 +1374,12 @@ vio_set_rx_filter(struct vio_softc *sc)
> sc->sc_ctrl_mac_tbl_mc->nentries * ETHER_ADDR_LEN, 1);
> VIO_DMAMEM_ENQUEUE(sc, vq, slot, sc->sc_ctrl_status,
> sizeof(*sc->sc_ctrl_status), 0);
> virtio_enqueue_commit(vsc, vq, slot, 1);
>
> - if (vio_wait_ctrl_done(sc)) {
> - r = EIO;
> + if ((r = vio_wait_ctrl_done(sc)) != 0)
> goto out;
> - }
>
> VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_cmd,
> sizeof(*sc->sc_ctrl_cmd), BUS_DMASYNC_POSTWRITE);
> VIO_DMAMEM_SYNC(vsc, sc, sc->sc_ctrl_mac_info,
> VIO_CTRL_MAC_INFO_SIZE, BUS_DMASYNC_POSTWRITE);
> @@ -1436,29 +1444,22 @@ vio_iff(struct vio_softc *sc)
>
> /* set unicast address, VirtualBox wants that */
> memcpy(sc->sc_ctrl_mac_tbl_uc->macs[0], ac->ac_enaddr, ETHER_ADDR_LEN);
> sc->sc_ctrl_mac_tbl_uc->nentries = 1;
>
> - if (rxfilter) {
> + if (rxfilter)
> sc->sc_ctrl_mac_tbl_mc->nentries = nentries;
> - r = vio_set_rx_filter(sc);
> - if (r != 0) {
> - rxfilter = 0;
> - allmulti = 1; /* fallback */
> - }
> - } else {
> - sc->sc_ctrl_mac_tbl_mc->nentries = 0;
You must not remove this this in the !rxfilter case, or an old value of
nentries may be kept.
> - vio_set_rx_filter(sc);
> - }
>
> - if (allmulti) {
> - r = vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_ALLMULTI, 1);
> - if (r != 0) {
> - allmulti = 0;
> - promisc = 1; /* fallback */
> - }
> - } else {
> - vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_ALLMULTI, 0);
> - }
> + r = vio_set_rx_filter(sc);
> + if (r && r != ENOTSUP)
> + return;
That's wrong. If the host's filter table is not large enough,
vio_set_rx_filter() will return EIO. In this case we should re-try with
promisc.
> + else if (r == ENOTSUP && rxfilter)
> + allmulti = 1; /* fallback */
> +
> + r = vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_ALLMULTI, allmulti);
> + if (r && r != ENOTSUP)
> + return;
> + else if (r == ENOTSUP && allmulti)
> + promisc = 1; /* fallback */
In the ENOTSUP case, vio_ctrl_rx() will always fail, too. There is no use
for a retry, then.
Maybe leave the logic unchanged, and just add a few checks to return in
case of EINTR?
>
> vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_PROMISC, promisc);
> }
>
>
