[PATCH v7] pipe-source: implement autosuspend option

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 02/20/2018 06:28 PM, Raman Shishniou wrote:
> 
> 
> On 02/20/2018 06:22 PM, Georg Chini wrote:
>> On 20.02.2018 16:13, Georg Chini wrote:
>>> On 20.02.2018 15:47, Raman Shishniou wrote:
>>>> On 02/20/2018 04:55 PM, Georg Chini wrote:
>>>>> On 20.02.2018 14:30, Raman Shishniou wrote:
>>>>>> On 02/20/2018 03:11 PM, Georg Chini wrote:
>>>>>>> On 19.02.2018 16:01, 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, 215 insertions(+), 64 deletions(-)
>>>>>>>>
>>>>>>>> +
>>>>>>>>         for (;;) {
>>>>>>>>             int ret;
>>>>>>>> -        struct pollfd *pollfd;
>>>>>>>>     -        pollfd = pa_rtpoll_item_get_pollfd(u->rtpoll_item, NULL);
>>>>>>>> +        if (chunk.length) {
>>>>>>>> +            /* We have a pending data, let's stop polling pipe.
>>>>>>>> +             * Setting up pollfd->events = 0 is not enough to stop
>>>>>>>> +             * POLLHUP spam if all writers are closed pipe.
>>>>>>>> +             * We need to stop polling pipe completely */
>>>>>>>> +            if (rtpoll_item) {
>>>>>>>> +                pa_rtpoll_item_free(rtpoll_item);
>>>>>>>> +                rtpoll_item = NULL;
>>>>>>>> +            }
>>>>>>>> +        } else {
>>>>>>>> +            /* We have no pending data, let's start polling pipe */
>>>>>>>> +            if (rtpoll_item == NULL) {
>>>>>>>> +                rtpoll_item = pa_rtpoll_item_new(u->rtpoll, PA_RTPOLL_NEVER, 1);
>>>>>>>> +                pollfd = pa_rtpoll_item_get_pollfd(rtpoll_item, NULL);
>>>>>>>> +                pollfd->events = POLLIN;
>>>>>>>> +                pollfd->fd = u->fd;
>>>>>>>> +            }
>>>>>>>> +        }
>>>>>>> Why do you need to do that? As in your previous patches you
>>>>>>> open the pipe for writing if all writers are disconnected, which
>>>>>>> should stop POLLHUP's.
>>>>>> This patch tries to be platform-independent.
>>>>>> Unfortunately not all platforms set POLLHUP when a writer closed pipe.
>>>>> Well, all platforms either set POLLIN or POLLHUP. If you have POLLHUP,
>>>>> you know the pipe was closed. If you have POLLIN and l = 0 on your
>>>>> read, you also know the pipe was closed.
>>>>> Then you open your corkfd and stop the POLLHUP/POLLIN.
>>>>> So why do you have to stop polling completely? When the next
>>>>> POLLIN comes, you know that data has been written and POLLHUP
>>>>> can no longer happen.
>>>>>
>>>> If I open pipe for writing before all data was read, i.e. when I see
>>>> POLLHUP - read() will never return 0. Also send suspend message when
>>>> I see POLLHUP is too early. I should open pipe for writing and send
>>>> suspend message when read() return 0.
>>>>
>>>>
>>>>>> Moreover, when POLLHUP was set there may be data in pipe that must be read
>>>>>> before suspend. Linux sets POLLIN in that case. Freebsd and macos sets
>>>>>> POLLIN|POLLHUP even if no any data in pipe, but no writers left. Some platforms
>>>>>> do not set POLLHUP at all. So do read() and check it return 0 must be the only
>>>>>> condition to suspend source and open pipe for writing.
>>>>> Yes, I agree, see above, but I still don't understand why you stop polling.
>>>>>
>>>>>> Also it cover cases when source suspended by user - we read data from pipe,
>>>>>> send resume, but source stays suspended. We must stop polling pipe, but
>>>>>> pollfd->events = 0 is not enough. If the writer make a open-write-close
>>>>>> while source stays suspended we will get endless POLLHUPs
>>>>> I think this could be covered differently. You can set a flag "is_suspended"
>>>>> when you have suspended the source because the writer disconnected.
>>>>> Then you can do something like
>>>>>
>>>>> if (SOURCE_IS_OPENED(source) || is_suspended) {
>>>>>      if (!is_suspended && u->corkfd >= 0)
>>>>>         close pipe for writing
>>>>>
>>>>> ...
>>>>>
>>>>> } else if (u->corkfd < 0)
>>>>>      open pipe for writing
>>>>>
>>>>>
>>>> I need to stop polling to wait for source to be resumed if some bytes was
>>>> read and cannot be posted.
>>>>
>>>> I can get POLLHUP spam if source is suspended, some data is waiting for source
>>>> to be resumed and writer closed pipe.
>>>>
>>>> Example: source suspended by user and by autosuspend, pipe is waiting
>>>> for event (POLLIN), someone opened a pipe and send us data:
>>>>
>>>> First iteration:
>>>>
>>>> 1. poll:
>>>>        events = POLLIN
>>>>        pa_rtpoll_run()
>>>>        revents == POLLIN
>>>> 2. read:
>>>>        read() return 100 bytes
>>>>        send message to resume source
>>>> 3. post:
>>>>        source suspended, keep data in memchunk.
>>>>
>>>> Second iteration, 100 bytes waiting for source to be resumed:
>>>>
>>>> 1. poll:
>>>>        events = 0
>>>>        pa_rtpoll_run() (processed resume message, but source stays suspended)
>>>>        revents == 0
>>>> 2. read:
>>>>        some data still pending, do nothing
>>>> 3. post:
>>>>        source still suspended, keep pending data
>>>>
>>>> ***writer closed pipe***
>>>>
>>>> Third iteration, POLLHUP spam, 100 bytes still waiting for source to be resumed:
>>>
>>> Why would you get POLLHUP spam here with the code I proposed above?
>>> Because the source is suspended by user but not suspended by you,
>>> the code would open the pipe for writing (and close it again as soon
>>> as the user suspend cause is removed). And on the first iteration, you
>>> can discard the data immediately when the source is not opened.
>>
>> The way you are doing it now, the source will in fact keep running, even if it is
>> suspended by the user. One mistake above - you cannot immediately discard
>> the data because at that time you do not know if the source is user suspended
>> so you would have to discard any remaining data at some other point like that:
>>
>> if (SOURCE_IS_OPENED(source) || is_suspended) {
>>      if (!is_suspended && u->corkfd >= 0) {
>>         close pipe for writing
>>         discard remaining data
>>      }
>>
>> do all your other processing
>>
>> } else if (u->corkfd < 0)
>>      open pipe for writing
>>
> 
> The whole loop can be much easier if I can just discard user data :)
> But why I need to discard any data? I can just wait for source to be
> resumed temporary removing pipe from polling. The source will not keep
> running. It will just wait until it will be resumed.
> 
> This is exactly what the original pipe-source is doing.
> 

The code from original pipe-source:

/* Hmm, nothing to do. Let's sleep */
pollfd->events = (short) (u->source->thread_info.state == PA_SOURCE_RUNNING ? POLLIN : 0);

if ((ret = pa_rtpoll_run(u->rtpoll)) < 0)
    goto fail;

It just stop polling the pipe if source not running.
One difference - it will never get POLLHUP because pipe opened for both reading and writing.

--
Raman


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux