Re: [edk2] [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
On 09/21/18 08:33, Wu, Jiaxin wrote: > I agree we need document something to highlight that. Siyuan/Ting, if no > objection, I will do that in separate patch instead of mixing with this new > feature support. That's a good idea (well, both: documenting the differences, and adding the documentation in a separate patch). Thanks! Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
Hi Laszlo, > > If that's the case, should we consider the three drivers under > "MdeModulePkg/Universal/Network/" deprecated, and should we abandon > them > completely? > I think the answer is NO (At least for now). In my opinion, the drivers in MdeModulePkg is also useful in some case, because it already has been used in some platform since it's lightweight to reduce the platform size requirement. As you said below, there is no openssl dependency in MdeModulePkg/ISCSI. > If that's the case, then it is really important to document. > > Because, if a user reports a networking-related error (after building > OVMF without NETWORK_IP6_ENABLE), then I wouldn't like to start > investigating, just to find out that the issue was fixed in NetworkPkg a > year earlier. > > Furthermore, if the MdeModulePkg/Universal/Network/ drivers are > deprecated, when do we intend to actually remove them from the tree? > > Oh, wait, is this related to OpenSSL? When including IScsiDxe from > NetworkPkg, OpenSSL is required. When including IScsiDxe from > MdeModulePkg, OpenSSL is not required -- but the networking > functionality may have some known bugs (which are already fixed in > NetworkPkg). > > Is this the real trade-off? > For the missed fixes, that's because we didn't take a look whether it also existed in MdeModulePkg, you know, some issues need to be analyzed case by case and most of them are usage related, but actually, it's not so such critical to the other platform. That's why we prefer/try to stay the same of those drivers in MdeModulePkg, if so, it also can help us reduce the development/testing effort against two sets of stacks. What we prefer is to fix the problem / include new feature separately. As I said before, depends on the request/report from customer. So, that's the reason why we recommend to use the stacks in NetworkPkg. Of Couse, if any issue was exposed in MdeModulePkg, we will help to investigate and fix it. I agree we need document something to highlight that. Siyuan/Ting, if no objection, I will do that in separate patch instead of mixing with this new feature support. Thanks Jiaxin ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
On 09/20/18 07:54, Wu, Jiaxin wrote: > Hi Laszlo, > > I agree there is no document to describe the detailed difference > against the overlapped network drivers the between NetworkPkg and > MdeModulePkg (except IPv4/IPv6 support ). We only declared that those > drivers should not be used at the same > (https://github.com/tianocore/tianocore.github.io/wiki/NetworkPkg-Getting-Started-Guide). > > Actually, the problem you mentioned here only exists in ISCSI/TCP/PXE > drivers - Tcp4Dxe VS TcpDxe, IScsiDxe VS IScsiDxe, UefiPxeBcDxe VS > UefiPxeBcDxe. So, as you said, it's the time for us to add some > declaration somewhere (inf or wiki) -- For those three drivers in > MdeModulePkg, they will remain unchanged until there is any specific > requirement that we need fix any issue. That will greatly reduce the > effort to maintain/test those combine of two drivers. So, we don't > recommend to use those three drivers in MdeModulePkg because they > might some issues, which has been fixed in the NetworkPkg drivers. If > you agree, we will add some statement in the corresponding *.inf > files. In OVMF, we now have: > !if $(NETWORK_IP6_ENABLE) == TRUE > NetworkPkg/Ip6Dxe/Ip6Dxe.inf > NetworkPkg/TcpDxe/TcpDxe.inf > NetworkPkg/Udp6Dxe/Udp6Dxe.inf > NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf > NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf > NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf > NetworkPkg/IScsiDxe/IScsiDxe.inf > !else > MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf > MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf > MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf > !endif My understanding has been that the three drivers from MdeModulePkg/Universal/Network/, namely Tcp4Dxe, UefiPxeBcDxe, IScsiDxe, only lack features (mainly IPv6, but perhaps some more). However, your email suggests that the three drivers in question could even miss fixes to known bugs. If that's the case, should we consider the three drivers under "MdeModulePkg/Universal/Network/" deprecated, and should we abandon them completely? If that's the case, then it is really important to document. Because, if a user reports a networking-related error (after building OVMF without NETWORK_IP6_ENABLE), then I wouldn't like to start investigating, just to find out that the issue was fixed in NetworkPkg a year earlier. Furthermore, if the MdeModulePkg/Universal/Network/ drivers are deprecated, when do we intend to actually remove them from the tree? Oh, wait, is this related to OpenSSL? When including IScsiDxe from NetworkPkg, OpenSSL is required. When including IScsiDxe from MdeModulePkg, OpenSSL is not required -- but the networking functionality may have some known bugs (which are already fixed in NetworkPkg). Is this the real trade-off? Thanks Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
Hi Laszlo, I agree there is no document to describe the detailed difference against the overlapped network drivers the between NetworkPkg and MdeModulePkg (except IPv4/IPv6 support ). We only declared that those drivers should not be used at the same (https://github.com/tianocore/tianocore.github.io/wiki/NetworkPkg-Getting-Started-Guide). Actually, the problem you mentioned here only exists in ISCSI/TCP/PXE drivers - Tcp4Dxe VS TcpDxe, IScsiDxe VS IScsiDxe, UefiPxeBcDxe VS UefiPxeBcDxe. So, as you said, it's the time for us to add some declaration somewhere (inf or wiki) -- For those three drivers in MdeModulePkg, they will remain unchanged until there is any specific requirement that we need fix any issue. That will greatly reduce the effort to maintain/test those combine of two drivers. So, we don’t recommend to use those three drivers in MdeModulePkg because they might some issues, which has been fixed in the NetworkPkg drivers. If you agree, we will add some statement in the corresponding *.inf files. Thanks, Jiaxin > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, September 19, 2018 7:25 PM > To: Wu, Jiaxin ; edk2-devel@lists.01.org > Cc: Ye, Ting ; Carsey, Jaben ; > Fu, Siyuan ; Shao, Ming > Subject: Re: [edk2] [Patch 0/5] Support windowsize to benefit tftp/pxe > download performance. > > On 09/19/18 04:20, Wu, Jiaxin wrote: > >> 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 > >> > > > > 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). > > I don't currently have a use case or "requirement" for the window size > feature to work in an OVMF build that does *not* have > NETWORK_IP6_ENABLE. So from my side, we can delay the porting until such > a need materializes (it might never materialize, of course!) > > However, because this information -- i.e., the feature separation > between the IPv4-only, and the combined IPv4/IPv6 driver -- is new to > me, can we please document it somewhere in the code, for example, in the > INF files? We don't have to spell out the TFTP window size feature by > name, just the fact that NetworkPkg's UefiPxeBcDxe has more features in > general (even such features that are orthogonal to internet protocol > version, v4 vs. v6). > > If such documentation already exists, then I missed it, sorry! > > Thanks! > Laszlo > > >> - Patch #4 (regardless of package DEC) should be extended with > >> documentation (both DEC and UNI). > >> > >> Thanks > >> Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
On 09/19/18 04:20, Wu, Jiaxin wrote: >> 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 >> > > 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). I don't currently have a use case or "requirement" for the window size feature to work in an OVMF build that does *not* have NETWORK_IP6_ENABLE. So from my side, we can delay the porting until such a need materializes (it might never materialize, of course!) However, because this information -- i.e., the feature separation between the IPv4-only, and the combined IPv4/IPv6 driver -- is new to me, can we please document it somewhere in the code, for example, in the INF files? We don't have to spell out the TFTP window size feature by name, just the fact that NetworkPkg's UefiPxeBcDxe has more features in general (even such features that are orthogonal to internet protocol version, v4 vs. v6). If such documentation already exists, then I missed it, sorry! Thanks! Laszlo >> - Patch #4 (regardless of package DEC) should be extended with >> documentation (both DEC and UNI). >> >> Thanks >> Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
> 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 > 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch 0/5] Support windowsize to benefit tftp/pxe download performance.
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 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. - Patch #4 (regardless of package DEC) should be extended with documentation (both DEC and UNI). Thanks Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
