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:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Marcin Wojtas
> > Sent: Friday, April 29, 2016 2:15 AM
> > To: Fu, Siyuan <siyuan...@intel.com>
> > Cc: ha...@marvell.com; edk2-devel@lists.01.org; Gao, Liming
> > <liming....@intel.com>; n...@marvell.com; leif.lindh...@linaro.org;
> > Kinney, Michael D <michael.d.kin...@intel.com>
> > 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 <siyuan...@intel.com>:
> > > 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:edk2-devel-boun...@lists.01.org] On Behalf Of
> > >> Marcin Wojtas
> > >> Sent: Friday, April 29, 2016 9:09 AM
> > >> To: edk2-devel@lists.01.org
> > >> Cc: ha...@marvell.com; leif.lindh...@linaro.org; n...@marvell.com;
> > Gao,
> > >> Liming <liming....@intel.com>; Kinney, Michael D
> > >> <michael.d.kin...@intel.com>
> > >> Subject: [edk2] [RFC] MdeModulePkg, ShellPkg: increase TFTP block size
> > >>
> > >> From: Bartosz Szczepanek <b...@semihalf.com>
> > >>
> > >> 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
> > >> edk2-devel@lists.01.org
> > >> https://lists.01.org/mailman/listinfo/edk2-devel
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
