On 09/24/2016 08:02 AM, Ard Biesheuvel wrote:
On 24 September 2016 at 02:00, Daniil Egranov daniil.egranov@arm.com wrote:
On 09/23/2016 03:10 AM, Ard Biesheuvel wrote:
(+ Alan)
On 23 September 2016 at 00:49, Daniil Egranov daniil.egranov@arm.com wrote:
- Corrected Ethernet frame size to 1538.
- Performance optimization. Removed extra CopyMem() call.
Could you split this in 2 patches please?
Will do.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Daniil Egranov daniil.egranov@arm.com
NOTE: The driver requires [edk2] [PATCH 0/2] Juno PCI fixes
Drivers/Net/MarvellYukonDxe/if_msk.c | 22 +++++++++------------- Drivers/Net/MarvellYukonDxe/if_msk.h | 2 +- 2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.c b/Drivers/Net/MarvellYukonDxe/if_msk.c index 6fb8af4..8e9b726 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.c +++ b/Drivers/Net/MarvellYukonDxe/if_msk.c @@ -1876,7 +1876,6 @@ msk_rxeof ( struct msk_rxdesc *rxd; INTN cons; INTN rxlen;
MSK_DMA_BUF m;
DEBUG ((EFI_D_NET, "Marvell Yukon: rxeof\n"));
@@ -1906,16 +1905,6 @@ msk_rxeof ( rxd = &sc_if->msk_cdata.msk_rxdesc[cons]; #endif
- gBS->CopyMem (&m, &rxd->rx_m, sizeof(m));
- Status = msk_newbuf (sc_if, cons);
- if (EFI_ERROR (Status)) {
// This is a dropped packet, but we aren't counting drops// Reuse old buffermsk_discard_rxbuf (sc_if, cons);What happened to dropping the packet in this case?
I am going to review this part again. I guess we have an assumption that the driver will drop received messages in case of memory allocation issue but keep going.
No, it allocates a new buffer (and hands it to the device) before handling the contents of the old buffer. The old buffer is simply reused by the device, and not processed any further, if the packet needs to be dropped for any reason.
Yes, that's true. msk_newbuf() overwrites rxd, so the m copy is necessary. It's the way the original FreeBSD driver does it.
I don't see much reason to deviate significantly from the original driver in this case. In cases where we do deviate, there should be a good reason. The original FreeBSD driver is well-proven across many different Marvell parts and architectures, and while it might be tempting to do some refactoring, I don't believe the benefits outweigh the risks. There are more Marvell parts than we have the ability to test, and the same is true with architectures.
If you want to get rid of the gBS->CopyMem(), why not simply assign 'rxd->rx_m' to 'm' field by field? That should help the compiler optimize away the in-memory version of 'm' entirely
Will copping the receive buffer pointer to the queue buffer pointer be a more optimal way?
No, you need 'm' to store the values between the time that you allocate a new buffer for the device and process the contents of the old one.
This is correct. The m is necessary. See the FreeBSD driver. It's even called "m".
https://github.com/freebsd/freebsd/blob/master/sys/dev/msk/if_msk.c#L3181
break;- }
Status = mPciIo->Flush (mPciIo); if (EFI_ERROR (Status)) { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to Flush DMA\n"));@@ -1932,14 +1921,21 @@ msk_rxeof ( if (!EFI_ERROR (Status)) { gBS->SetMem (m_link, sizeof (MSK_LINKED_SYSTEM_BUF), 0); m_link->Signature = RX_MBUF_SIGNATURE;
m_link->SystemBuf.Buf = m.Buf;
m_link->SystemBuf.Buf = rxd->rx_m.Buf; m_link->SystemBuf.Length = len; InsertTailList (&mSoftc->ReceiveQueueHead, &m_link->Link); } else { DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate receivebuffer link. Dropping Frame\n"));
gBS->FreePool (m.Buf);
gBS->FreePool (rxd->rx_m.Buf); }- Status = msk_newbuf (sc_if, cons);
- if (EFI_ERROR (Status)) {
DEBUG ((EFI_D_NET, "Marvell Yukon: failed to allocate DMAbuffer\n"));
break;}
} while (0);
MSK_RX_INC (sc_if->msk_cdata.msk_rx_cons, MSK_RX_RING_CNT);
diff --git a/Drivers/Net/MarvellYukonDxe/if_msk.h b/Drivers/Net/MarvellYukonDxe/if_msk.h index 66513d3..64ddeac 100644 --- a/Drivers/Net/MarvellYukonDxe/if_msk.h +++ b/Drivers/Net/MarvellYukonDxe/if_msk.h @@ -15,7 +15,7 @@
#include <Uefi.h>
-#define MAX_SUPPORTED_PACKET_SIZE (1500) +#define MAX_SUPPORTED_PACKET_SIZE (1538)
This looks reasonable, but could you still explain why you need to make this change?
With 1500 size tftp transfer gets messed up. I missed UDP part here. The driver receives Layer 2 Ethernet frame with 18 (Ethernet) + 20 (IPv4) + 8 (UDP) + 1500 (max payload) = 1546. If driver will support jumbo frames, it should be changed accordingly. Do we care about frame size for IPv6 case?
IPv6 yes. Jumbo frames no, since, looking at the original driver, this is not trivial to implement.
The 1500 is the max payload for ethernet, not for UDP. The UDP headers don't enter into it. The number you are looking for is 1518 (1500+6+6+4+2). https://en.wikipedia.org/wiki/Ethernet_frame#Structure
Jumbo support is in the original BSD driver, but I agree that there's no reason to do it in UEFI. Let's not lose sight of the fact that this is firmware. Gigabit networking with 1500-byte frames should provide enough bandwidth.
Alan.