I missed that the default was from the RFC. I agree with you. -Jaben
> -----Original Message----- > From: Leif Lindholm [mailto:[email protected]] > Sent: Friday, April 29, 2016 11:00 AM > To: Carsey, Jaben <[email protected]> > Cc: Marcin Wojtas <[email protected]>; Fu, Siyuan <[email protected]>; > [email protected]; [email protected]; Gao, Liming > <[email protected]>; [email protected]; Kinney, Michael D > <[email protected]> > Subject: Re: [edk2] [RFC] MdeModulePkg, ShellPkg: increase TFTP block size > Importance: High > > Since the default is actually defined in the RFC, I would prefer this > to not be PCD configurable. Having it as an option to the shell command > makes a lot of sense, though. > > / > Leif > > On Fri, Apr 29, 2016 at 05:44:50PM +0000, Carsey, Jaben wrote: > > Maybe we should have a PCD control the default value so that people > > who build it can set it as per their own best value. Adding a > > parameter seems like a great (but separate) solution. > > > > -Jaben > > > > > -----Original Message----- > > > From: edk2-devel [mailto:[email protected]] On Behalf Of > > > Marcin Wojtas > > > Sent: Friday, April 29, 2016 2:15 AM > > > To: Fu, Siyuan <[email protected]> > > > Cc: [email protected]; [email protected]; Gao, Liming > > > <[email protected]>; [email protected]; [email protected]; > > > Kinney, Michael D <[email protected]> > > > Subject: Re: [edk2] [RFC] MdeModulePkg, ShellPkg: increase TFTP block > size > > > Importance: High > > > > > > Hi Siyuan, > > > > > > Thanks a lot for thorough explanation. The default 512B won't be > > > changed then - PCD and extending tftp command seem a good alternative. > > > We will come back later with more proper solution. > > > > > > Best regards, > > > Marcin > > > > > > 2016-04-29 10:40 GMT+02:00 Fu, Siyuan <[email protected]>: > > > > Hi, Marcin > > > > > > > > The RFC2348 has a detail discussion about the block size setting in tftp > and > > > give the answer: using value of MTU is not the best choice. You may find > you > > > still can gain more efficiency by increasing the block size over the path > MTU, > > > until the IP fragmentation gives more overhead than the tftp package > frame > > > and process. > > > > > > > > The default block size (512) of tftp is defined in the RFC1350, that's > > > > why > the > > > Mtftp driver use 512 as the default macro value. The MTFTP4 prococol > does > > > provide the interface to allow the caller to provide a "blksize" option in > the > > > MTFTP request, so I prefer to keep the default value to 512 in the Mtftp4 > > > driver, to follow RFC's definition. Another driver in edk2 stack which is > using > > > the mtftp protocol, like the PXE driver, provides a PCD > > > gEfiMdeModulePkgTokenSpaceGuid.PcdTftpBlockSize which allow the > users > > > to configure the default block size during PXE mtftp download. > > > > > > > > For the shell "tftp" command, I also suggest to add a new argument to > > > > let > > > user set the block size, not simply override it with 1468. > > > > > > > > Best Regards > > > > Siyuan > > > > > > > >> -----Original Message----- > > > >> From: edk2-devel [mailto:[email protected]] On Behalf > Of > > > >> Marcin Wojtas > > > >> Sent: Friday, April 29, 2016 9:09 AM > > > >> To: [email protected] > > > >> Cc: [email protected]; [email protected]; [email protected]; > > > Gao, > > > >> Liming <[email protected]>; Kinney, Michael D > > > >> <[email protected]> > > > >> Subject: [edk2] [RFC] MdeModulePkg, ShellPkg: increase TFTP block > size > > > >> > > > >> From: Bartosz Szczepanek <[email protected]> > > > >> > > > >> Hi, > > > >> > > > >> When dealing with low network speed during tftp, the analysis showed > > > >> two issues - UEFI tftp and network stack spend a lot of time (~3.5ms, > > > >> comparing to U-Boot's ~0.6ms) on preparing ACK packet after receiving > > > >> data block from host. The network driver itself is using SNP and its > > > >> time spent on executing SnpTransmit routine is negligible, so it is > > > >> definitely not a problem. > > > >> > > > >> This patch however is only releated to above mentioned problem. It > > > >> occurred that despite our MTU is set to 1500, each block of data has > > > >> to be split to 512 chunks, so the very long receive/ack sequence was > > > >> executed three times more than needed. > > > >> > > > >> Below is an example of a solution of increasing TFTP block size > > > >> to maximum allowed by 1500 MTU. The default block size is changed, > > > >> as well as this option is negotiatied with host. For sure a situation, > > > >> when proposed, increased block size is rejected by host (a default > > > >> fall back to 512 should happen) should be handled properly. > > > >> > > > >> Also a dynamic dependency between port's MTU was considered, but > > > there > > > >> are two problems - how to pass it to Mtftp4 module and way of > negotiating > > > >> size with host - option requires string format. Hence IMO it's better > > > >> to stick to arbitrary size. > > > >> > > > >> I would be very grateful for any comments and ideas, how to handle > > > >> this issue properly. > > > >> > > > >> Best reards, > > > >> Marcin > > > >> > > > >> --- > > > >> MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h | 2 +- > > > >> ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c | 5 +++++ > > > >> 2 files changed, 6 insertions(+), 1 deletion(-) > > > >> > > > >> diff --git > a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h > > > >> b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h > > > >> index 527fd1d..9a2d409 100644 > > > >> --- a/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h > > > >> +++ b/MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Impl.h > > > >> @@ -55,7 +55,7 @@ typedef struct _MTFTP4_PROTOCOL > > > >> MTFTP4_PROTOCOL; > > > >> #define MTFTP4_DEFAULT_SERVER_PORT 69 > > > >> #define MTFTP4_DEFAULT_TIMEOUT 3 > > > >> #define MTFTP4_DEFAULT_RETRY 5 > > > >> -#define MTFTP4_DEFAULT_BLKSIZE 512 > > > >> +#define MTFTP4_DEFAULT_BLKSIZE 1468 > > > >> #define MTFTP4_TIME_TO_GETMAP 5 > > > >> > > > >> #define MTFTP4_STATE_UNCONFIGED 0 > > > >> diff --git a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > > >> b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > > >> index 831b9c3..3e06b67 100644 > > > >> --- a/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > > >> +++ b/ShellPkg/Library/UefiShellTftpCommandLib/Tftp.c > > > >> @@ -894,12 +894,17 @@ DownloadFile ( > > > >> TftpContext->DownloadedNbOfBytes = 0; > > > >> TftpContext->LastReportedNbOfBytes = 0; > > > >> > > > >> + UINT8 name[] = "blksize"; > > > >> + UINT8 val[] = "1468"; > > > >> + EFI_MTFTP4_OPTION Blksize = { name, val }; > > > >> ZeroMem (&Mtftp4Token, sizeof (EFI_MTFTP4_TOKEN)); > > > >> Mtftp4Token.Filename = (UINT8*)AsciiFilePath; > > > >> Mtftp4Token.BufferSize = FileSize; > > > >> Mtftp4Token.Buffer = Buffer; > > > >> Mtftp4Token.CheckPacket = CheckPacket; > > > >> Mtftp4Token.Context = (VOID*)TftpContext; > > > >> + Mtftp4Token.OptionCount = 1; > > > >> + Mtftp4Token.OptionList = &Blksize; > > > >> > > > >> ShellPrintHiiEx ( > > > >> -1, -1, NULL, STRING_TOKEN (STR_TFTP_DOWNLOADING), > > > >> -- > > > >> 1.8.3.1 > > > >> > > > >> _______________________________________________ > > > >> edk2-devel mailing list > > > >> [email protected] > > > >> https://lists.01.org/mailman/listinfo/edk2-devel > > > _______________________________________________ > > > edk2-devel mailing list > > > [email protected] > > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
