From 54f5ee6f683141090d9c533a9c0a5ce2ffb1286e Mon Sep 17 00:00:00 2001 From: Taylor R Campbell 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/xennetback_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 = 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 = 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 == 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 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/xennetback_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 == 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 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/xennetback_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 = NULL, *mlast = NULL; int receive_pending; - RING_IDX req_cons; int queued = 0, m0_len = 0; struct xnetback_xstate *xst; const bool discard = ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) != (IFF_UP | IFF_RUNNING)); XENPRINTF(("xennetback_evthandler ")); - req_cons = 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 = 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 = xennetback_tx_m0len_fragment(xneti, - txreq.size, req_cons, &cnt); + txreq.size, xneti->xni_txring.req_cons, &cnt); m->m_len = m0_len; KASSERT(cnt <= XEN_NETIF_NR_SLOTS_MIN); @@ -961,7 +959,8 @@ mbuf_fail: 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))); xst = &xneti->xni_xstate[queued]; xst->xs_m = (m0 == NULL || m == m0) ? m : NULL; From b009b3e3f0b9346afbd6b5ce2efd2f23b6d36c15 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell 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/xennetback_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)); 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 == 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 = 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 Date: Fri, 4 Aug 2023 19:56:50 +0000 Subject: [PATCH 5/8] xvif(4): Omit needless membars in xennetback_evthandler. 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/xennetback_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 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/xennetback_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) 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 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/xennetback_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, struct xnetback_instance *xneti, } /* update pointer */ - xen_rmb(); xneti->xni_rxring.req_cons += queued; xneti->xni_rxring.rsp_prod_pvt += queued; RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(&xneti->xni_rxring, notify); /* 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 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/xennetback_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 = evop.u.bind_interdomain.local_port; - xen_wmb(); xneti->xni_status = CONNECTED; - xen_wmb(); xneti->xni_ih = xen_intr_establish_xname(-1, &xen_pic, xneti->xni_evtchn, IST_LEVEL, IPL_NET, xennetback_evthandler,