Re: [win-pv-devel] [PATCH 4/9] Rework enumeration code to work at PASSIVE_LEVEL

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel [mailto:win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On
> Behalf Of Owen Smith
> Sent: 22 April 2016 15:16
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Owen Smith
> Subject: [win-pv-devel] [PATCH 4/9] Rework enumeration code to work at
> PASSIVE_LEVEL
> 
> There should be no need to hold a lock whilst enumerating devices, as the
> insertion and removal of Pdos is already locked. This allows more
> operations to be called during creation/deletion of Pdos
> 
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>

Acked-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> ---
>  src/xenvbd/fdo.c | 151 ++++++++++++++++++++++++--------------------------
> -----
>  1 file changed, 67 insertions(+), 84 deletions(-)
> 
> diff --git a/src/xenvbd/fdo.c b/src/xenvbd/fdo.c
> index 939fb47..dadf395 100644
> --- a/src/xenvbd/fdo.c
> +++ b/src/xenvbd/fdo.c
> @@ -90,8 +90,9 @@ struct _XENVBD_FDO {
>      PXENVBD_PDO                 Targets[XENVBD_MAX_TARGETS];
> 
>      // Target Enumeration
> -    PXENVBD_THREAD              RescanThread;
> -    PXENBUS_STORE_WATCH         RescanWatch;
> +    PXENVBD_THREAD              ScanThread;
> +    KEVENT                      ScanEvent;
> +    PXENBUS_STORE_WATCH         ScanWatch;
>      PXENVBD_THREAD              FrontendThread;
> 
>      // Statistics
> @@ -123,6 +124,21 @@ __FdoSetDevicePowerState(
>      return Changed;
>  }
> 
> +static FORCEINLINE DEVICE_POWER_STATE
> +__FdoGetDevicePowerState(
> +    __in PXENVBD_FDO                Fdo
> +    )
> +{
> +    KIRQL               Irql;
> +    DEVICE_POWER_STATE  State;
> +
> +    KeAcquireSpinLock(&Fdo->Lock, &Irql);
> +    State = Fdo->DevicePower;
> +    KeReleaseSpinLock(&Fdo->Lock, Irql);
> +
> +    return State;
> +}
> +
>  __checkReturn
>  static FORCEINLINE PXENVBD_PDO
>  __FdoGetPdoAlways(
> @@ -637,48 +653,29 @@ __FdoEnumerate(
>          *NeedInvalidate |= (NT_SUCCESS(Status)) ? TRUE : FALSE;
>      }
>  }
> -__drv_requiresIRQL(DISPATCH_LEVEL)
>  static DECLSPEC_NOINLINE VOID
>  FdoScanTargets(
> -    __in    PXENVBD_FDO Fdo,
> -    __out   PBOOLEAN    NeedInvalidate,
> -    __out   PBOOLEAN    NeedReboot
> +    __in    PXENVBD_FDO Fdo
>      )
>  {
>      NTSTATUS        Status;
>      PCHAR           Buffer;
> +    BOOLEAN         NeedInvalidate;
> +    BOOLEAN         NeedReboot;
> 
> -    Trace("====>\n");
>      Status = XENBUS_STORE(Directory, &Fdo->Store, NULL, "device",
> FdoEnum(Fdo), &Buffer);
> -    if (NT_SUCCESS(Status)) {
> -        __FdoEnumerate(Fdo, Buffer, NeedInvalidate, NeedReboot);
> -        XENBUS_STORE(Free, &Fdo->Store, Buffer);
> -    } else {
> -        *NeedInvalidate = FALSE;
> -        *NeedReboot = FALSE;
> -    }
> -    Trace("<====\n");
> -}
> +    if (!NT_SUCCESS(Status))
> +        return;
> 
> -static DECLSPEC_NOINLINE VOID
> -FdoLogTargets(
> -    __in PCHAR                       Caller,
> -    __in PXENVBD_FDO                 Fdo
> -    )
> -{
> -    ULONG   TargetId;
> +    __FdoEnumerate(Fdo, Buffer, &NeedInvalidate, &NeedReboot);
> +    XENBUS_STORE(Free, &Fdo->Store, Buffer);
> 
> -    Verbose("%s ===>\n", Caller);
> -    for (TargetId = 0; TargetId < XENVBD_MAX_TARGETS; ++TargetId) {
> -        PXENVBD_PDO Pdo = __FdoGetPdoAlways(Fdo, TargetId,
> __FUNCTION__);
> -        if (Pdo) {
> -            const CHAR* Reason = PdoMissingReason(Pdo);
> -            Verbose("%s : Target[%d] = 0x%p %s\n", Caller, TargetId, Pdo,
> -                        (Reason != NULL) ? Reason : "(present)");
> -            PdoDereference(Pdo);
> -        }
> +    if (NeedInvalidate) {
> +        StorPortNotification(BusChangeDetected, Fdo, 0);
> +    }
> +    if (NeedReboot) {
> +        DriverNotifyInstaller();
>      }
> -    Verbose("%s <===\n", Caller);
>  }
> 
>  __checkReturn
> @@ -689,33 +686,27 @@ FdoScan(
>      )
>  {
>      PXENVBD_FDO     Fdo = Context;
> +    PKEVENT         Event = ThreadGetEvent(Thread);
> 
>      for (;;) {
> -        KIRQL   Irql;
> -        BOOLEAN NeedInvalidate;
> -        BOOLEAN NeedReboot;
> -
> -        if (!ThreadWait(Thread))
> -            break;
> -
> -        KeAcquireSpinLock(&Fdo->Lock, &Irql);
> -        if (Fdo->DevicePower != PowerDeviceD0) {
> -            KeReleaseSpinLock(&Fdo->Lock, Irql);
> -            continue;
> -        }
> -
> -        FdoScanTargets(Fdo, &NeedInvalidate, &NeedReboot);
> +        Trace("waiting...\n");
> 
> -        KeReleaseSpinLock(&Fdo->Lock, Irql);
> +        (VOID) KeWaitForSingleObject(Event,
> +                                     Executive,
> +                                     KernelMode,
> +                                     FALSE,
> +                                     NULL);
> +        KeClearEvent(Event);
> 
> -        if (NeedInvalidate) {
> -            FdoLogTargets("ScanThread", Fdo);
> -            StorPortNotification(BusChangeDetected, Fdo, 0);
> -        }
> +        if (ThreadIsAlerted(Thread))
> +            break;
> +
> +        if (__FdoGetDevicePowerState(Fdo) == PowerDeviceD0)
> +            FdoScanTargets(Fdo);
> 
> -        if (NeedReboot)
> -            DriverNotifyInstaller();
> +        KeSetEvent(&Fdo->ScanEvent, IO_NO_INCREMENT, FALSE);
>      }
> +    KeSetEvent(&Fdo->ScanEvent, IO_NO_INCREMENT, FALSE);
> 
>      return STATUS_SUCCESS;
>  }
> @@ -1258,13 +1249,13 @@ __FdoD3ToD0(
> 
>      (VOID) FdoSetDistribution(Fdo);
> 
> -    ASSERT3P(Fdo->RescanWatch, ==, NULL);
> +    ASSERT3P(Fdo->ScanWatch, ==, NULL);
>      Status = XENBUS_STORE(WatchAdd,
>                            &Fdo->Store,
>                            "device",
>                            FdoEnum(Fdo),
> -                          ThreadGetEvent(Fdo->RescanThread),
> -                          &Fdo->RescanWatch);
> +                          ThreadGetEvent(Fdo->ScanThread),
> +                          &Fdo->ScanWatch);
>      if (!NT_SUCCESS(Status))
>          goto fail1;
> 
> @@ -1300,8 +1291,8 @@ __FdoD0ToD3(
> 
>      (VOID) XENBUS_STORE(WatchRemove,
>                          &Fdo->Store,
> -                        Fdo->RescanWatch);
> -    Fdo->RescanWatch = NULL;
> +                        Fdo->ScanWatch);
> +    Fdo->ScanWatch = NULL;
> 
>      FdoClearDistribution(Fdo);
> 
> @@ -1539,6 +1530,7 @@ __FdoInitialize(
>      KeInitializeSpinLock(&Fdo->TargetLock);
>      KeInitializeSpinLock(&Fdo->Lock);
>      KeInitializeEvent(&Fdo->RemoveEvent, SynchronizationEvent, FALSE);
> +    KeInitializeEvent(&Fdo->ScanEvent, SynchronizationEvent, FALSE);
> 
>      Fdo->ReferenceCount = 1;
>      Fdo->Signature = FDO_SIGNATURE;
> @@ -1559,7 +1551,7 @@ __FdoInitialize(
>          goto fail2;
> 
>      // start enum thread
> -    Status = ThreadCreate(FdoScan, Fdo, &Fdo->RescanThread);
> +    Status = ThreadCreate(FdoScan, Fdo, &Fdo->ScanThread);
>      if (!NT_SUCCESS(Status))
>          goto fail3;
> 
> @@ -1588,9 +1580,9 @@ fail5:
>      Fdo->FrontendThread = NULL;
>  fail4:
>      Error("fail4\n");
> -    ThreadAlert(Fdo->RescanThread);
> -    ThreadJoin(Fdo->RescanThread);
> -    Fdo->RescanThread = NULL;
> +    ThreadAlert(Fdo->ScanThread);
> +    ThreadJoin(Fdo->ScanThread);
> +    Fdo->ScanThread = NULL;
>  fail3:
>      Error("fail3\n");
>      __FdoZeroInterfaces(Fdo);
> @@ -1634,9 +1626,9 @@ __FdoTerminate(
>      Fdo->FrontendThread = NULL;
> 
>      // stop enum thread
> -    ThreadAlert(Fdo->RescanThread);
> -    ThreadJoin(Fdo->RescanThread);
> -    Fdo->RescanThread = NULL;
> +    ThreadAlert(Fdo->ScanThread);
> +    ThreadJoin(Fdo->ScanThread);
> +    Fdo->ScanThread = NULL;
> 
>      // clear device objects
>      Fdo->DeviceObject = NULL;
> @@ -1669,6 +1661,7 @@ __FdoTerminate(
>      RtlZeroMemory(&Fdo->Enumerator, sizeof(ANSI_STRING));
>      RtlZeroMemory(&Fdo->TargetLock, sizeof(KSPIN_LOCK));
>      RtlZeroMemory(&Fdo->Lock, sizeof(KSPIN_LOCK));
> +    RtlZeroMemory(&Fdo->ScanEvent, sizeof(KEVENT));
>      RtlZeroMemory(&Fdo->RemoveEvent, sizeof(KEVENT));
>      __FdoZeroInterfaces(Fdo);
> 
> @@ -2035,26 +2028,16 @@ FdoDispatchPnp(
> 
>      case IRP_MN_QUERY_DEVICE_RELATIONS:
>          if (Stack->Parameters.QueryDeviceRelations.Type == BusRelations) {
> -            KIRQL   Irql;
> -            BOOLEAN NeedInvalidate;
> -            BOOLEAN NeedReboot;
> -
> -            KeAcquireSpinLock(&Fdo->Lock, &Irql);
> -
> -            if (Fdo->DevicePower == PowerDeviceD0) {
> -                FdoScanTargets(Fdo, &NeedInvalidate, &NeedReboot);
> -            } else {
> -                NeedInvalidate = FALSE;
> -                NeedReboot = FALSE;
> -            }
> -
> -            KeReleaseSpinLock(&Fdo->Lock, Irql);
> +            KeClearEvent(&Fdo->ScanEvent);
> +            ThreadWake(Fdo->ScanThread);
> 
> -            if (NeedInvalidate)
> -                FdoLogTargets("QUERY_RELATIONS", Fdo);
> +            Trace("waiting for scan thread\n");
> 
> -            if (NeedReboot)
> -                DriverNotifyInstaller();
> +            (VOID) KeWaitForSingleObject(&Fdo->ScanEvent,
> +                                         Executive,
> +                                         KernelMode,
> +                                         FALSE,
> +                                         NULL);
>          }
>          FdoDereference(Fdo);
>          break;
> --
> 1.9.4.msysgit.1
> 
> 
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel