On Thu, Nov 24, 2016 at 20:52 +0100, Mike Belopuhov wrote:
> On Wed, Nov 23, 2016 at 21:52 +0100, Stefan Fritsch wrote:
> > 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.
> >
>
> The vio_iff changes are needed to get the interruptable sleep
> working. I've rewritten that part of the diff.
>
> > > - 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.
> >
>
> Ack.
>
> > > - 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.
> >
>
> This wasn't documented and EIO wasn't explicitely handled which
> made this code look strange. This is a good reason to clean it up.
>
> > > + 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.
> >
>
> I think we can move the "vsc->sc_nvqs < 3" check out of those
> functions to make it even more simpler.
>
> > Maybe leave the logic unchanged, and just add a few checks to return in
> > case of EINTR?
> >
>
> I propose less checks (or better ones if you will) since it's
> a forest of if's as it is.
>
OK?
> diff --git sys/dev/pci/if_vio.c sys/dev/pci/if_vio.c
> index d2a56a3..3926edf 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 *);
> @@ -1216,15 +1216,14 @@ vio_ctrl_rx(struct vio_softc *sc, int cmd, int onoff)
> {
> struct virtio_softc *vsc = sc->sc_virtio;
> struct virtqueue *vq = &sc->sc_vq[VQCTL];
> int r, slot;
>
> - 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;
>
> @@ -1247,14 +1246,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);
> @@ -1272,28 +1269,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;
> 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
> @@ -1332,14 +1339,12 @@ vio_set_rx_filter(struct vio_softc *sc)
> struct virtqueue *vq = &sc->sc_vq[VQCTL];
> int r, slot;
>
> 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,
> @@ -1365,14 +1370,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);
> @@ -1380,10 +1383,11 @@ vio_set_rx_filter(struct vio_softc *sc)
> sizeof(*sc->sc_ctrl_status), BUS_DMASYNC_POSTREAD);
>
> if (sc->sc_ctrl_status->ack == VIRTIO_NET_OK) {
> r = 0;
> } else {
> + /* The host's filter table is not large enough */
> printf("%s: failed setting rx filter\n", sc->sc_dev.dv_xname);
> r = EIO;
> }
>
> out:
> @@ -1437,29 +1441,24 @@ 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) {
> - 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;
> - vio_set_rx_filter(sc);
> - }
> + sc->sc_ctrl_mac_tbl_mc->nentries = rxfilter ? nentries : 0;
>
> - 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);
> - }
> + if (vsc->sc_nvqs < 3)
> + return;
> +
> + r = vio_set_rx_filter(sc);
> + if (r == EIO)
> + allmulti = 1; /* fallback */
> + else if (r != 0)
> + return;
> +
> + r = vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_ALLMULTI, allmulti);
> + if (r == EIO)
> + promisc = 1; /* fallback */
> + else if (r != 0)
> + return;
>
> vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_PROMISC, promisc);
> }
