Re: [ros-dev] [ros-diffs] [tfaber] 69236: [MSFS] - Use a NULL timeout for infinite waits instead of waiting for 100 ns. CORE-10188 #resolve - Wait for available read data in user mode to handle thread termination - Return S...

[Alex Ionescu]

I'm not convinced on the user->kernel changes Thomas.

Best regards,
Alex Ionescu

On Tue, Sep 15, 2015 at 5:40 AM, <tfaber(a)svn.reactos.org> wrote:

>   Author: tfaber
>  Date: Tue Sep 15 09:40:30 2015
>  New Revision: 69236
>
>  URL: http://svn.reactos.org/svn/reactos?rev=69236&view=rev
>  Log:
>  [MSFS]
>  - Use a NULL timeout for infinite waits instead of waiting for 100 ns.
>  CORE-10188 #resolve
>  - Wait for available read data in user mode to handle thread termination
>  - Return STATUS_IO_TIMEOUT also for a zero-length timeout. Fixes Wine tests
>  - Avoid MmGetSystemAddressForMdl
>  - Acquiring a mutex is not a UserRequest
>
>  Modified:
>      trunk/reactos/drivers/filesystems/msfs/msfs.h
>      trunk/reactos/drivers/filesystems/msfs/rw.c
>
>  Modified: trunk/reactos/drivers/filesystems/msfs/msfs.h
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/msfs/m…
>
>  ==============================================================================
>  --- trunk/reactos/drivers/filesystems/msfs/msfs.h       [iso-8859-1]
>  (original)
>  +++ trunk/reactos/drivers/filesystems/msfs/msfs.h       [iso-8859-1] Tue
>  Sep 15 09:40:30 2015
>  @@ -54,7 +54,7 @@
>
>
>   #define KeLockMutex(x) KeWaitForSingleObject(x, \
>  -                                             UserRequest, \
>  +                                             Executive, \
>                                                KernelMode, \
>                                                FALSE, \
>                                                NULL);
>
>  Modified: trunk/reactos/drivers/filesystems/msfs/rw.c
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/filesystems/msfs/r…
>
>  ==============================================================================
>  --- trunk/reactos/drivers/filesystems/msfs/rw.c [iso-8859-1] (original)
>  +++ trunk/reactos/drivers/filesystems/msfs/rw.c [iso-8859-1] Tue Sep 15
>  09:40:30 2015
>  @@ -29,6 +29,7 @@
>       ULONG LengthRead = 0;
>       PVOID Buffer;
>       NTSTATUS Status;
>  +    PLARGE_INTEGER Timeout;
>
>       DPRINT("MsfsRead(DeviceObject %p Irp %p)\n", DeviceObject, Irp);
>
>  @@ -52,16 +53,21 @@
>
>       Length = IoStack->Parameters.Read.Length;
>       if (Irp->MdlAddress)
>  -        Buffer = MmGetSystemAddressForMdl (Irp->MdlAddress);
>  +        Buffer = MmGetSystemAddressForMdlSafe(Irp->MdlAddress,
>  NormalPagePriority);
>       else
>           Buffer = Irp->UserBuffer;
>
>  +    if (Fcb->TimeOut.QuadPart == -1LL)
>  +        Timeout = NULL;
>  +    else
>  +        Timeout = &Fcb->TimeOut;
>  +
>       Status = KeWaitForSingleObject(&Fcb->MessageEvent,
>                                      UserRequest,
>  -                                   KernelMode,
>  +                                   UserMode,
>                                      FALSE,
>  -                                   &Fcb->TimeOut);
>  -    if (NT_SUCCESS(Status))
>  +                                   Timeout);
>  +    if (Status != STATUS_USER_APC)
>       {
>           if (Fcb->MessageCount > 0)
>           {
>  @@ -84,7 +90,7 @@
>                   KeClearEvent(&Fcb->MessageEvent);
>               }
>           }
>  -        else if (Fcb->TimeOut.QuadPart != 0LL)
>  +        else
>           {
>               /* No message found after waiting */
>               Status = STATUS_IO_TIMEOUT;
>  @@ -135,7 +141,7 @@
>
>       Length = IoStack->Parameters.Write.Length;
>       if (Irp->MdlAddress)
>  -        Buffer = MmGetSystemAddressForMdl (Irp->MdlAddress);
>  +        Buffer = MmGetSystemAddressForMdlSafe(Irp->MdlAddress,
>  NormalPagePriority);
>       else
>           Buffer = Irp->UserBuffer;
>
>
>
>

[Thomas Faber]

You mean KernelMode->UserMode in the KeWaitForSingleObject call?
The only purpose of this wait is to fulfill a user request (namely a
read operation). If the thread performing the read is terminated,
there's no reason to stick around in msfs until data is available in the
mailslot. The wait should instead be aborted and the thread allowed
to terminate, which is exactly what this change will achieve.


On 2015-09-16 02:27, Alex Ionescu wrote:
>   I'm not convinced on the user->kernel changes
> Thomas.
>  
>  Best regards,
>  Alex Ionescu
>  
>  On Tue, Sep 15, 2015 at 5:40 AM, <tfaber(a)svn.reactos.org> wrote:
> > --- trunk/reactos/drivers/filesystems/msfs/rw.c [iso-8859-1] (original)
> > +++ trunk/reactos/drivers/filesystems/msfs/rw.c [iso-8859-1] Tue Sep 15
> > 09:40:30 2015
> > @@ -52,16 +53,21 @@
> >
> >      Length = IoStack->Parameters.Read.Length;
> >      if (Irp->MdlAddress)
> > -        Buffer = MmGetSystemAddressForMdl (Irp->MdlAddress);
> > +        Buffer = MmGetSystemAddressForMdlSafe(Irp->MdlAddress,
> > NormalPagePriority);
> >      else
> >          Buffer = Irp->UserBuffer;
> >
> > +    if (Fcb->TimeOut.QuadPart == -1LL)
> > +        Timeout = NULL;
> > +    else
> > +        Timeout = &Fcb->TimeOut;
> > +
> >      Status = KeWaitForSingleObject(&Fcb->MessageEvent,
> >                                     UserRequest,
> > -                                   KernelMode,
> > +                                   UserMode,
> >                                     FALSE,
> > -                                   &Fcb->TimeOut);
> > -    if (NT_SUCCESS(Status))
> > +                                   Timeout);
> > +    if (Status != STATUS_USER_APC)
> >      {
> >          if (Fcb->MessageCount > 0)
> >          {

[Alex Ionescu]

I read the diff backward. I agree with the *actual* change, which is why I
was concerned about the opposite ;-)

Best regards,
Alex Ionescu

On Wed, Sep 16, 2015 at 5:42 AM, Thomas Faber <thomas.faber(a)reactos.org>
wrote:

>   You mean KernelMode->UserMode in the
> KeWaitForSingleObject call?
>  The only purpose of this wait is to fulfill a user request (namely a
>  read operation). If the thread performing the read is terminated,
>  there's no reason to stick around in msfs until data is available in the
>  mailslot. The wait should instead be aborted and the thread allowed
>  to terminate, which is exactly what this change will achieve.
>
>
>  On 2015-09-16 02:27, Alex Ionescu wrote:
> >   I'm not convinced on the user->kernel
> > changes Thomas.
> >
> >  Best regards,
> >  Alex Ionescu
> >
> >  On Tue, Sep 15, 2015 at 5:40 AM, <tfaber(a)svn.reactos.org> wrote:
> > > --- trunk/reactos/drivers/filesystems/msfs/rw.c [iso-8859-1] (original)
> > > +++ trunk/reactos/drivers/filesystems/msfs/rw.c [iso-8859-1] Tue Sep 15
> > > 09:40:30 2015
> > > @@ -52,16 +53,21 @@
> > >
> > >      Length = IoStack->Parameters.Read.Length;
> > >      if (Irp->MdlAddress)
> > > -        Buffer = MmGetSystemAddressForMdl (Irp->MdlAddress);
> > > +        Buffer = MmGetSystemAddressForMdlSafe(Irp->MdlAddress,
> > > NormalPagePriority);
> > >      else
> > >          Buffer = Irp->UserBuffer;
> > >
> > > +    if (Fcb->TimeOut.QuadPart == -1LL)
> > > +        Timeout = NULL;
> > > +    else
> > > +        Timeout = &Fcb->TimeOut;
> > > +
> > >      Status = KeWaitForSingleObject(&Fcb->MessageEvent,
> > >                                     UserRequest,
> > > -                                   KernelMode,
> > > +                                   UserMode,
> > >                                     FALSE,
> > > -                                   &Fcb->TimeOut);
> > > -    if (NT_SUCCESS(Status))
> > > +                                   Timeout);
> > > +    if (Status != STATUS_USER_APC)
> > >      {
> > >          if (Fcb->MessageCount > 0)
> > >          { 
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>