Re: [PATCH] Standardize blkif ring cleanup iteration

[Owen Smith <owen.smith@xxxxxxxxxx>]

Its probably better to keep the same logic in all drivers, and use
LONG Index;
while (--Index >= 0)

I will rework and resubmit

Owen

________________________________________
From: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>
Sent: 08 June 2026 11:24 AM
To: Owen Smith; win-pv-devel@xxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH] Standardize blkif ring cleanup iteration

On 08/06/2026 11:03, Owen Smith wrote:
> Use post-decrement when walking back through the ring array calling
> blkif ring cleanup calls, so that the 0-th element is not missed.
>
> This is especially important for the BlkifRingDestroy call, which
> needs to stop the RingWatchdog thread during IRP_MN_REMOVE_DEVICE.
> Normally, the boot disk prevents the PnP manager issuing the
> IRP_MN_REMOVE_DEVICE IRP so the following problem doesnt occur,
> but when the boot disk is emulated, its possible to get the IRP and
> have xenvbd unload. In this case, the RingWatchdog thread can still
> be scheduled for blkif ring-0, which results in a page fault, and
> 0xCE DRIVER_UNLOADED_WITHOUT_CANCELLING_PENDING_OPERATIONS bugcheck.

Hi,

The "while (Index-- > 0)" pattern works with an unsigned Index so I
think you won't need to add the signed conversion here.

>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>   src/xenvbd/ring.c | 40 ++++++++++++++++------------------------
>   1 file changed, 16 insertions(+), 24 deletions(-)
>
> diff --git a/src/xenvbd/ring.c b/src/xenvbd/ring.c
> index d05dfc3..d28ee76 100644
> --- a/src/xenvbd/ring.c
> +++ b/src/xenvbd/ring.c
> @@ -2418,7 +2418,7 @@ RingCreate(
>       PXENVBD_TARGET          Target = FrontendGetTarget(Frontend);
>       PXENVBD_ADAPTER         Adapter = TargetGetAdapter(Target);
>       ULONG                   MaxQueues;
> -    ULONG                   Index;
> +    LONG                    Index;
>       NTSTATUS                status;
>
>       *Ring = __RingAllocate(sizeof(XENVBD_RING));
> @@ -2447,7 +2447,7 @@ RingCreate(
>           goto fail2;
>
>       Index = 0;
> -    while (Index < MaxQueues) {
> +    while (Index < (LONG) MaxQueues) {
>           PXENVBD_BLKIF_RING  BlkifRing;
>
>           status = BlkifRingCreate(*Ring, Index, &BlkifRing);
> @@ -2463,7 +2463,7 @@ RingCreate(
>   fail3:
>       Error("fail3\n");
>
> -    while (--Index > 0) {
> +    while (Index-- > 0) {
>           PXENVBD_BLKIF_RING  BlkifRing = (*Ring)->Ring[Index];
>
>           (*Ring)->Ring[Index] = NULL;
> @@ -2503,12 +2503,12 @@ RingDestroy(
>       IN  PXENVBD_RING    Ring
>       )
>   {
> -    ULONG               Index;
> +    LONG                Index;
>
> -    Index = FrontendGetMaxQueues(Ring->Frontend);
> +    Index = (LONG) FrontendGetMaxQueues(Ring->Frontend);
>       ASSERT3U(Index, !=, 0);
>
> -    while (--Index != 0) {
> +    while (Index-- > 0) {
>           PXENVBD_BLKIF_RING  BlkifRing = Ring->Ring[Index];
>
>           Ring->Ring[Index] = NULL;
> @@ -2544,7 +2544,7 @@ RingConnect(
>       )
>   {
>       ULONG               MaxQueues;
> -    ULONG               Index;
> +    LONG                Index;
>       PCHAR               Buffer;
>       NTSTATUS            status;
>
> @@ -2592,7 +2592,7 @@ RingConnect(
>
>       MaxQueues = FrontendGetNumQueues(Ring->Frontend);
>       Index = 0;
> -    while (Index < MaxQueues) {
> +    while (Index < (LONG) MaxQueues) {
>           PXENVBD_BLKIF_RING  BlkifRing = Ring->Ring[Index];
>
>           status = BlkifRingConnect(BlkifRing);
> @@ -2619,11 +2619,8 @@ fail6:
>   fail5:
>       Error("fail5\n");
>
> -    while (Index != 0) {
> -        PXENVBD_BLKIF_RING  BlkifRing;
> -
> -        --Index;
> -        BlkifRing = Ring->Ring[Index];
> +    while (Index-- > 0) {
> +        PXENVBD_BLKIF_RING  BlkifRing = Ring->Ring[Index];
>
>           BlkifRingDisconnect(BlkifRing);
>       }
> @@ -2736,14 +2733,12 @@ RingDisable(
>       IN  PXENVBD_RING    Ring
>       )
>   {
> -    ULONG               Index;
> +    LONG                Index;
>
>       Index = FrontendGetNumQueues(Ring->Frontend);
> -    while (Index != 0) {
> -        PXENVBD_BLKIF_RING  BlkifRing;
> +    while (Index-- > 0) {
> +        PXENVBD_BLKIF_RING  BlkifRing = Ring->Ring[Index];
>
> -        --Index;
> -        BlkifRing = Ring->Ring[Index];
>           BlkifRingDisable(BlkifRing);
>       }
>   }
> @@ -2753,7 +2748,7 @@ RingDisconnect(
>       IN  PXENVBD_RING    Ring
>       )
>   {
> -    ULONG               Index;
> +    LONG                Index;
>
>       XENBUS_DEBUG(Deregister,
>                    &Ring->DebugInterface,
> @@ -2762,11 +2757,8 @@ RingDisconnect(
>
>       Index = FrontendGetNumQueues(Ring->Frontend);
>
> -    while (Index != 0) {
> -        PXENVBD_BLKIF_RING  BlkifRing;
> -
> -        --Index;
> -        BlkifRing = Ring->Ring[Index];
> +    while (Index-- > 0) {
> +        PXENVBD_BLKIF_RING  BlkifRing = Ring->Ring[Index];
>
>           BlkifRingDisconnect(BlkifRing);
>       }



--
Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech