On 22.02.2018 22:01, Raman Shishniou wrote:
> On 02/22/2018 10:18 PM, Georg Chini wrote:
>>> - /* Hmm, nothing to do. Let's sleep */
>>> - pollfd->events = (short) (u->source->thread_info.state == PA_SOURCE_RUNNING ? POLLIN : 0);
>>> + /* Post data to source, discard data or wait for state transition to be complete */
>>> + if (PA_SOURCE_IS_OPENED(u->source->thread_info.state)) {
>>> +
>>> + if (u->writer_connected && u->corkfd >= 0) {
>>> + pa_assert_se(pa_close(u->corkfd) == 0);
>>> + u->corkfd = -1;
>>> + }
>>> +
>>> + if (u->memchunk.length > 0) {
>>> + pa_source_post(u->source, &u->memchunk);
>>> + pa_memblock_unref(u->memchunk.memblock);
>>> + u->memchunk.memblock = pa_memblock_new(u->core->mempool, u->pipe_size);
>>> + u->memchunk.length = 0;
>>> + }
>>> +
>>> + } else if (u->suspended_by_user)
>> You have to open the corkfd here as well to avoid POLLHUP spam if the writer
>> disconnects during user suspend.
>>
> During user suspend we are doing normal polling. If the writer will be disconnected
> we'll see it by reading 0 bytes and corkfd will be opened.
See below, I would like to avoid continuous polling during user suspend.
>
>>> + u->memchunk.length = 0;
>>> +
>>> +
>>> + pollfd->events = u->memchunk.length ? 0 : POLLIN;
>> How do you wake up from autosuspend? When the writer disconnects,
>> memchunk.length will be 0 and so you do no longer listen for POLLIN.
>> I guess you need to know the current suspend cause here:
> > user suspended -> events = 0
>> (only auto suspended + no writer connected) or running -> events = POLLIN
>> only auto suspended + writer connected (=wake up transition) -> events = 0
>>
> Yes. This is what I do, keep data in memchunk and wait while source will be resumed.
>
Yeah, looks I am too dump to understand a simple comparison ... sorry.
(Shame on
me, I know I have been making a lot of silly mistakes during my reviews
and your
knowledge of PA is better than mine. I hope I did not annoy you too much.)
But now I have another issue:
You are polling the pipe and running the loop even if the source is user
suspended.
This seems like a waste of CPU (even more than accepting some POLLIN spam
during wakeup transition). I know you do it to discard data that is
written during
suspend, but I wonder if there is no better way to discard that data
without running
the loop permanently.
I am thinking of draining the pipe in the SET_STATE handler. If you are
setting
events = 0 and open the corkfd on user suspend, nothing except messages
should wake us up. Now, when the state changes to running, you can drain the
pipe in the SET_STATE handler. The thread loop will just run through on
the first
iteration after user suspend, because revents = 0 and chunk.length = 0. Now,
because the source is open again, you can set events = POLLIN.
After that, you are back to normal.
You can safely assume writer_connected=false during user suspend (you do
not notice when the writer disconnects if you set events = 0). If the writer
is still connected after suspend, writer_connected will get set when you
read
the first data. It will cause an unnecessary unsuspend message, but this
will
have no effect because neither the suspend cause nor the state change.
I would also suggest to use a flag like set_pollin in the comparison,
set and reset
the flag in the appropriate places and explain why in a comment. This is
one of
the situations, where a little bit more code could make the concept
clearer. I don't
mind keeping it as is however, if you think it's not worth the effort.
