Re: Fixup tsleeps in vio(4) and make them interruptable
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_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_ctrl_inuse, IPL_NET, "vio_wait", 0);
> + int r = 0;
> +
> + while (sc->sc_ctrl_inuse != FREE) {
> + r = tsleep(>sc_ctrl_inuse, PRIBIO|PCATCH,
Re: Fixup tsleeps in vio(4) and make them interruptable
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_ctrl_inuse, IPL_NET, "vio_wait", 0);
> + int r = 0;
> +
> + while (sc->sc_ctrl_inuse != FREE) {
> + r = tsleep(>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_ctrl_inuse, IPL_NET, "vio_wait", 0);
> + r = tsleep(>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 {
> -
Fixup tsleeps in vio(4) and make them interruptable
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?
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);
intvio_set_rx_filter(struct vio_softc *);
void vio_iff(struct vio_softc *);
intvio_media_change(struct ifnet *);
void vio_media_status(struct ifnet *, struct ifmediareq *);
intvio_ctrleof(struct virtqueue *);
-void vio_wait_ctrl(struct vio_softc *sc);
+intvio_wait_ctrl(struct vio_softc *sc);
intvio_wait_ctrl_done(struct vio_softc *sc);
void vio_ctrl_wakeup(struct vio_softc *, enum vio_ctrl_state);
intvio_alloc_mem(struct vio_softc *);
intvio_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_ctrl_inuse, IPL_NET, "vio_wait", 0);
+ int r = 0;
+
+ while (sc->sc_ctrl_inuse != FREE) {
+ r = tsleep(>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_ctrl_inuse, IPL_NET, "vio_wait", 0);
+ r = tsleep(>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;
- vio_set_rx_filter(sc);
- }
- if (allmulti) {
- r = vio_ctrl_rx(sc, VIRTIO_NET_CTRL_RX_ALLMULTI, 1);
- if (r != 0) {
