From 23078154cd8baf36824869bd177749192be5dc88 Mon Sep 17 00:00:00 2001 From: Alexander Boettcher Date: Wed, 31 Jan 2024 17:31:23 +0100 Subject: [PATCH] vbox: avoid blocking nic_ep thread during receive the nic_ep may block as long as the guest does not provide another receive network descriptor. In the meantime, all Genode signals regarding the network interface, e.g. tx, will be postponed, which may effect the throughput. Instead use the nic_ep for rx packets unblocking. Add an notification mechanism to the e1000 vbox network model, to notify us as soon as the guest added new receive descriptors in the model. Issue #5146 --- repos/ports/ports/virtualbox5.hash | 2 +- repos/ports/ports/virtualbox6.hash | 2 +- repos/ports/src/virtualbox5/network.cpp | 71 ++++++++++++++++--- .../src/virtualbox5/patches/network.patch | 24 +++++++ repos/ports/src/virtualbox6/network.cc | 68 +++++++++++++++--- .../src/virtualbox6/patches/network.patch | 25 +++++++ 6 files changed, 173 insertions(+), 19 deletions(-) diff --git a/repos/ports/ports/virtualbox5.hash b/repos/ports/ports/virtualbox5.hash index 3af8f1b82d..9467c611c6 100644 --- a/repos/ports/ports/virtualbox5.hash +++ b/repos/ports/ports/virtualbox5.hash @@ -1 +1 @@ -9d3de38fdb7c95517c4ce76dbd090a147fa7b79c +4df67ce24bce24be1c0171bfb9eef96a42e95c6f diff --git a/repos/ports/ports/virtualbox6.hash b/repos/ports/ports/virtualbox6.hash index 91d62f3a94..a1455e97db 100644 --- a/repos/ports/ports/virtualbox6.hash +++ b/repos/ports/ports/virtualbox6.hash @@ -1 +1 @@ -a1376cc9dbcced9ff29f37da20ddc54363e24f3d +4f4c05a80b5767a0132c333a352fada6ba8965a8 diff --git a/repos/ports/src/virtualbox5/network.cpp b/repos/ports/src/virtualbox5/network.cpp index d9b7b3aced..821462b231 100644 --- a/repos/ports/src/virtualbox5/network.cpp +++ b/repos/ports/src/virtualbox5/network.cpp @@ -71,6 +71,8 @@ typedef struct DRVNIC PPDMDRVINS pDrvIns; /** Transmit lock used by pfnBeginXmit/pfnEndXmit. */ RTCRITSECT XmitLock; + /** Receive lock used by nic_ep and EMT-0..X */ + RTCRITSECT RecvLock; /** Nic::Session client wrapper. */ Nic_client *nic_client; } DRVNIC, *PDRVNIC; @@ -120,31 +122,57 @@ class Nic_client PDRVNIC _drvnic; void _handle_rx_packet_avail() + { + int const rc = RTCritSectEnter(&_drvnic->RecvLock); + AssertRC(rc); + + _handle_rx_packet_avail_unlocked(); + + RTCritSectLeave(&_drvnic->RecvLock); + } + + void _handle_rx_packet_avail_unlocked() { auto &rx = *_nic.rx(); bool progress = false; + bool unready = false; while (rx.packet_avail() && rx.ready_to_ack()) { + Libc::with_libc([&] () { + RTMSINTERVAL const wait_ms = 0; /* try once without blocking */ + + int rc = _down_net->pfnWaitReceiveAvail(_down_net, wait_ms); + if (rc != VINF_SUCCESS) + unready = true; + }); + + /* network model of VBox can't accept a new packet at moment */ + if (unready) + break; + auto const rx_packet = rx.try_get_packet(); - if (!rx_packet.size()) + if (!rx_packet.size()) { + RTLogPrintf("unexpected - should not happen - size 0\n"); break; + } char *rx_content = rx.packet_content(rx_packet); - if (!rx_content) + if (!rx_content) { + RTLogPrintf("unexpected - should not happen - no content\n"); break; + } Libc::with_libc([&] () { - int rc = _down_net->pfnWaitReceiveAvail(_down_net, RT_INDEFINITE_WAIT); - if (RT_FAILURE(rc)) return; + int rc = _down_net->pfnReceive(_down_net, rx_content, + rx_packet.size()); - rc = _down_net->pfnReceive(_down_net, rx_content, rx_packet.size()); - AssertRC(rc); - - if (rx.try_ack_packet(rx_packet)) + if (rc == VINF_SUCCESS && rx.try_ack_packet(rx_packet)) progress = true; + else + RTLogPrintf("unexpected - should not happen - ack packet\n"); }); } @@ -352,6 +380,16 @@ class Nic_client _tx_wakeup = false; _nic.tx()->wakeup(); } + + void rx_resume() + { + int const rc = RTCritSectEnter(&_drvnic->RecvLock); + AssertRC(rc); + + _handle_rx_packet_avail_unlocked(); + + RTCritSectLeave(&_drvnic->RecvLock); + } }; @@ -604,10 +642,23 @@ static DECLCALLBACK(void) drvNicDestruct(PPDMDRVINS pDrvIns) /* wait until the recv thread exits */ destruct_blockade().block(); + if (nic_client) + destroy(vmm_heap(), nic_client); + if (RTCritSectIsInitialized(&pThis->XmitLock)) RTCritSectDelete(&pThis->XmitLock); - destroy(vmm_heap(), nic_client); + if (RTCritSectIsInitialized(&pThis->RecvLock)) + RTCritSectDelete(&pThis->RecvLock); +} + + +static DECLCALLBACK(void) drvNicNetworkUp_ReceiveReady(PPDMINETWORKUP pInterface) +{ + PDRVNIC const pThis = PDMINETWORKUP_2_DRVNIC(pInterface); + Nic_client * const nic_client = pThis->nic_client; + + nic_client->rx_resume(); } @@ -636,10 +687,12 @@ static DECLCALLBACK(int) drvNicConstruct(PPDMDRVINS pDrvIns, PCFGMNODE pCfg, uin pThis->INetworkUp.pfnEndXmit = drvNicNetworkUp_EndXmit; pThis->INetworkUp.pfnSetPromiscuousMode = drvNicNetworkUp_SetPromiscuousMode; pThis->INetworkUp.pfnNotifyLinkChanged = drvNicNetworkUp_NotifyLinkChanged; + pThis->INetworkUp.pfnReceiveReady = drvNicNetworkUp_ReceiveReady; /* INetworkConfig - used on Genode to request Mac address of nic_session */ pThis->INetworkConfig.pfnGetMac = drvGetMac; RTCritSectInit(&pThis->XmitLock); + RTCritSectInit(&pThis->RecvLock); /* * Check that no-one is attached to us. diff --git a/repos/ports/src/virtualbox5/patches/network.patch b/repos/ports/src/virtualbox5/patches/network.patch index 8037da6aa7..f967eac688 100644 --- a/repos/ports/src/virtualbox5/patches/network.patch +++ b/repos/ports/src/virtualbox5/patches/network.patch @@ -4,6 +4,19 @@ diff --git a/src/app/virtualbox/src/VBox/Devices/Network/DevE1000.cpp b/src/app/ index aa3eb87..fc2660a 100644 --- a/src/app/virtualbox/src/VBox/Devices/Network/DevE1000.cpp +++ b/src/app/virtualbox/src/VBox/Devices/Network/DevE1000.cpp +@@ -1614,6 +1614,12 @@ + static void e1kWakeupReceive(PPDMDEVINS pDevIns) + { + PE1KSTATE pThis = PDMINS_2_DATA(pDevIns, PE1KSTATE); ++ ++ if (pThis->pDrvR3 && pThis->pDrvR3->pfnReceiveReady) { ++ pThis->pDrvR3->pfnReceiveReady(pThis->pDrvR3); ++ return; ++ } ++ + if ( pThis->fMaybeOutOfSpace + && pThis->hEventMoreRxDescAvail != NIL_RTSEMEVENT) + { @@ -7629,7 +7629,7 @@ return PDMDEV_SET_ERROR(pDevIns, rc, N_("Configuration error: Failed to get the value of 'EthernetCRC'")); @@ -94,3 +107,14 @@ index 481267e..80f4af9 100644 if (rc == VINF_NAT_DNS) { #ifdef RT_OS_LINUX +--- a/src/app/virtualbox/include/VBox/vmm/pdmnetifs.h ++++ b/src/app/virtualbox/include/VBox/vmm/pdmnetifs.h +@@ -322,6 +322,8 @@ + + /** @todo Add a callback that informs the driver chain about MAC address changes if we ever implement that. */ + ++ DECLR3CALLBACKMEMBER(void, pfnReceiveReady,(PPDMINETWORKUP pInterface)); ++ + } PDMINETWORKUP; + + /** Ring-0 edition of PDMINETWORKUP. */ diff --git a/repos/ports/src/virtualbox6/network.cc b/repos/ports/src/virtualbox6/network.cc index 1a5a150be8..2b2c34f33d 100644 --- a/repos/ports/src/virtualbox6/network.cc +++ b/repos/ports/src/virtualbox6/network.cc @@ -74,6 +74,8 @@ typedef struct DRVNIC PPDMDRVINS pDrvIns; /** Transmit lock used by pfnBeginXmit/pfnEndXmit. */ RTCRITSECT XmitLock; + /** Receive lock used by nic_ep and EMT-0..X */ + RTCRITSECT RecvLock; /** Nic::Session client wrapper. */ Nic_client *nic_client; } DRVNIC, *PDRVNIC; @@ -136,31 +138,57 @@ class Nic_client PDRVNIC _drvnic; void _handle_rx_packet_avail() + { + int const rc = RTCritSectEnter(&_drvnic->RecvLock); + AssertRC(rc); + + _handle_rx_packet_avail_unlocked(); + + RTCritSectLeave(&_drvnic->RecvLock); + } + + void _handle_rx_packet_avail_unlocked() { auto &rx = *_nic.rx(); bool progress = false; + bool unready = false; while (rx.packet_avail() && rx.ready_to_ack()) { + Libc::with_libc([&] () { + RTMSINTERVAL const wait_ms = 0; /* try once without blocking */ + + int rc = _down_net->pfnWaitReceiveAvail(_down_net, wait_ms); + if (rc != VINF_SUCCESS) + unready = true; + }); + + /* network model of VBox can't accept a new packet at moment */ + if (unready) + break; + auto const rx_packet = rx.try_get_packet(); - if (!rx_packet.size()) + if (!rx_packet.size()) { + RTLogPrintf("unexpected - should not happen - size 0\n"); break; + } char *rx_content = rx.packet_content(rx_packet); - if (!rx_content) + if (!rx_content) { + RTLogPrintf("unexpected - should not happen - no content\n"); break; + } Libc::with_libc([&] () { - int rc = _down_net->pfnWaitReceiveAvail(_down_net, RT_INDEFINITE_WAIT); - if (RT_FAILURE(rc)) return; + int rc = _down_net->pfnReceive(_down_net, rx_content, + rx_packet.size()); - rc = _down_net->pfnReceive(_down_net, rx_content, rx_packet.size()); - AssertRC(rc); - - if (rx.try_ack_packet(rx_packet)) + if (rc == VINF_SUCCESS && rx.try_ack_packet(rx_packet)) progress = true; + else + RTLogPrintf("unexpected - should not happen - ack packet\n"); }); } @@ -367,6 +395,16 @@ class Nic_client _tx_wakeup = false; _nic.tx()->wakeup(); } + + void rx_resume() + { + int const rc = RTCritSectEnter(&_drvnic->RecvLock); + AssertRC(rc); + + _handle_rx_packet_avail_unlocked(); + + RTCritSectLeave(&_drvnic->RecvLock); + } }; @@ -619,6 +657,18 @@ static DECLCALLBACK(void) drvNicDestruct(PPDMDRVINS pDrvIns) if (RTCritSectIsInitialized(&pThis->XmitLock)) RTCritSectDelete(&pThis->XmitLock); + + if (RTCritSectIsInitialized(&pThis->RecvLock)) + RTCritSectDelete(&pThis->RecvLock); +} + + +static DECLCALLBACK(void) drvNicNetworkUp_ReceiveReady(PPDMINETWORKUP pInterface) +{ + PDRVNIC const pThis = PDMINETWORKUP_2_DRVNIC(pInterface); + Nic_client * const nic_client = pThis->nic_client; + + nic_client->rx_resume(); } @@ -647,10 +697,12 @@ static DECLCALLBACK(int) drvNicConstruct(PPDMDRVINS pDrvIns, PCFGMNODE pCfg, uin pThis->INetworkUp.pfnEndXmit = drvNicNetworkUp_EndXmit; pThis->INetworkUp.pfnSetPromiscuousMode = drvNicNetworkUp_SetPromiscuousMode; pThis->INetworkUp.pfnNotifyLinkChanged = drvNicNetworkUp_NotifyLinkChanged; + pThis->INetworkUp.pfnReceiveReady = drvNicNetworkUp_ReceiveReady; /* INetworkConfig - used on Genode to request Mac address of nic_session */ pThis->INetworkConfig.pfnGetMac = drvGetMac; RTCritSectInit(&pThis->XmitLock); + RTCritSectInit(&pThis->RecvLock); /* * Check that no-one is attached to us. diff --git a/repos/ports/src/virtualbox6/patches/network.patch b/repos/ports/src/virtualbox6/patches/network.patch index 42ebee7e15..c513192c5d 100644 --- a/repos/ports/src/virtualbox6/patches/network.patch +++ b/repos/ports/src/virtualbox6/patches/network.patch @@ -2,6 +2,20 @@ diff --git a/src/virtualbox6/src/VBox/Devices/Network/DevE1000.cpp b/src/virtual index 2c2e366..b45855b 100644 --- a/src/virtualbox6/src/VBox/Devices/Network/DevE1000.cpp +++ b/src/virtualbox6/src/VBox/Devices/Network/DevE1000.cpp +@@ -1768,6 +1768,13 @@ + */ + static void e1kWakeupReceive(PPDMDEVINS pDevIns, PE1KSTATE pThis) + { ++ PE1KSTATECC pThisCC = PDMDEVINS_2_DATA_CC(pDevIns, PE1KSTATECC); ++ ++ if (pThisCC->pDrvR3 && pThisCC->pDrvR3->pfnReceiveReady) { ++ pThisCC->pDrvR3->pfnReceiveReady(pThisCC->pDrvR3); ++ return; ++ } ++ + if ( pThis->fMaybeOutOfSpace + && pThis->hEventMoreRxDescAvail != NIL_SUPSEMEVENT) + { @@ -8001,7 +8001,7 @@ return PDMDEV_SET_ERROR(pDevIns, rc, N_("Configuration error: Failed to get the value of 'EthernetCRC'")); @@ -93,3 +107,14 @@ index bb69ca7..6d75de0 100644 pThisCC->pDrv = PDMIBASE_QUERY_INTERFACE(pThisCC->pDrvBase, PDMINETWORKUP); AssertMsgReturn(pThisCC->pDrv, ("Failed to obtain the PDMINETWORKUP interface!\n"), VERR_PDM_MISSING_INTERFACE_BELOW); +--- a/src/virtualbox6/include/VBox/vmm/pdmnetifs.h ++++ b/src/virtualbox6/include/VBox/vmm/pdmnetifs.h +@@ -325,6 +325,8 @@ + + /** @todo Add a callback that informs the driver chain about MAC address changes if we ever implement that. */ + ++ DECLR3CALLBACKMEMBER(void, pfnReceiveReady,(PPDMINETWORKUP pInterface)); ++ + } PDMINETWORKUP; + + /** Ring-0 edition of PDMINETWORKUP. */