[systemd-devel] [PATCH kdbus] handle: Return POLLOUT | POLLWRNORM mask when no messages are pending

[David Herrmann]

Hi

On Mon, Aug 18, 2014 at 4:15 PM, Marcel Holtmann <marcel at holtmann.org> wrote:
> Hi Lennart,
>
>>>>> To facility the feature of doing an asynchronous sending of messages
>>>>> when the bus is idle, make sure to return POLLOUT | POLLWRNORM from
>>>>> kdbus_handle_poll.
>>>>>
>>>>> Signed-off-by: Marcel Holtmann <marcel at holtmann.org>
>>>>> ---
>>>>> handle.c | 2 ++
>>>>> 1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/handle.c b/handle.c
>>>>> index ac6868133280..fc15d28351b3 100644
>>>>> --- a/handle.c
>>>>> +++ b/handle.c
>>>>> @@ -884,6 +884,8 @@ static unsigned int kdbus_handle_poll(struct file *file,
>>>>>            mask |= POLLERR | POLLHUP;
>>>>>    else if (!list_empty(&conn->msg_list))
>>>>>            mask |= POLLIN | POLLRDNORM;
>>>>> +  else
>>>>> +          mask |= POLLOUT | POLLWRNORM;
>>>>
>>>> Hmm, what's your use case here? list_empty(&conn->msg_list) only checks
>>>> the incoming list of the current connection for pending messages. That
>>>> doesn't tell you whether the bus is idle, or if your receiving end has
>>>> messages pending ...
>>>
>>> if you are a good client citizen, then you only send messages when
>>> POLLOUT gets signaled. That is what this is allowing now.
>>>
>>> Blindly sending messages is never a good idea. You want to poll for
>>> POLLOUT first. This does not make a big difference for current kernel,
>>> but it allows future extensions when the clients are well behaving and
>>> just waiting for POLLOUT. Meaning once the kernel does not signal
>>> POLLOUT, no new messages will come from the client.
>>>
>>> Current code does not do this POLLOUT before sending a messages, but
>>> our kdbus client actually does that. It is the right thing to do. And
>>> we have been doing this with all of our protocols that are using
>>> asynchronous IO. No point in kdbus being any different.
>>
>> Well, kdbus keeps per-reciever buffers only, hence signalling on the
>> kdbus fd when you are able to write is not really possible, since this
>> information is not bound to the sender fd but only to the receiever of
>> which there are many... If I understand you correctly you hence want the
>> kdbus fd to always return EPOLLOUT then, because if a client wants to
>> send something it can do that at any time?
>>
>> If that's the case then POLLOUT should really be ORed into the mask
>> unconditionally, not just in some cases...
>>
>> So, I can sympathize with what you are trying to do. However, I think
>> your patch doesn't do the right thing... It should really OR the POLLOUT
>> into all masks always.
>
> always returning POLLOUT is also fine. As long as the fd signals POLLOUT and not just swallows it.

Yes, we should add POLLOUT unconditionally. I will push the fix.

Thanks
David