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.

[Paul Durrant <xadimgnik@xxxxxxxxx>]

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;