From: Jinoh Kang Subject: Re: [RFC PATCH v2 2/5] server: Add a new server request "notify_async_direct_result." Message-Id: <2ca6c62e-baed-1ef5-1b98-c15c04a1663f@gmail.com> Date: Fri, 28 Jan 2022 13:17:14 +0900 In-Reply-To: <13faa864-c9d4-8c5f-2b83-ae0b9609ab67@codeweavers.com> References: <148c894d-c8f1-bbe1-1c26-25ed265f7f37@gmail.com> <53a3b943-dd1b-12b7-b9dd-8666a139ca69@gmail.com> <98480b97-74f2-922e-83d9-bff0a4adbabe@gmail.com> <13faa864-c9d4-8c5f-2b83-ae0b9609ab67@codeweavers.com> On 1/28/22 08:32, Zebediah Figura (she/her) wrote: > On 1/27/22 13:22, Jinoh Kang wrote: >> On 1/28/22 04:13, Jinoh Kang wrote: >>> On 1/28/22 04:05, Jinoh Kang wrote: >>>> + >>>> +/* Notify direct completion of async and close the wait handle */ >>>> +DECL_HANDLER(notify_async_direct_result) >>>> +{ >>>> +    struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops ); >>>> + >>>> +    if (!async) return; >>>> + >>>> +    if (async->iosb && async->unknown_status && !async->pending && async->terminated) >>>> +    { >>>> +        /* Reactivate async.  We call async_reselect() later. */ >>>> +        async->terminated = 0; >>>> + >>>> +        /* Set result for async_handoff(). */ >>>> +        set_error( req->status ); >>>> +        async->iosb->result = req->information; >>>> + >>>> +        /* The async_handoff() call prior to the current server request was >>>> +         * effectively a no-op since async->unknown_status is 1.  Calling it >>>> +         * again with async->unknown_status = 0 will do the remaining steps. >>>> +         */ >>>> +        async->unknown_status = 0; >>>> +        async_handoff( async, NULL, 0 ); >>>> + >>>> +        if (get_error() == STATUS_PENDING) >>>> +        { >>>> +            async_reselect( async ); >>>> +        } >>>> +        else if (NT_ERROR( get_error() )) >>>> +        { >>>> +            /* synchronous I/O failure: don't invoke callbacks, only dequeue it. */ >>>> +            async_dequeue( async ); >>> >>> Note: we can't use async_set_result() here.  async_handoff() leaves async->terminated as 0, and async_set_result() has "assert( async->terminated )". >>> >>> Thus, reusing async_set_result() here requires either: >>> >>> 1. Setting async->pending to 0 so that async_handoff() will call async_terminate(). >>>     This is obviously incorrect since unwanted IOCP packets and APCs will fire on synchronous failure. >>> >>> 2. Not using async_handoff().  We have to copy a lot of code (particulary setting pending and direct_result flags) out of async_handoff() to do this. >> >> Maybe just merge both branches (error/success) and force async_terminate() before calling async_set_result()? > > Actually, I believe the right thing to do is just to set async->terminated = 1. In fact, I believe you should set async->terminated = 1 in the recv_socket request, Yes, it's already done in async_handoff(). Note the precondition above: > + if (async->iosb && async->unknown_status && !async->pending && async->terminated) > + { > + /* Reactivate async. We call async_reselect() later. */ > + async->terminated = 0; > otherwise async_waiting() will return 1 and the server will end up spinning and/or trying to wake the async. (Granted, that could be protected against by checking unknown_status, but...) > > There are a few different ways to look at this, but one way (and what I think makes the most sense to me) is that we're completing the async as if it had been alerted, much as we would from a kernel APC, except that we're also setting the initial status, which wasn't known yet. Hence we want to go through async_set_result(). > > Along these lines, if you also set async->alerted = 1, you get the STATUS_PENDING "restart the wait" behaviour for free. Which makes conceptual sense as well. I'm honestly split off as to which side should take the responsibility of setting "async->alerted = 1", async_handoff() or notify_async_direct_result. I suppose I'm going with the former, since that's where STATUS_ALERTED is received. But then, shall the latter have "assert( async->alerted );"...? > > I suppose this is why you mentioned direct_result in [1]—i.e. we could just async_terminate(STATUS_ALERTED) in recv_socket. I can see both approaches working, but I think that open-coding the relevant parts of async_terminate would be better than trying to get direct_result manipulation exactly right. I agree. I hope we don't also have to also open code "async_handoff()", though. > Of course, I also haven't tried... > > [1] https://www.winehq.org/pipermail/wine-devel/2022-January/205774.html That said, the more I work with the async code, the more I learn to hate all those status bitfields... I think it's much better off merging as many states as possible into a single enum, so that the lifecycle is explicit (e.g. ASYNC_INITIAL -> ASYNC_QUEUED -> ASYNC_ALERTED -> ...) (I'm aware it was much worse before that, making the status field serve the dual role of async state and I/O status.) -- Sincerely, Jinoh Kang