From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <54A9982E.7020309@hurleysoftware.com> Date: Sun, 04 Jan 2015 14:44:46 -0500 From: Peter Hurley MIME-Version: 1.0 To: Christian Riesch CC: Greg Kroah-Hartman , Jiri Slaby , linux-serial@vger.kernel.org, "linux-kernel@vger.kernel.org" , stable Subject: Re: [PATCH] n_tty: Fix unordered accesses to lockless read buffer References: <1419941156-5303-1-git-send-email-peter@hurleysoftware.com> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: On 01/01/2015 06:00 AM, Christian Riesch wrote: > Peter, > > Thank you for this patch! Unfortunately I had not much time for this > since my last patch submission, so I am happy that someone continued > this work. > > I have a few comments/questions, please see below. > > On Tue, Dec 30, 2014 at 1:05 PM, Peter Hurley wrote: >> Add commit_head buffer index, which the producer-side publishes >> after input processing. This ensures the consumer-side observes >> correctly-ordered writes in raw mode > > I understand that the commit_head reduces the number of memory > barriers and makes some things easier. But what is so special about > raw mode that requires the introduction of commit_head? commit_head is simply the read_head after each received buffer is processed by the input worker. In this context, I meant 'raw mode' as any non-canonical mode, ie., any mode in which copy_from_read_buf() is used by the reader. I'll replace 'raw' with 'non-canonical' instead. >> (ie., the buffer data is >> written before the buffer index is advanced). >> >> Further, remove read_cnt() and expand inline, using ACCESS_ONCE() > > "ACCESS_ONCE() and memory barriers"? This portion of the changelog refers only to read_cnt(), for which memory barriers are not required. But, while writing the explanatory fragment [1], I realized that input_available_p() must load the most recent relevant head index (either canon_head or commit_head based on the mode) because it may conclude there is no more input _and_ then observe an end-of-file condition. So I need to fix this up to do the smp_load_acquire() in input_available_p() and ACCESS_ONCE() in the *_copy_from_read_buf(). Regards, Peter Hurley [1] Strictly speaking, the ACCESS_ONCE()'s are optimizations. Neither the producer nor consumer require the most recent 'opposed' index; if the compiler elided the opposed index load, instead reusing an existing load