Siyuan,
> The FIN_SENT flag seems OK because an incoming TCP message won't
> override this flag, while the other control flag like the TIMER_REXMIT
> and RTT are more complex which need case by case analysis. There is
> no DPC mechanism when writing the first revision TCP code, so I think
> we need to review the TCP code for the similar cases to make it more
> robust.
Thanks for the analysis - can I interpret your response that the REXMIT and RTT
stuff are at risk for incorrect behavior today?
> For your question, why not update DPC "such that DPCs at the same
> TPL didn't preempt each other", the answer is that's the reason why
> DPC is introduced. The design intention of DPC is to interrupt current
> code when the preempted code is in the same TPL level with the code
> being interrupted.
> You can see that we must allow some event at same TPL level with the
> current running code to preempt, otherwise there is a deadlock.
Thanks for the explanation - this makes sense. So the fundamental problem to
be solved is that while blocking we need DPC callbacks to run to make progress
on the operation. I might argue that the current DPC design causes more
nesting than is necessary to achieve that goal. Consider the case where we saw
14 levels of nested DPC processing including overlapping TCP processing.
We have been testing a change where nesting of DPCs is prevented - in
DispatchDPC we check if we are already processing a dispatch and exit if we
are. (We also modified the dispatch loop to keep looping until the DPC queues
are emptied.) With this approach we still meet the same-TPL async execution
but avoid the whole mess of nesting.
So our original example that looked like this:
TcpTicking
DpcDispatchDpc TPL=CALLBACK
TcpTickingDpc
TcpTickingDpc Tcb: 0x49a42dd0, DelayedAck=1, CtrlFlag: 0x1019
TcpTickingDpc call TcpSendAck delayed is: 1
TcpSendAck Seq=158464588 Ack=4152002304
TcpTransmitSegment DelayedAck = 0
Nbuf SendIpPacket: DestPort=62462 Seq=158464588 Ack=4152002304 Window=19840
MnpSyncSendPacket call DispatchDpc
DpcDispatchDpc TPL=CALLBACK
Ip4AccpetFrame call DispatchDpc
DpcDispatchDpc TPL=CALLBACK
Ip4FreeTxToken call DispatchDpc
DpcDispatchDpc TPL=CALLBACK
TcpInput: DestPort=8816 Seq=4152002304 Ack=158464588 Len=1460
TcpClearTimer Tcb: 0x49a42dd0
Would become something like this:
TcpTicking
DpcDispatchDpc TPL=CALLBACK
TcpTickingDpc
TcpTickingDpc Tcb: 0x49a42dd0, DelayedAck=1, CtrlFlag: 0x1019
TcpTickingDpc call TcpSendAck delayed is: 1
TcpSendAck Seq=158464588 Ack=4152002304
TcpTransmitSegment DelayedAck = 0
Nbuf SendIpPacket: DestPort=62462 Seq=158464588 Ack=4152002304 Window=19840
MnpSyncSendPacket call DispatchDpc <- SUPPRESS, DPC ALREADY IN PROGRESS
<next DPC queued item>
Ip4AccpetFrame call DispatchDpc <- SUPPRESS, DPC ALREADY IN PROGRESS
<next DPC queued item>
Ip4FreeTxToken call DispatchDpc DispatchDpc <- SUPPRESS, DPC ALREADY IN
PROGRESS
<next DPC queued item>
TcpInput: DestPort=8816 Seq=4152002304 Ack=158464588 Len=1460
TcpClearTimer Tcb: 0x49a42dd0
As you can see the nesting is removed - specifically the troublesome
overlapping TCP operations are separated into distinct calls.
I've included the patch for this change for your consideration.
--
This resolves acknowledgement issues seen in TCP4 when overlapping
packet processing occurs for the same connection.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eugene Cohen <[email protected]>
---
MdeModulePkg/Universal/Network/DpcDxe/Dpc.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Universal/Network/DpcDxe/Dpc.c
b/MdeModulePkg/Universal/Network/DpcDxe/Dpc.c
index c605f72..bb3f6f2 100644
--- a/MdeModulePkg/Universal/Network/DpcDxe/Dpc.c
+++ b/MdeModulePkg/Universal/Network/DpcDxe/Dpc.c
@@ -40,6 +40,11 @@ UINTN mDpcQueueDepth = 0;
UINTN mMaxDpcQueueDepth = 0;
//
+// Global variable used to indicate that DPC dispatch is occurring
+//
+BOOLEAN mDpcDispatchActive = FALSE;
+
+//
// Free list of DPC entries. As DPCs are queued, entries are removed from this
// free list. As DPC entries are dispatched, DPC entries are added to the
free list.
// If the free list is empty and a DPC is queued, the free list is grown by
allocating
@@ -236,10 +241,18 @@ DpcDispatchDpc (
//
OriginalTpl = gBS->RaiseTPL (TPL_HIGH_LEVEL);
+ // If DPC dispatching is already in progress exit to prevent nested DPC
invocations
+ if (mDpcDispatchActive) {
+ gBS->RestoreTPL (OriginalTpl);
+ return EFI_NOT_FOUND;
+ }
+
+ mDpcDispatchActive = TRUE;
+
//
// Check to see if there are 1 or more DPCs currently queued
//
- if (mDpcQueueDepth > 0) {
+ while (mDpcQueueDepth > 0) {
//
// Loop from TPL_HIGH_LEVEL down to the current TPL value
//
@@ -291,6 +304,8 @@ DpcDispatchDpc (
}
}
+ mDpcDispatchActive = FALSE;
+
//
// Restore the original TPL level when this function was called
//
--
1.9.5.msysgit.0
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
