[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/6] In some cases, the frontend may stop processing Tx ring requests in a timely manner. If this happens at the same time as ring disable, then the Tx code could spin forever at dispatch IRQL.


  • To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
  • From: Paul Durrant <xadimgnik@xxxxxxxxx>
  • Date: Wed, 21 Jul 2021 18:08:46 +0100
  • Delivery-date: Wed, 21 Jul 2021 17:08:49 +0000
  • List-id: Developer list for the Windows PV Drivers subproject <win-pv-devel.lists.xenproject.org>

I assume you mean 'backend' rather than 'frontend' here?

On 20/07/2021 14:29, Martin Harvey wrote:
This patch introduces a hard limit on how long the code will spin,
allowing a device disable or power transition to complete, albeit
at the cost of Tx requests being dropped or the ring being in
an indeterminate state. This has normally been seen at guest shutdown
where final ring state is of little consequence.

Signed-off-by: Martin Harvey <martin.harvey@xxxxxxxxxx>
---
  src/xenvif/transmitter.c | 21 +++++++++++++++------
  1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/xenvif/transmitter.c b/src/xenvif/transmitter.c
index f6935a6..e114061 100644
--- a/src/xenvif/transmitter.c
+++ b/src/xenvif/transmitter.c
@@ -3933,6 +3933,8 @@ __TransmitterRingDisable(
      XenbusState                     State;
      ULONG                           Attempt;
      NTSTATUS                        status;
+    BOOLEAN                         ToProcess;
+    BOOLEAN                         Abort;
Transmitter = Ring->Transmitter;
      Frontend = Transmitter->Frontend;
@@ -3985,24 +3987,31 @@ __TransmitterRingDisable(
      }
Attempt = 0;
+    ToProcess = Ring->ResponsesProcessed != Ring->RequestsPushed;
+    Abort = ((Attempt >= 100) || (State != XenbusStateConnected));
+
      ASSERT3U(Ring->RequestsPushed, ==, Ring->RequestsPosted);
-    while (Ring->ResponsesProcessed != Ring->RequestsPushed) {

Why not leave this clause alone, and simply add '&& !Abort' after having set it FALSE above? I.e. why do we need the 'ToProcess' boolean?

+    while (ToProcess && !Abort) {
          Attempt++;
-        ASSERT(Attempt < 100);
+
+        KeStallExecutionProcessor(1000);    // 1ms
// Try to move things along
          __TransmitterRingSend(Ring);
          (VOID) TransmitterRingPoll(Ring);
- if (State != XenbusStateConnected)
-            __TransmitterRingFakeResponses(Ring);

Actually, do we even need 'Abort'? Could you not just break here...

-
          // We are waiting for a watch event at DISPATCH_LEVEL so
          // it is our responsibility to poll the store ring.
          XENBUS_STORE(Poll,
                       &Transmitter->StoreInterface);
- KeStallExecutionProcessor(1000); // 1ms
+        ToProcess = Ring->ResponsesProcessed != Ring->RequestsPushed;
+        Abort = ((Attempt >= 100) || (State != XenbusStateConnected));
+    }
+    if (Abort)

... and then check if Ring->ResponsesProcessed != Ring->RequestsPushed here (and fake responses accordingly if need be)?

+    {
+        __TransmitterRingFakeResponses(Ring);
+        (VOID) TransmitterRingPoll(Ring);
      }
Ring->Enabled = FALSE;





 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.