Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1290680

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

Show key headers only | View raw

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/6] More n_tty fixes Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100
  [PATCH 6/6] n_tty: Remove tty count checks from unthrottle Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100
  [PATCH 4/6] tty: Fix ioctl(FIOASYNC) on hungup file Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100
  [PATCH 3/6] tty: Add fasync() hung up file operation Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100
  [PATCH 1/6] n_tty: Always wake up read()/poll() if new input Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100
    Re: [PATCH 1/6] n_tty: Always wake up read()/poll() if new input Johannes Stezenbach <js@sig21.net> - 2015-12-13 15:50 +0100
      Re: [PATCH 1/6] n_tty: Always wake up read()/poll() if new input Peter Hurley <peter@hurleysoftware.com> - 2015-12-13 21:00 +0100
  [PATCH 2/6] tty, n_tty: Remove fasync() ldisc notification Peter Hurley <peter@hurleysoftware.com> - 2015-12-12 23:20 +0100

csiph-web