Re: [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

> On 09/17/18 07:43, Jiaxin Wu wrote:
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=886
> >
> > The series patches are to support the TFTP windowsize option described in
> RFC 7440.
> > TFTP shell command and UEFI PXE driver will use the feature to benefit the
> download
> > performance.
> 
> I tested this series, between two virtual machines running on my laptop.
> The TFTP server program that I used was "tftp-server-5.2-22.el7.x86_64".
> The downloaded file was 478,150,656 bytes in size. I built OVMF with
> NETWORK_IP6_ENABLE, so that the last patch would take effect for both
> PXEv4 and PXEv6.
> 
> Before the series:
> - PXEv4:  75 seconds (~ 6225 KB/s)
> - PXEv6: 100 seconds (~ 4669 KB/s)
> 
> After the series:
> - PXEv4: 48 seconds (~ 9728 KB/s)
> - PXEv6: 60 seconds (~ 7782 KB/s)
> 
> These measurements are very rough (I didn't run them multiple times
> etc), but I think they are still quite good indicators.
> 
> For the testing, I used the UEFI boot options in UiApp, and not the
> shell command, hence I have no feedback on patch #3.
> 
> For patches #1, #2, and #5:
> 
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 

Thanks the verification.


> However, as I pointed out elsewhere in the thread, I think:
> 
> - You might want to port the changes from patch#5 to
> "MdeModulePkg/Universal/Network/UefiPxeBcDxe" as well, in a separate
> patch (patch #6).
> 
> - If not, then (a) we should document this feature difference in the INF
> files of the UefiPxeBcDxe drivers, (b) patch #4 should be re-done so
> that it target NetworkPkg, not MdeModulePkg.
> 

As I said in the previous email, normally, we only add the new feature into the combo driver. But I think that's depends on the request. If you want to include the feature into MdeModulePkg/Universal/Network/UefiPxeBcDxe since the OVMF platform might use it, I will create patch #6. If not, I will follow the comments (a)/(b).


> - Patch #4 (regardless of package DEC) should be extended with
> documentation (both DEC and UNI).
> 
> Thanks
> Laszlo