Subject: Re: [PATCH 1/6] n_tty: Always wake up read()/poll() if new input
To: Johannes Stezenbach <js@sig21.net>
References: <1449958599-5533-1-git-send-email-peter@hurleysoftware.com>
 <1449958599-5533-2-git-send-email-peter@hurleysoftware.com>
 <20151213144922.GA10204@sig21.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.cz>, linux-kernel@vger.kernel.org
From: Peter Hurley <peter@hurleysoftware.com>
Message-ID: <566DCCAA.1020503@hurleysoftware.com>
Date: Sun, 13 Dec 2015 11:53:14 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.4.0
MIME-Version: 1.0
In-Reply-To: <20151213144922.GA10204@sig21.net>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2599
Lines: 56

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/