On 02/23/2018 01:26 PM, Georg Chini wrote: > On 23.02.2018 11:03, Raman Shishniou wrote: >> On 02/23/2018 11:38 AM, Georg Chini wrote: >> >>> 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. >>> >> We will face two main problems if we do something like that: >> >> First problem - we don't know how writer will react to full pipe. >> If it open pipe in non-blocking mode, it will get EAGAIN on every >> write() while pipe stays full. If it open pipe in blocking mode, >> it will just stuck at write() until user unsuspend the source. >> I think both behaviors are bad for writer - it should contain a >> special code to deal with it. > > I guess that should be the problem of the writer. If it is intended > to write to a pipe, it must be able to deal with the situation that > the pipe is full. > Ok, but it can just ignore EAGAIN/EWOULDBLOCK while writing to pipe and we'll can get broken stream when user unsuspend source. Writer should do the same handling - if it got EAGAIN/EWOULDBLOCK, it should drop the buffer keeping the last frame, move it to the head of next chunk and try to write next chunk. Each writer should do something like this in order to be ready for pipe suspend. Actually this it very cheap to do poll, read data, copy some bytes inside a buffer and poll again. This is about several megabits. For example, I do this on my laptop: $ mkfifo testfifo $ dd if=testfifo of=/dev/null bs=512 And on second terminal: $ timeout 5 cat /dev/zero > testfifo Results: 6991136+0 records in 6991136+0 records out 3579461632 bytes (3.6 GB) copied, 5.0019 s, 716 MB/s I.e. 6991136 * 8 * 512 / 5 / 1000000000 ~ 5.72 Gbps And 6991136 / 5 ~ 1398227 read() -> write() pairs per second >> >> The second problem is hidden now because I temporary dropped >> a part of code that keep frame boundaries. If we drain the pipe >> as soon as user resume the source - we'll loose frame boundaries. >> Audio stream will be broken for any case except s8/u8 mono. >> Actually we have to read every time while suspended by user and drop >> whole chunk except for not completely read last frame, move it to >> the head of memchunk and do next read() at position where this frame ends. > > We could work around this I think. You just need to have the last > read fragment available in the SET_STATE handler. Then you do > not loose frame boundaries, because you continue to read where > you have stopped. > Yep, but after resume it will be part of another frame. And only if writer does right things while we a suspended. >> >> BTW, currently pipe-source PA just crashed if I try to write s24le to pipe: >> >> I decided to do not open a bug because almost whole pipe-source should >> be rewritten, and this is what I'm doing now. >> > Yes, makes sense. If your patch fixes a crash bug, it has a good > chance to get into 12.0 (which it would not have otherwise). It depends on whether Tanu's patches come to the master or not. -- Raman
