Re: problems in commit 2d4dc890b5c8 (block: add helpers to run flush_dcache_page() against a bio and a request's pages)

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Thu, 2009-12-10 at 19:05 +0000, Russell King wrote:
> On Thu, Dec 10, 2009 at 12:20:55PM -0600, James Bottomley wrote:
> > On Thu, 2009-12-10 at 18:06 +0000, Russell King wrote:
> > > The above example code comes from non-aliasing VIPT - where for the
> > > vast majority of cases, unmapping without flush is fine provided we
> > > haven't written data.  However, unmapping with flush is required to
> > > ensure coherency with the instruction cache.
> > 
> > right, but you can check those two cases in the unmap, can't you?
> 
> How?  (I'd have thought that would've been plainly obvious since I wrote
> in the quoted bit below "_if_ you have such flags".)
> 
> > > > So I really think in kunmap(_atomic) we need to check to see if the page
> > > > was modified (using the pte flags),
> > > 
> > > That's fine _if_ you have such flags.  Not everything has - in which
> > > case, going down the route you're proposing means that every single
> > > kunmap_atomic() ends up having to flush the whole page whether it's
> > > actually needed on an architecture "just because" - with no technical
> > > reason to actually do so.
> > > 
> > > We need the two cases separated for hardware which is not PARISC.
> > 
> > So having such a flag is a requirement of the linux mm code, isn't it?
> > 
> > I thought what you did on arm was mark the page read only (even if it's
> > supposed to be read/write) and then trap on the write request and update
> > the dirty bit and set the page to read/write ... don't you do that
> > anymore?
> 
> We do that for user pages, and only user pages - it's partly maintained
> by the generic kernel code, and partly by the page table attribute
> translation.  We only make pages _user_ writable if they have the Linux
> 'write' and Linux 'dirty' bits set.  However, they remain writable from
> normal kernel stores - but we use a special instruction to access them
> which ensures that the user mode permissions get checked.
> 
> Essentially, the protections that the majority of ARM CPUs have available
> to them are:
> 
> 	User		Kernel
> 0:	No access	Read only
> 1:	No access	Read/write
> 2:	Read only	Read/write
> 3:	Read/write	Read/write
> 
> The logic we use for implementing the userspace dirty support switches
> the page permissions between case 2 and 3 - which is going to be of no
> use for kernel accesses.
> 
> Moreover, we don't map kernel RAM using 4K pages - we map it using 1MB
> section mappings to save the TLB from being cycled through.  If we were
> to apply the same principle there, we'd have to do it on a 1MB by 1MB
> basis, or take an additional memory hit with TLB usage.
> 
> So, in order to have bits for each page, what you're asking for is:
> - avoid using efficient section mappings which only use 1 level of
>   page table, map everything using 2 levels of page tables using 4K
>   pages.
> - add code to handle an additional special "dirty" bit processing for
>   kernel pages.
> 
> I think that is far too inefficient an option to even contemplate.

For 2MB pages, yes, I'd have to agree.

so the fallback proposal is adding the flush to the kunmap(_atomic) and
then adding a kunmap_noflush(_atomic) that we convert the already
flushed architecture cases to using ... is that OK?

James