On 1/12/2023 3:36 PM, Richard Ho (何明忠) wrote: > Hi Michael, > > We add this patch in my X86 platform and use the NCM device to test IPV4 PXE > boot from 1330MB ISO file. > > No this patch: 35 sec to download 1330M ISO file > Add this patch: 181 sec to download 1330M ISO file > > The patch will increase boot time in my X86 platform. > > Thanks, > Richard > > -----Original Message----- > From: Michael Brown <[email protected]> > Sent: 2023年1月10日 7:50 AM > To: [email protected]; [email protected]; Richard Ho (何明忠) > <[email protected]> > Subject: [EXTERNAL] Re: [edk2-devel] [PATCH 1/3] UsbNetworkPkg/UsbRndis: Add > USB RNDIS devices support > > > **CAUTION: The e-mail below is from an external source. Please exercise > caution before opening attachments, clicking links, or following guidance.** > > On 08/01/2023 10:41, tinhnguyen via groups.io wrote: >> The root cause is that we spend a long time waiting for USB >> UsbBulkTransfer to complete, but if there is no data to communicate >> -> it will always time out. > Coincidentally, I have just implemented a fix for this. It works by > implementing a simple credit-based rate limiter. Calls to > UsbEthReceive() are limited to 10 per second while the receive datapath is > idle. No limiting takes place while the receive datapath is active. > > I have tried to match the existing code style (i.e. zero explanatory > comments). > > Richard Ho: please consider using this rate limiting approach (or something > very similar). > > Patch inline below: > > diff --git a/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h > b/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h > index df6ebc64ef..d0e2048114 100644 > --- a/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h > +++ b/UsbNetworkPkg/Include/Protocol/EdkIIUsbEthernetProtocol.h > @@ -160,6 +160,8 @@ typedef struct { > UINT8 CurrentNodeAddress[PXE_MAC_LENGTH]; > UINT8 BroadcastNodeAddress[PXE_MAC_LENGTH]; > EFI_USB_DEVICE_REQUEST Request; > + EFI_EVENT RateLimiter; > + UINTN RateLimitCredit; > } NIC_DATA; > > #define NIC_DATA_SIGNATURE SIGNATURE_32('n', 'i', 'c', 'd') diff --git > a/UsbNetworkPkg/NetworkCommon/DriverBinding.h > b/UsbNetworkPkg/NetworkCommon/DriverBinding.h > index 946727ffc9..ae1d3c201e 100644 > --- a/UsbNetworkPkg/NetworkCommon/DriverBinding.h > +++ b/UsbNetworkPkg/NetworkCommon/DriverBinding.h > @@ -25,6 +25,8 @@ > #define RX_BUFFER_COUNT 32 > #define TX_BUFFER_COUNT 32 > #define MEMORY_REQUIRE 0 > +#define RATE_LIMITER_INTERVAL 1000000 // 10Hz > +#define RATE_LIMITER_BURST 10 > > #define UNDI_DEV_SIGNATURE SIGNATURE_32('u','n','d','i') > #define UNDI_DEV_FROM_THIS(a) CR(a, NIC_DEVICE, NiiProtocol, > UNDI_DEV_SIGNATURE) > diff --git a/UsbNetworkPkg/NetworkCommon/PxeFunction.c > b/UsbNetworkPkg/NetworkCommon/PxeFunction.c > index fd53a215a4..2a9b4f6111 100644 > --- a/UsbNetworkPkg/NetworkCommon/PxeFunction.c > +++ b/UsbNetworkPkg/NetworkCommon/PxeFunction.c > @@ -29,6 +29,21 @@ API_FUNC gUndiApiTable[] = { > UndiReceive > }; > > +STATIC > +VOID > +EFIAPI > +UndiRateLimiterCallback ( > + IN EFI_EVENT Event, > + IN VOID *Context > + ) > +{ > + NIC_DATA *Nic = Context; > + > + if (Nic->RateLimitCredit < RATE_LIMITER_BURST) { > + Nic->RateLimitCredit++; > + } > +} > + > /** > This command is used to determine the operational state of the UNDI. > > @@ -100,9 +115,6 @@ UndiStart ( > Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED; > Cdb->StatCode = PXE_STATCODE_INVALID_CDB; > return; > - } else { > - Cdb->StatFlags = PXE_STATFLAGS_COMMAND_COMPLETE; > - Cdb->StatCode = PXE_STATCODE_SUCCESS; > } > > if (Nic->State != PXE_STATFLAGS_GET_STATE_STOPPED) { @@ -120,14 +132,46 > @@ UndiStart ( > Nic->PxeStart.UnMap_Mem = 0; > Nic->PxeStart.Sync_Mem = Cpb->Sync_Mem; > Nic->PxeStart.Unique_ID = Cpb->Unique_ID; > - Nic->State = PXE_STATFLAGS_GET_STATE_STARTED; > + > + Status = gBS->CreateEvent ( > + EVT_TIMER | EVT_NOTIFY_SIGNAL, > + TPL_CALLBACK, > + UndiRateLimiterCallback, > + Nic, > + &Nic->RateLimiter > + ); > + if (EFI_ERROR (Status)) { > + goto ErrorCreateEvent; > + } > + > + Status = gBS->SetTimer ( > + Nic->RateLimiter, > + TimerPeriodic, > + RATE_LIMITER_INTERVAL > + ); > + if (EFI_ERROR (Status)) { > + goto ErrorSetTimer; > + } > > if (Nic->UsbEth->UsbEthUndi.UsbEthUndiStart != NULL) { > Status = Nic->UsbEth->UsbEthUndi.UsbEthUndiStart (Cdb, Nic); > if (EFI_ERROR (Status)) { > - Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED; > + goto ErrorUndiStart; > } > } > + > + Nic->State = PXE_STATFLAGS_GET_STATE_STARTED; > + Cdb->StatFlags = PXE_STATFLAGS_COMMAND_COMPLETE; Cdb->StatCode = > + PXE_STATCODE_SUCCESS; return; > + > + ErrorUndiStart: > + gBS->SetTimer (&Nic->RateLimiter, TimerCancel, 0); > + ErrorSetTimer: > + gBS->CloseEvent (&Nic->RateLimiter); > + ErrorCreateEvent: > + Cdb->StatFlags = PXE_STATFLAGS_COMMAND_FAILED; Cdb->StatCode = > + PXE_STATCODE_DEVICE_FAILURE; > } > > /** > @@ -183,6 +227,10 @@ UndiStop ( > Nic->PxeStart.Sync_Mem = 0; > Nic->State = PXE_STATFLAGS_GET_STATE_STOPPED; > > + gBS->SetTimer (&Nic->RateLimiter, TimerCancel, 0); > + > + gBS->CloseEvent (&Nic->RateLimiter); > + > if (Nic->UsbEth->UsbEthUndi.UsbEthUndiStop != NULL) { > Status = Nic->UsbEth->UsbEthUndi.UsbEthUndiStop (Cdb, Nic); > if (EFI_ERROR (Status)) { > @@ -1506,8 +1554,13 @@ Receive ( > } > > while (1) { > + if (Nic->RateLimitCredit == 0) { > + break; > + } > + > Status = Nic->UsbEth->UsbEthReceive (Cdb, Nic->UsbEth, (VOID > *)BulkInData, &DataLength); > if (EFI_ERROR (Status)) { > + Nic->RateLimitCredit--; > break; > } > > > Thanks, > > Michael > > -The information contained in this message may be confidential and > proprietary to American Megatrends (AMI). This communication is intended to > be read only by the individual or entity to whom it is addressed or by their > designee. If the reader of this message is not the intended recipient, you > are on notice that any distribution of this message, in any form, is strictly > prohibited. Please promptly notify the sender by reply e-mail or by telephone > at 770-246-8600, and then delete or destroy all copies of the transmission.
Hi, Sorry for the late response to this thread. My platform cannot utilize this driver without this fix, whether we can support a PCD to enable/disable this rate limiting? Thanks, - Tinh -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#99644): https://edk2.groups.io/g/devel/message/99644 Mute This Topic: https://groups.io/mt/95531719/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
