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

Wed Feb 21 15:43:33 UTC 2018

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.

>
> 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).

>
> I can wait while Tanu merges his patches:
> [PATCH 5/8] pass pa_suspend_cause_t to set_state() callbacks
> [PATCH 6/8] pass pa_suspend_cause_t to SINK/SOURCE_SET_STATE handlers
> to next and reimplement the whole this patch to track suspend cause.
>
> Or save the original pipe-source behaviour right now and make another
> patch to track suspend cause after Tanu will apply his patches.
>
Lets do it in this order:
1) Wait until Tanu's patches are merged (won't take long)
2) Send a patch to track the suspend cause in thread context
3) Redo module-pipe-sink