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(-)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>> I think I need post a simple pseudo code of new thread loop because it
>>>>>>>>>>>>> was completely rewritten. There are too many changes in one patch.
>>>>>>>>>>>>> It can be difficult to see the whole picture of new main loop.
>>>>>>>>>>>> Well, I applied the patch and looked at the result. I still don't like the approach.
>>>>>>>>>>>>
>>>>>>>>>>>> I would propose this:
>>>>>>>>>>>>
>>>>>>>>>>>> auto_suspended = false;
>>>>>>>>>>>> revents = 0
>>>>>>>>>>>> events = POLLIN
>>>>>>>>>>>>
>>>>>>>>>>>> for (;;) {
>>>>>>>>>>>>
>>>>>>>>>>>> /* This is the part that is run when the source is opened
>>>>>>>>>>>> * or auto suspended
>>>>>>>>>>>> if (SOURCE_IS_OPENED(source) || auto_suspended) {
>>>>>>>>>>>>
>>>>>>>>>>>> /* Check if we wake up from user suspend */
>>>>>>>>>>>> if (corkfd >= 0 && !auto_suspended) {
>>>>>>>>>>>> len = 0
>>>>>>>>>>>> close pipe for writing
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> /* We received POLLIN or POLLHUP or both */
>>>>>>>>>>>> if (revents) {
>>>>>>>>>>>>
>>>>>>>>>>>> /* Read data from pipe */
>>>>>>>>>>>> len = read data
>>>>>>>>>>>>
>>>>>>>>>>>> /* Got data, post it */
>>>>>>>>>>>> if (len > 0) {
>>>>>>>>>>>> if (auto_suspend) {
>>>>>>>>>>>> send unsuspend message
>>>>>>>>>>>> auto_suspend = false
>>>>>>>>>>>> }
>>>>>>>>>>>> post data
>>>>>>>>>>> We cannot post data here because source still suspended. Sending resume message is not enough
>>>>>>>>>>> to immediately resume the source. We need to wait several poll runs until it will be resumed.
>>>>>>>>>>> (source->thread_info.state changed in this thread, i.e. during poll run). But we will see
>>>>>>>>>>> POLLIN and/or POLLHUP each run if we don't remove pipe fd from polling.
>>>>>>>>>> Why do we have to wait? The source will be unsuspended on the next rtpollrun.
>>>>>>>>>> I do not see why we cannot already push data. Or does something get lost?
>>>>>>>>> Why would we receive POLLIN on each run? We read the data from the pipe.
>>>>>>>>> If you think the data should not be posted, you can just skip posting and discard
>>>>>>>>> the data. According to your pseudo-code it is done like tis in your previous patch.
>>>>>>>> I should not write mails before I have woken up completely ... I see what you mean
>>>>>>>> now (and I also see that you do not discard data as I thought). But I still believe you
>>>>>>>> can post the data before the source gets unsuspended. What is the difference if the
>>>>>>>> samples are stored in the source or in the source output? Anyway we are talking
>>>>>>>> about a time frame of (very probably) less than 1 ms between sending the message
>>>>>>>> and receiving it. To ensure that the loop works as expected, auto_suspended should
>>>>>>>> be set/reset in the suspend/unsuspend message and not directly in the thread function.
>>>>>>>> POLLHUP spam cannot happen because corkfd will be opened on the first POLLHUP.
>>>>>>>> POLLIN spam cannot happen when auto_suspend is set/reset from the message
>>>>>>>> handler.
>>>>>>> Not, I can't post it here. The source may not be resumed at all after we send a resume message.
>>>>>>> Not within 1 ms, not within next hour. It can be autosuspended and suspended by user manually
>>>>>>> after it. I that case we read data and should discard it instead of posting (as you propose).
>>>>>>> But that algorithm will post data to suspended source while it suspended by user.
>>>>>>>
>>>>>>> Also auto_suspended can't be set/reset in suspend/resume message handler because it called from
>>>>>>> main context and accessed from thread context.
>>>>>>>
>>>>>>> That's why I read data and wait while source will be resumed before posting.
>>>>>>>
>>>>>> I just looked into pa_source_post() code:
>>>>>>
>>>>>> void pa_source_post(pa_source*s, const pa_memchunk *chunk) {
>>>>>> pa_source_output *o;
>>>>>> void *state = NULL;
>>>>>>
>>>>>> pa_source_assert_ref(s);
>>>>>> pa_source_assert_io_context(s);
>>>>>> pa_assert(PA_SOURCE_IS_LINKED(s->thread_info.state));
>>>>>> pa_assert(chunk);
>>>>>>
>>>>>> if (s->thread_info.state == PA_SOURCE_SUSPENDED)
>>>>>> return;
>>>>>>
>>>>>> ...
>>>>>>
>>>>>>
>>>>>> There are only 3 valid states of source to post data:
>>>>>> static inline bool PA_SOURCE_IS_LINKED(pa_source_state_t x) {
>>>>>> return x == PA_SOURCE_RUNNING || x == PA_SOURCE_IDLE || x == PA_SOURCE_SUSPENDED;
>>>>>> }
>>>>>>
>>>>>> And if the source is suspended:
>>>>>> if (s->thread_info.state == PA_SOURCE_SUSPENDED)
>>>>>> return;
>>>>>>
>>>>>> If we read some data, send resume and try to post, chunk will be just discarded
>>>>>> in pa_source_post().
>>>>>>
>>>>>> 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.
>>
>> 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.
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 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.
--
Raman
