Date: Sun, 13 Dec 2015 15:49:22 +0100
From: Johannes Stezenbach <js@sig21.net>
To: Peter Hurley <peter@hurleysoftware.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.cz>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/6] n_tty: Always wake up read()/poll() if new input
Message-ID: <20151213144922.GA10204@sig21.net>
References: <1449958599-5533-1-git-send-email-peter@hurleysoftware.com>
 <1449958599-5533-2-git-send-email-peter@hurleysoftware.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1449958599-5533-2-git-send-email-peter@hurleysoftware.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2179
Lines: 47

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.


Thanks,
Johannes
--
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/