Re: [PATCH 1/6] n_tty: Always wake up read()/poll() if new input

[Peter Hurley <peter@hurleysoftware.com>]

Hi Johannes,

On 12/13/2015 06:49 AM, Johannes Stezenbach wrote:
> Hi Peter,
> 
> On Sat, Dec 12, 2015 at 02:16:34PM -0800, Peter Hurley wrote:
>> A read() in non-canonical mode when VMIN > 0 and VTIME == 0 does not
>> complete until at least VMIN chars have been read (or the user buffer is
>> full). In this infrequent read mode, n_tty_read() attempts to reduce
>> wakeups by computing the amount of data still necessary to complete the
>> read (minimum_to_wake) and only waking the read()/poll() when that much
>> unread data has been processed. This is the only read mode for which
>> new data does not necessarily generate a wakeup.
>>
>> However, this optimization is broken and commonly leads to hung reads
>> even though the necessary amount of data has been received. Since the
>> optimization is of marginal value anyway, just remove the whole
>> thing. This also remedies a race between a concurrent poll() and
>> read() in this mode, where the poll() can reset the minimum_to_wake
>> of the read() (and vice versa).
> ...
>> @@ -1632,7 +1631,7 @@ static void __receive_buf(struct tty_struct *tty, const unsigned char *cp,
>>  	/* publish read_head to consumer */
>>  	smp_store_release(&ldata->commit_head, ldata->read_head);
>>  
>> -	if ((read_cnt(ldata) >= ldata->minimum_to_wake) || L_EXTPROC(tty)) {
>> +	if (read_cnt(ldata)) {
>>  		kill_fasync(&tty->fasync, SIGIO, POLL_IN);
>>  		wake_up_interruptible_poll(&tty->read_wait, POLLIN);
>>  	}
> 
> Your patch looks fine, I just want to mention that there was
> some undocumented behaviour for async IO to take VMIN
> into account for deciding when to send SIGIO, but it was
> implemented incorrectly because minimum_to_wake was
> only updated in read() and poll(), not directly by the
> tcsetattr() ioctl.  I think your change does the right
> thing to fix this case, too.  I had to debug some
> proprietary code which dynamically changed VMIN based on
> expected message size and thus sometimes wasn't woken up,
> in the end we decided to keep VMIN=1 to solve it.

I considered re-implementing the minimum_to_wake mechanism
(in a race-free way) but I'm not sure it's worth the effort
(either in initial implementation time or in maintenance head-ache).
Now that termios changes are serialized with an active reader and the
input worker, it is at least possible.


Regards,
Peter Hurley