[pulseaudio-discuss] [PATCH v8] pipe-source: implement autosuspend option

Wed Feb 21 17:07:37 UTC 2018

On 21.02.2018 17:09, Raman Shishniou wrote:
> On 02/21/2018 06:43 PM, Georg Chini wrote:
>> On 21.02.2018 16:15, Raman Shishniou wrote:
>>> On 02/21/2018 05:59 PM, Georg Chini wrote:
>>>> On 21.02.2018 15:33, Raman Shishniou wrote:
>>>>> On 02/21/2018 05:00 PM, Georg Chini wrote:
>>>>>> On 21.02.2018 12:50, Raman Shishniou wrote:
>>>>>>> On 02/21/2018 02:24 PM, Georg Chini wrote:
>>>>>>>> On 21.02.2018 12:22, Raman Shishniou wrote:
>>>>>>>>> On 02/21/2018 12:13 PM, Raman Shishniou wrote:
>>>>>>>>>> On 02/21/2018 09:39 AM, Georg Chini wrote:
>>>>>>>>>>> On 21.02.2018 06:05, Georg Chini wrote:
>>>>>>>>>>>> On 21.02.2018 05:55, Georg Chini wrote:
>>>>>>>>>>>>> On 20.02.2018 22:34, Raman Shishniou wrote:
>>>>>>>>>>>>>> On 02/20/2018 11:04 PM, Georg Chini wrote:
>>>>>>>>>>>>>>> On 20.02.2018 19:49, Raman Shishniou wrote:
>>>>>>>>>>>>>>>> On 02/20/2018 07:02 PM, Georg Chini wrote:
>>>>>>>>>>>>>>>>> On 20.02.2018 16:38, Raman Shyshniou wrote:
>>>>>>>>>>>>>>>>>> Currently the pipe-source will remain running even if no
>>>>>>>>>>>>>>>>>> writer is connected and therefore no data is produced.
>>>>>>>>>>>>>>>>>> This patch adds the autosuspend=<bool> option to prevent this.
>>>>>>>>>>>>>>>>>> Source will stay suspended if no writer is connected.
>>>>>>>>>>>>>>>>>> This option is enabled by default.
>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>          src/modules/module-pipe-source.c | 279 +++++++++++++++++++++++++++++----------
>>>>>>>>>>>>>>>>>>          1 file changed, 212 insertions(+), 67 deletions(-)
>>>>>>>>>>>>>>>>>>
>>>>>>>>> So we must to wait source->thread_info.state will be changed to RUNNING or IDLE
>>>>>>>>> before posting any data. And the only way to wait - call some pa_rtpoll_run()
>>>>>>>>> and check the source state to be valid for posting after each call. Again,
>>>>>>>>> we must stop polling pipe while we waiting because we can get endless loop
>>>>>>>>> if source stays suspended for long time after we send a resume message.
>>>>>>>>>
>>>>>>>>> I think my algorithm implemented in this patch is the simplest way to achieve this.
>>>>>>>>>
>>>>>>>> Well, your code is not doing the right thing either. When the source gets user
>>>>>>>> suspended, there will be some (trailing) data you read from the pipe. Now you
>>>>>>>> use this data as an indicator, that the source got suspended. When the source
>>>>>>>> gets unsuspended, the first thing you do is post the trailing data that was read
>>>>>>>> when the source was suspended. And only after that you start polling the pipe
>>>>>>>> again
>>>>>>> I can't track the suspend reason in i/o thread right now. It's not copied to
>>>>>>> thread_info in pa_source struct along with state during state changes.
>>>>>>>
>>>>>>> Tanu proposed a patches that will pass pa_suspend_cause_t to SINK/SOURCE_SET_STATE
>>>>>>> handlers and set_state() callbacks. It we add suspend_cause to thread_info too,
>>>>>>> there will be easy way to discard data if we are suspended by user:
>>>>>>>
>>>>>>> if (PA_SOURCE_IS_OPENED(u->source->thread_info.state)) {
>>>>>>>         ... post data ...
>>>>>>>         chunk.length = 0;
>>>>>>> } else if (PA_SUSPEND_APPLICATION is not in thread_info->suspend_cause) {
>>>>>>>         ... discard data ...
>>>>>>>         chunk.length = 0;
>>>>>>> }
>>>>>>>
>>>>>> I see another problem. If, during suspend, a writer connects and
>>>>>> disconnects again, the pipe may be full of old data after we resume.
>>>>>> So I guess we have to read data from the pipe continuously and
>>>>>> discard it while the source is suspended.
>>>>>>
>>>>> Right now yes. This is what original code does.
>>
>> No, it does not. It stops reading the pipe, it sets events = 0 on suspend.
>>
>>>>> But if we'll track suspend cause, pending data will be discarded right after
>>>>> our unsuspend message will be processed, i.e
>>>>>
>>>>> {i/o thread} send message to main thread ->
>>>>>      {main thread} pipe_source_process_msg() will send message to i/o thread ->
>>>>>        {i/o thread} source_process_msg() will process message from main thread
>>>>>                                          and change thread_info.
>>>>>
>>>>> If PA_SUSPEND_APPLICATION not in suspend_cause but thread still suspended,
>>>>> all pending data will be discarded and pipe will be continuously monitored.
>>>>> All futher data will be posted or discarded while PA_SUSPEND_APPLICATION not in
>>>>> suspend_cause.
>>>>>
>>>>> So we will stop pipe polling only for short period to wait our unsuspend message will
>>>>> be processed.
>>>>>
>>>>> -- 
>>>>> Raman
>>>> Yes, if we track the suspend cause, things will be simpler. But then my
>>>> proposal (or rather something very similar) will work as well and I don't
>>>> believe it is necessary to stop polling completely.
>>>>
>>> We still can get POLLHUP spam while waiting for couple of milliseconds:
>>>
>>> Writer open() pipe, write() some data and close() it immediately.
>>>
>>> We got data, close corkfd, send unsuspend message, set events = 0, but will
>>> get POLLHUP every poll run until our unsuspend message will be processed.
>>
>> My proposal opens the corkfd again and keeps it open until the
>> source is running. So no POLLHUP spam.
> We will never receive 0 from pipe if we open corkfd as soon as we see POLLHUP.
Maybe you misunderstood me. What I mean, is that the pipe can be
opened for writing as long as we are suspended. So it open when
we see that the source is suspended or when we auto suspend. Close
it as soon as the source gets unsuspended. This will avoid POLLHUP
during suspend completely. While auto suspended, we additionally have
to listen for POLLIN.
This way we can only get POLLHUP (or POLLIN with no data) when the
source is running.

> Moreover, some platforms do not send POLLHUP at all. We can get POLLIN while
> waiting if writer write more data or close pipe while we still waiting.
>
>>> It will be like kernel's spin lock - endless loop for short undefined
>>> period of time (1us - 1ms) - just waste of CPU.
>>>
>>> The way we completely remove pipe fd from polling have to be
>>> simpler - without freeing and allocating memory, but this is the only
>>> way current polling implementation gives to us.
>> I still do not see it is necessary and I think there is no need that the
>> thread function is so different from all other thread functions.
>> It helps a lot when the code is consistent. Also your thread loop is
>> difficult to understand and uses indirect hints (like the length of
>> some data to indicate that the source is suspended or a file
>> descriptor is set to indicate auto suspend).
>>
> Wrong: chunk.length indicates we have a pending data.
>
> I think the loop is so simple as it can be:
>
> for (;;) {
>      { prepare to run poll - add or remove fd from poll if we a polling pipe or just waiting }
>
>      { run poll and check it's error }
>
>      { check if we have a data to read and read it }
>
>      { post data or discard it }
> }
>
> I can do it like this:
>
> for (;;) {
>      { check if we have a data to read and read it }
>
>      { post data or discard it }
>
>      { prepare to run poll - add or remove fd from poll if we a polling pipe or just waiting }
>
>      { run poll and check it's error }
> }
>
> The order is not so important, but it will look like other thread functions :)

I prefer the second order - and still see absolutely no need to 
add/remove the
file descriptor from polling. See above.