Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock

From: Alison Schofield <amsfield22@gmail.com>
To: Gargi Sharma <gs051095@gmail.com>
Cc: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Jonathan Cameron <jic23@kernel.org>,
	Julia Lawall <julia.lawall@lip6.fr>,
	outreachy-kernel@googlegroups.com, Michael.Hennerich@analog.com,
	knaack.h@gmx.de, Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	linux-iio@vger.kernel.org
Subject: Re: [Outreachy kernel] [PATCH] staging: iio: adc: Replace mlock with driver private lock
Date: Thu, 16 Mar 2017 10:45:27 -0700	[thread overview]
Message-ID: <20170316174526.GA3623@d830.WORKGROUP> (raw)
In-Reply-To: <20170316174101.GA3473@d830.WORKGROUP>

On Thu, Mar 16, 2017 at 10:41:02AM -0700, Alison Schofield wrote:
> On Wed, Mar 15, 2017 at 11:45:11PM +0530, Gargi Sharma wrote:
> > On Wed, Mar 15, 2017 at 4:14 AM, Jonathan Cameron
> > <jic23@jic23.retrosnub.co.uk> wrote:
> > >
> > >
> > > On 14 March 2017 17:36:16 GMT+00:00, Gargi Sharma <gs051095@gmail.com> wrote:
> > >>On Tue, Mar 14, 2017 at 2:24 AM, Lars-Peter Clausen <lars@metafoo.de>
> > >>wrote:
> > >>> On 03/13/2017 09:51 PM, Jonathan Cameron wrote:
> > >>> [...]
> > >>>>>> Gargi,
> > >>>>>>
> > >>>>>> Please insert the new lock above the __cacheline_aligned struct
> > >>>>>> member.
> > >>>>>
> > >>>>> I will do that, but is there any reason why the lock should be
> > >>above
> > >>>>> ____cacheline_aligned struct member?
> > >>>> Yes.  It's in fact very important that nothing comes after that.
> > >>>>
> > >>>> Will leave the why as an exercise for the reader.   I'll give the
> > >>>> hit of spi drivers that do DMA...
> > >>>
> > >>>
> > >>> One hint though, the answer is somewhere in Documentation/DMA-API.txt
> > >>
> > > >From what I have understood is that cacheline is used for keeping most
> > >>frequently accessed values in adjacent cachelines.
> > >
> > > Why? What does cache line actually mean?
> > >
> > >> We want the
> > >>cacheline_aligned at the end in the struct definition so as to avoid
> > >>any holes after the cacheline boundary.
> > >
> > > Nope.  Try
> > > http://www.pebblebay.com/a-guide-to-using-direct-memory-access-in-embedded-systems-part-two/
> > >
> > > (Hint, search for 'shares')
> > 
> > Okay, I got two things from the article:
> > 1. Hardware that uses DMA reads faster if the data is aligned at
> > 16/32/64B boundaries.
> > 2. You want to align buffer being accessed for DMa transfers to
> > prevent cache incoherence.
> > 
> > What I still don't understand is why nothing should come after
> > __cacheline_aligned struct member?
> > 
> > Thanks,
> > Gargi
> 
> Hi Gargi,
> 
> Short answer to why nothing can come after, is because it could
> indeed share the cacheline.  ___cacheline_aligned only guarantees
> alignment at the beginning of the cacheline.
> 
> I've been following and reading the posted links.  Here's another
> link, kind of ancient, but does a nice explanation of the process
> whereby that extra field, sharing your buffers cacheline, could
> lead to corruption of that field or the buffer data.
> https://lwn.net/Articles/2265/
> 
> I still haven't bottomed out on this though. I get the need
> to do this to be safe, but I'd like to understand how it fits
> in with systems that claim cache coherency.  Is this a case
> those schemes cannot handle, or, are there MP systems that
> are just not cache incoherent?
oops...a bit incoherent there. Meant to say - 
  are just not cache coherent at all!  ie..totally incoherent! 
> 
> alisons
> > >
> > >>>
> > >>--
> > >>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > >>the body of a message to majordomo@vger.kernel.org
> > >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > >
> > > --
> > > Sent from my Android device with K-9 Mail. Please excuse my brevity.