Re: Kmap_atomic vs Kmap

Andrew Morton
2005-03-19T03:41:58Z

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.