NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: kern/57560: xennetback_xenbus.c rev 1.109 broke guests, causes DOM0 reboot
The following reply was made to PR port-xen/57560; it has been noted by GNATS.
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: Brad Spencer <brad%anduin.eldar.org@localhost>
Cc: gnats-bugs%NetBSD.org@localhost, martin%NetBSD.org@localhost
Subject: Re: kern/57560: xennetback_xenbus.c rev 1.109 broke guests, causes DOM0 reboot
Date: Fri, 4 Aug 2023 20:26:25 +0000
This is a multi-part message in MIME format.
--=_8xiQiGqtxVZ2LhiPkIgdNm2J38qsNMwx
> Date: Fri, 4 Aug 2023 20:19:33 +0000
> From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
>
> So I went through to try to redo the changes more carefully. If you
> have the opportunity to test or bisect on another patch, I've drafted
> a series of changes, attached: [...]
...attached for real this time.
--=_8xiQiGqtxVZ2LhiPkIgdNm2J38qsNMwx
Content-Type: text/plain; charset="ISO-8859-1"; name="xvifmembars"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="xvifmembars.diff"
diff --git a/sys/arch/xen/xen/xennetback_xenbus.c b/sys/arch/xen/xen/xennet=
back_xenbus.c
index dac32a00d8db..559230017ff5 100644
--- a/sys/arch/xen/xen/xennetback_xenbus.c
+++ b/sys/arch/xen/xen/xennetback_xenbus.c
@@ -497,9 +497,7 @@ xennetback_connect(struct xnetback_instance *xneti)
goto err2;
}
xneti->xni_evtchn =3D evop.u.bind_interdomain.local_port;
- xen_wmb();
xneti->xni_status =3D CONNECTED;
- xen_wmb();
=20
xneti->xni_ih =3D xen_intr_establish_xname(-1, &xen_pic,
xneti->xni_evtchn, IST_LEVEL, IPL_NET, xennetback_evthandler,
@@ -805,28 +803,27 @@ xennetback_evthandler(void *arg)
netif_tx_request_t txreq;
struct mbuf *m, *m0 =3D NULL, *mlast =3D NULL;
int receive_pending;
- RING_IDX req_cons;
int queued =3D 0, m0_len =3D 0;
struct xnetback_xstate *xst;
const bool discard =3D ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) !=3D
(IFF_UP | IFF_RUNNING));
=20
XENPRINTF(("xennetback_evthandler "));
- req_cons =3D xneti->xni_txring.req_cons;
- while (1) {
- xen_rmb(); /* be sure to read the request before updating */
- xneti->xni_txring.req_cons =3D req_cons;
- xen_wmb();
- RING_FINAL_CHECK_FOR_REQUESTS(&xneti->xni_txring,
- receive_pending);
- if (receive_pending =3D=3D 0)
- break;
- RING_COPY_REQUEST(&xneti->xni_txring, req_cons,
- &txreq);
+again:
+ while (RING_HAS_UNCONSUMED_REQUESTS(&xneti->xni_txring)) {
+ /*
+ * Ensure we have read the producer's queue index in
+ * RING_FINAL_CHECK_FOR_REQUESTS before we read the
+ * content of the producer's next request in
+ * RING_COPY_REQUEST.
+ */
xen_rmb();
+ RING_COPY_REQUEST(&xneti->xni_txring,
+ xneti->xni_txring.req_cons,
+ &txreq);
XENPRINTF(("%s pkt size %d\n", xneti->xni_if.if_xname,
txreq.size));
- req_cons++;
+ xneti->xni_txring.req_cons++;
if (__predict_false(discard)) {
/* interface not up, drop all requests */
if_statinc(ifp, if_iqdrops);
@@ -875,7 +872,7 @@ mbuf_fail:
*/
int cnt;
m0_len =3D xennetback_tx_m0len_fragment(xneti,
- txreq.size, req_cons, &cnt);
+ txreq.size, xneti->xni_txring.req_cons, &cnt);
m->m_len =3D m0_len;
KASSERT(cnt <=3D XEN_NETIF_NR_SLOTS_MIN);
=20
@@ -941,7 +938,8 @@ mbuf_fail:
=20
XENPRINTF(("%s pkt offset %d size %d id %d req_cons %d\n",
xneti->xni_if.if_xname, txreq.offset,
- txreq.size, txreq.id, req_cons & (RING_SIZE(&xneti->xni_txring) - 1)=
));
+ txreq.size, txreq.id,
+ xneti->xni_txring.req_cons & (RING_SIZE(&xneti->xni_txring) - 1)));
=20
xst =3D &xneti->xni_xstate[queued];
xst->xs_m =3D (m0 =3D=3D NULL || m =3D=3D m0) ? m : NULL;
@@ -962,6 +960,9 @@ mbuf_fail:
queued =3D 0;
}
}
+ RING_FINAL_CHECK_FOR_REQUESTS(&xneti->xni_txring, receive_pending);
+ if (receive_pending)
+ goto again;
if (m0) {
/* Queue empty, and still unfinished multi-fragment request */
printf("%s: dropped unfinished multi-fragment\n",
@@ -1021,14 +1022,12 @@ xennetback_rx_copy_process(struct ifnet *ifp, struc=
t xnetback_instance *xneti,
}
=20
/* update pointer */
- xen_rmb();
xneti->xni_rxring.req_cons +=3D queued;
xneti->xni_rxring.rsp_prod_pvt +=3D queued;
RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&xneti->xni_rxring, notify);
=20
/* send event */
if (notify) {
- xen_rmb();
XENPRINTF(("%s receive event\n",
xneti->xni_if.if_xname));
hypervisor_notify_via_evtchn(xneti->xni_evtchn);
--=_8xiQiGqtxVZ2LhiPkIgdNm2J38qsNMwx
Content-Type: text/plain; charset="ISO-8859-1"; name="xvifmembars"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="xvifmembars.patch"
From 54f5ee6f683141090d9c533a9c0a5ce2ffb1286e Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 4 Aug 2023 19:40:06 +0000
Subject: [PATCH 1/8] xvif(4): Comment on memory barriers in
xennetback_evthandler.
Note which ones appear unnecessary and which ones appear too strong,
but don't change them.
No functional change intended.
---
sys/arch/xen/xen/xennetback_xenbus.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/sys/arch/xen/xen/xennetback_xenbus.c b/sys/arch/xen/xen/xennet=
back_xenbus.c
index dac32a00d8db..beaa73ed5bb1 100644
--- a/sys/arch/xen/xen/xennetback_xenbus.c
+++ b/sys/arch/xen/xen/xennetback_xenbus.c
@@ -814,15 +814,28 @@ xennetback_evthandler(void *arg)
XENPRINTF(("xennetback_evthandler "));
req_cons =3D xneti->xni_txring.req_cons;
while (1) {
+ /*
+ * XXX The xen_rmb here and comment make no sense:
+ * xneti->xni_txring.req_cons is a private variable.
+ */
xen_rmb(); /* be sure to read the request before updating */
xneti->xni_txring.req_cons =3D req_cons;
+ /* XXX Unclear what this xen_wmb is for. */
xen_wmb();
+ /*
+ * XXX RING_FINAL_CHECK_FOR_REQUESTS issues the most
+ * expensive memory barrier, xen_mb. This should be
+ * used only at the end of the loop after we updating
+ * the producer with the last index of the requests we
+ * consumed in the queue.
+ */
RING_FINAL_CHECK_FOR_REQUESTS(&xneti->xni_txring,
receive_pending);
if (receive_pending =3D=3D 0)
break;
RING_COPY_REQUEST(&xneti->xni_txring, req_cons,
&txreq);
+ /* XXX Unclear what this xen_rmb is for. */
xen_rmb();
XENPRINTF(("%s pkt size %d\n", xneti->xni_if.if_xname,
txreq.size));
From ac1bce0e69957aefe966fac39c8afb13432a6774 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 4 Aug 2023 19:41:16 +0000
Subject: [PATCH 2/8] xvif(4): Add missing xen_rmb in xennetback_evthandler.
---
sys/arch/xen/xen/xennetback_xenbus.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/sys/arch/xen/xen/xennetback_xenbus.c b/sys/arch/xen/xen/xennet=
back_xenbus.c
index beaa73ed5bb1..3b0104ba094e 100644
--- a/sys/arch/xen/xen/xennetback_xenbus.c
+++ b/sys/arch/xen/xen/xennetback_xenbus.c
@@ -833,6 +833,13 @@ xennetback_evthandler(void *arg)
receive_pending);
if (receive_pending =3D=3D 0)
break;
+ /*
+ * Ensure we have read the producer's queue index in
+ * RING_FINAL_CHECK_FOR_REQUESTS before we read the
+ * content of the producer's next request in
+ * RING_COPY_REQUEST.
+ */
+ xen_rmb();
RING_COPY_REQUEST(&xneti->xni_txring, req_cons,
&txreq);
/* XXX Unclear what this xen_rmb is for. */
From 122583462eeaf947293b6fb8443d5ec2bd8db3cd Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 4 Aug 2023 19:46:39 +0000
Subject: [PATCH 3/8] xvif(4): Omit local variable aliasing
xneti->xni_txring.req_cons.
No functional change intended.
---
sys/arch/xen/xen/xennetback_xenbus.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/sys/arch/xen/xen/xennetback_xenbus.c b/sys/arch/xen/xen/xennet=
back_xenbus.c
index 3b0104ba094e..ff53e5d1ad46 100644
--- a/sys/arch/xen/xen/xennetback_xenbus.c
+++ b/sys/arch/xen/xen/xennetback_xenbus.c
@@ -805,21 +805,18 @@ xennetback_evthandler(void *arg)
netif_tx_request_t txreq;
struct mbuf *m, *m0 =3D NULL, *mlast =3D NULL;
int receive_pending;
- RING_IDX req_cons;
int queued =3D 0, m0_len =3D 0;
struct xnetback_xstate *xst;
const bool discard =3D ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) !=3D
(IFF_UP | IFF_RUNNING));
=20
XENPRINTF(("xennetback_evthandler "));
- req_cons =3D xneti->xni_txring.req_cons;
while (1) {
/*
* XXX The xen_rmb here and comment make no sense:
* xneti->xni_txring.req_cons is a private variable.
*/
xen_rmb(); /* be sure to read the request before updating */
- xneti->xni_txring.req_cons =3D req_cons;
/* XXX Unclear what this xen_wmb is for. */
xen_wmb();
/*
@@ -840,13 +837,14 @@ xennetback_evthandler(void *arg)
* RING_COPY_REQUEST.
*/
xen_rmb();
- RING_COPY_REQUEST(&xneti->xni_txring, req_cons,
+ RING_COPY_REQUEST(&xneti->xni_txring,
+ xneti->xni_txring.req_cons,
&txreq);
/* XXX Unclear what this xen_rmb is for. */
xen_rmb();
XENPRINTF(("%s pkt size %d\n", xneti->xni_if.if_xname,
txreq.size));
- req_cons++;
+ xneti->xni_txring.req_cons++;
if (__predict_false(discard)) {
/* interface not up, drop all requests */
if_statinc(ifp, if_iqdrops);
@@ -895,7 +893,7 @@ mbuf_fail:
*/
int cnt;
m0_len =3D xennetback_tx_m0len_fragment(xneti,
- txreq.size, req_cons, &cnt);
+ txreq.size, xneti->xni_txring.req_cons, &cnt);
m->m_len =3D m0_len;
KASSERT(cnt <=3D XEN_NETIF_NR_SLOTS_MIN);
=20
@@ -961,7 +959,8 @@ mbuf_fail:
=20
XENPRINTF(("%s pkt offset %d size %d id %d req_cons %d\n",
xneti->xni_if.if_xname, txreq.offset,
- txreq.size, txreq.id, req_cons & (RING_SIZE(&xneti->xni_txring) - 1)=
));
+ txreq.size, txreq.id,
+ xneti->xni_txring.req_cons & (RING_SIZE(&xneti->xni_txring) - 1)));
=20
xst =3D &xneti->xni_xstate[queued];
xst->xs_m =3D (m0 =3D=3D NULL || m =3D=3D m0) ? m : NULL;
From b009b3e3f0b9346afbd6b5ce2efd2f23b6d36c15 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 4 Aug 2023 19:55:02 +0000
Subject: [PATCH 4/8] xvif(4): Move expensive xen_mb out of
xennetback_evthandler loop.
Use the cheaper RING_HAS_UNCONFIRMED_REQUESTS for most of the loop.
This should improve throughput without any impact on correctness.
---
sys/arch/xen/xen/xennetback_xenbus.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/sys/arch/xen/xen/xennetback_xenbus.c b/sys/arch/xen/xen/xennet=
back_xenbus.c
index ff53e5d1ad46..a2f375ac4e2e 100644
--- a/sys/arch/xen/xen/xennetback_xenbus.c
+++ b/sys/arch/xen/xen/xennetback_xenbus.c
@@ -811,6 +811,7 @@ xennetback_evthandler(void *arg)
(IFF_UP | IFF_RUNNING));
=20
XENPRINTF(("xennetback_evthandler "));
+again:
while (1) {
/*
* XXX The xen_rmb here and comment make no sense:
@@ -819,16 +820,7 @@ xennetback_evthandler(void *arg)
xen_rmb(); /* be sure to read the request before updating */
/* XXX Unclear what this xen_wmb is for. */
xen_wmb();
- /*
- * XXX RING_FINAL_CHECK_FOR_REQUESTS issues the most
- * expensive memory barrier, xen_mb. This should be
- * used only at the end of the loop after we updating
- * the producer with the last index of the requests we
- * consumed in the queue.
- */
- RING_FINAL_CHECK_FOR_REQUESTS(&xneti->xni_txring,
- receive_pending);
- if (receive_pending =3D=3D 0)
+ if (!RING_HAS_UNCONSUMED_REQUESTS(&xneti->xni_txring))
break;
/*
* Ensure we have read the producer's queue index in
@@ -981,6 +973,9 @@ mbuf_fail:
queued =3D 0;
}
}
+ RING_FINAL_CHECK_FOR_REQUESTS(&xneti->xni_txring, receive_pending);
+ if (receive_pending)
+ goto again;
if (m0) {
/* Queue empty, and still unfinished multi-fragment request */
printf("%s: dropped unfinished multi-fragment\n",
From ce882c7c51fc0c88e225e9b3814c10cfaf996e78 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 4 Aug 2023 19:56:50 +0000
Subject: [PATCH 5/8] xvif(4): Omit needless membars in xennetback_evthandle=
r.
This should improve throughput without any impact on correctness.
---
sys/arch/xen/xen/xennetback_xenbus.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/sys/arch/xen/xen/xennetback_xenbus.c b/sys/arch/xen/xen/xennet=
back_xenbus.c
index a2f375ac4e2e..20887b2c2dc0 100644
--- a/sys/arch/xen/xen/xennetback_xenbus.c
+++ b/sys/arch/xen/xen/xennetback_xenbus.c
@@ -813,13 +813,6 @@ xennetback_evthandler(void *arg)
XENPRINTF(("xennetback_evthandler "));
again:
while (1) {
- /*
- * XXX The xen_rmb here and comment make no sense:
- * xneti->xni_txring.req_cons is a private variable.
- */
- xen_rmb(); /* be sure to read the request before updating */
- /* XXX Unclear what this xen_wmb is for. */
- xen_wmb();
if (!RING_HAS_UNCONSUMED_REQUESTS(&xneti->xni_txring))
break;
/*
@@ -832,8 +825,6 @@ again:
RING_COPY_REQUEST(&xneti->xni_txring,
xneti->xni_txring.req_cons,
&txreq);
- /* XXX Unclear what this xen_rmb is for. */
- xen_rmb();
XENPRINTF(("%s pkt size %d\n", xneti->xni_if.if_xname,
txreq.size));
xneti->xni_txring.req_cons++;
From 7b450b9e18a4c874dffb9fb36f938771b201b8b6 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Fri, 4 Aug 2023 19:58:50 +0000
Subject: [PATCH 6/8] xvif(4): Simplify while loop in xennetback_evthandler.
No functional change intended.
---
sys/arch/xen/xen/xennetback_xenbus.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/sys/arch/xen/xen/xennetback_xenbus.c b/sys/arch/xen/xen/xennet=
back_xenbus.c
index 20887b2c2dc0..d07153ea4e62 100644
--- a/sys/arch/xen/xen/xennetback_xenbus.c
+++ b/sys/arch/xen/xen/xennetback_xenbus.c
@@ -812,9 +812,7 @@ xennetback_evthandler(void *arg)
=20
XENPRINTF(("xennetback_evthandler "));
again:
- while (1) {
- if (!RING_HAS_UNCONSUMED_REQUESTS(&xneti->xni_txring))
- break;
+ while (RING_HAS_UNCONSUMED_REQUESTS(&xneti->xni_txring)) {
/*
* Ensure we have read the producer's queue index in
* RING_FINAL_CHECK_FOR_REQUESTS before we read the
From 351d5093721ae68925ba8abb7bc305680ecc5d28 Mon Sep 17 00:00:00 2001
From: riastradh <riastradh%NetBSD.org@localhost>
Date: Sat, 25 Feb 2023 00:34:25 +0000
Subject: [PATCH 7/8] xvif(4): Omit needless membars in
xennetback_rx_copy_process.
- No need for barrier around touching req_cons and rsp_prod_pvt,
which are private.
- RING_PUSH_RESPONSES_AND_CHECK_NOTIFY updates the shared req_prod and
then issues xen_mb, which is all that we need between the update of
shared req_prod and hypervisor_notify_via_evtchn.
(Between updating the shared req_prod and issuing
hypervisor_notify_via_evtchn, only xen_wmb is needed. But after
writing to the shared req_prod, RING_PUSH_REQUESTS_AND_CHECK_NOTIFY
must also read from the shared rsp_event, which requires the
store-before-load ordering that only xen_mb provides.)
---
sys/arch/xen/xen/xennetback_xenbus.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sys/arch/xen/xen/xennetback_xenbus.c b/sys/arch/xen/xen/xennet=
back_xenbus.c
index d07153ea4e62..f873b0d1cdc7 100644
--- a/sys/arch/xen/xen/xennetback_xenbus.c
+++ b/sys/arch/xen/xen/xennetback_xenbus.c
@@ -1024,14 +1024,12 @@ xennetback_rx_copy_process(struct ifnet *ifp, struc=
t xnetback_instance *xneti,
}
=20
/* update pointer */
- xen_rmb();
xneti->xni_rxring.req_cons +=3D queued;
xneti->xni_rxring.rsp_prod_pvt +=3D queued;
RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&xneti->xni_rxring, notify);
=20
/* send event */
if (notify) {
- xen_rmb();
XENPRINTF(("%s receive event\n",
xneti->xni_if.if_xname));
hypervisor_notify_via_evtchn(xneti->xni_evtchn);
From 0e95d0a87aa7109b82a565d88e87ad22f66b60a0 Mon Sep 17 00:00:00 2001
From: riastradh <riastradh%NetBSD.org@localhost>
Date: Sat, 25 Feb 2023 00:34:36 +0000
Subject: [PATCH 8/8] xvif(4): Omit needless membars in xennetback_connect.
xneti is a private data structure to which we have exclusive access
here; ordering the stores doesn't make sense.
---
sys/arch/xen/xen/xennetback_xenbus.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sys/arch/xen/xen/xennetback_xenbus.c b/sys/arch/xen/xen/xennet=
back_xenbus.c
index f873b0d1cdc7..559230017ff5 100644
--- a/sys/arch/xen/xen/xennetback_xenbus.c
+++ b/sys/arch/xen/xen/xennetback_xenbus.c
@@ -497,9 +497,7 @@ xennetback_connect(struct xnetback_instance *xneti)
goto err2;
}
xneti->xni_evtchn =3D evop.u.bind_interdomain.local_port;
- xen_wmb();
xneti->xni_status =3D CONNECTED;
- xen_wmb();
=20
xneti->xni_ih =3D xen_intr_establish_xname(-1, &xen_pic,
xneti->xni_evtchn, IST_LEVEL, IPL_NET, xennetback_evthandler,
--=_8xiQiGqtxVZ2LhiPkIgdNm2J38qsNMwx--
Home |
Main Index |
Thread Index |
Old Index