[PATCH 4/5] server: Added server side mamed pipe read and write implementation and use it for message mode pipes.

[Sebastian Lackner]

On 28.02.2017 14:03, Jacek Caban wrote:
> Hi Sebastian,
> 
> On 27.02.2017 19:42, Sebastian Lackner wrote:
>> On 22.02.2017 14:51, Jacek Caban wrote:
>>> Signed-off-by: Jacek Caban <jacek at codeweavers.com>
>>> ---
>>>  dlls/kernel32/tests/pipe.c |  28 ++-
>>>  dlls/ntdll/file.c          |   7 +-
>>>  dlls/ntdll/tests/file.c    |   2 +-
>>>  server/async.c             |   9 +
>>>  server/file.h              |   1 +
>>>  server/named_pipe.c        | 446
>>> ++++++++++++++++++++++++++++++++++++++++++---
>>>  6 files changed, 447 insertions(+), 46 deletions(-)
>> As I see this updated patchset allows to return the result immediately,
>> but still uses a USR1 signal to transfer back the IO completion info.
>> Wouldn't it be useful to get rid of that aswell?
> I believe it would be useful, but I assumed it does not block this
> patch. It's tricky, but I gave it some thoughts before sending patches
> and intend to look at that as some point. Here are some thoughts:

I'm not sure if this should be blocking the patch, but at least for me it
was not really obvious how you were planning to fix it afterwards.

> 
> As long as we transfer result over server socket, we don't have control
> in server over when iosb and result buffer are filled, so we need a
> signal from client when we can signal async completion (signal
> event/object, queue completion). I sent a patch that was more inline
> with how we do that for calls where I/O is implemented on client side
> [1], but Alexandre preferred that to be in server [2]. Having that in
> server is not only cleaner, but also gives us more control, like in
> which case to fill iosb or signal event. It will also make it possible
> to implement other solution (bellow).

I assume Alexandre was mainly referring to the event / APC stuff. I assume
he also would be fine with a code path to return the result immediately
when available. This could for example be done by marking the async object
as "in progress" to avoid queing unnecessary APCs. If a real result is
available at the end of the call, it could be transferred right away. This
would also help to fix the problem with "immediate returns" below.

> 
> As long as we need a way for client to signal that data is transferred,
> we need some sort of notification from client. We could add a new server
> call, that client would be responsible to call after receiving data,
> that we could use instead of wait handle and APCs, but I think that
> moving more logic to client is not the right solution.
> 
> An other solution is not to transfer result over server socket, but use
> write_process_memory() instead. This would allow us to do everything in
> a single server call. Unfortunately, it seems to me that there will be
> cases (like writing to memory what has write watch), when we will need
> APC-based fallback anyway. This is a huge change on its own, it needs at
> least:
> [...]

I don't think write_process_memory is the right way to do it. This function
also has a lot of overhead, like suspending the thread, or if /proc is not
available reading/writing is done in chunks of sizeof(long). I'm pretty sure
it would be faster to just transfer back the result with the same wineserver
call. Also please note that the client side already adds a reply buffer,
so the changes would mostly be on the server side.

>> Also please note that we need to distinguish "immediate returns" and
>> "async returns" to properly implement FileIoCompletionNotificationInformation.
>> This especially means that waiting internally is not an option.
> It's easy to distinguish and still have internal waits. I can't point
> the right solution without looking deeper at
> FileIoCompletionNotificationInformation, but just as an example, you
> could do:
> 
> if (iosb->status != STATUS_PENDING) mark_async_as_immediate_result();
> 
> inside request handlers just after calling fd ops. Then you'd have all
> the info you need inside async object. Am I missing something?

With the currently proposed solution this would not work. The fd ops
already call async_terminate -> async_set_result -> add_async_completion ->
add_completion, but depending on the completion flags this should not be
done if it is an immediate result. There is no way to know if its supposed
to be an "immediate return" inside of the async_*() helpers, and checking
afterwards is too late (unless we introduce a flag to block async objects
from doing actions right away). Did you have a different idea how it should
be implemented?

Best regards,
Sebastian