Path: csiph.com!goblin2!goblin.stu.neva.ru!gothmog.csi.it!bofh.it!news.nic.it!robomod
From: Peter Hurley <peter@hurleysoftware.com>
Newsgroups: linux.kernel
Subject: Re: [PATCH 1/6] n_tty: Always wake up read()/poll() if new input
Date: Sun, 13 Dec 2015 21:00:02 +0100
Message-ID: <qFkYi-4eF-3@gated-at.bofh.it>
References: <qF0Gd-8bC-3@gated-at.bofh.it> <qF0Ge-8bC-13@gated-at.bofh.it> <qFg8h-1aw-1@gated-at.bofh.it>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: robomod@news.nic.it
Approved: robomod@news.nic.it
Lines: 56
Organization: linux.* mail to news gateway
Xref: csiph.com linux.kernel:1290680

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/