Re: Kmap_atomic vs Kmap

[Andrew Morton <akpm@osdl.org>]

Phillip Lougher <phillip@lougher.demon.co.uk> wrote:
>
> Andrew Morton wrote in relation to my initial SquashFS patch:
> > Phillip Lougher <phillip@lougher.demon.co.uk> wrote:
> > 
> >>+skip_read:
> >>+	memset(pageaddr + bytes, 0, PAGE_CACHE_SIZE - bytes);
> >>+	kunmap(page);
> >>+	flush_dcache_page(page);
> >>+	SetPageUptodate(page);
> >>+	unlock_page(page);
> >>+
> >>+	return 0;
> >>+}
> 
> > See if you can use kmap_atomic() here - kmap() is slow, theoretically
> > deadlocky and is deprecated.
> > 
> 
>  From some browsing of the kernel source and a useful article on lwn.net 
> I think I can replace all the readpage_xxx (symlink, readpage & 
> readpage4k) kmap/kunmaps with kmap_atomic(page, KM_USER0)/kunmap(kaddr, 
> KM_USER0)...

Yes.

Generally the only reason the kernel needs to modify pagecache pages is to
do a quick memset or to copy a string in there or something like that.  So
a quick kmap_atomic/memory copy/kunmap_atomic() cycle is a fairly common
pattern.

The one exception is the directory-in-pagecache code where we tend to do a
lot of stuff while holding the kmap.  Not that there's any particular
reason why we needed to do it that way.

> The lwn.net article mentions that k[un]map_atomic is the new alternative 
> to kmap (as Andrew Morton mentioned).
> 
> I've noticed in asm-ppc/highmem.h the following comment
> 
> /*
>   * The use of kmap_atomic/kunmap_atomic is discouraged - kmap/kunmap
>   * gives a more generic (and caching) interface. But kmap_atomic can
>   * be used in IRQ contexts, so in some (very limited) cases we need
>   * it.
>   */
> 
> Given what Andrew and the lwn.net article says, this comment is rather 
> confusing.  Is it wrong?

I'd say so, yes.  For a start, kmap_high() takes a global lock.