On Fri, Oct 09, 2020 at 06:30:36PM -0700, Eric Biggers wrote:
On Sat, Oct 10, 2020 at 01:39:54AM +0100, Matthew Wilcox wrote:
On Fri, Oct 09, 2020 at 02:34:34PM -0700, Eric Biggers wrote:
On Fri, Oct 09, 2020 at 12:49:57PM -0700, ira.weiny@intel.com wrote:
The kmap() calls in this FS are localized to a single thread. To avoid the over head of global PKRS updates use the new kmap_thread() call.
@@ -2410,12 +2410,12 @@ static inline struct page *f2fs_pagecache_get_page( static inline void f2fs_copy_page(struct page *src, struct page *dst) {
- char *src_kaddr = kmap(src);
- char *dst_kaddr = kmap(dst);
- char *src_kaddr = kmap_thread(src);
- char *dst_kaddr = kmap_thread(dst);
memcpy(dst_kaddr, src_kaddr, PAGE_SIZE);
- kunmap(dst);
- kunmap(src);
- kunmap_thread(dst);
- kunmap_thread(src);
}
Wouldn't it make more sense to switch cases like this to kmap_atomic()? The pages are only mapped to do a memcpy(), then they're immediately unmapped.
Maybe you missed the earlier thread from Thomas trying to do something similar for rather different reasons ...
https://lore.kernel.org/lkml/20200919091751.011116649@linutronix.de/
I did miss it. I'm not subscribed to any of the mailing lists it was sent to.
Anyway, it shouldn't matter. Patchsets should be standalone, and not require reading random prior threads on linux-kernel to understand.
Sorry, but I did not think that the discussion above was directly related. If I'm not mistaken, Thomas' work was directed at relaxing kmap_atomic() into kmap_thread() calls. While interesting, it is not the point of this series. I want to restrict kmap() callers into kmap_thread().
For this series it was considered to change the kmap_thread() call sites to kmap_atomic(). But like I said in the cover letter kmap_atomic() is not the same semantic. It is too strict. Perhaps I should have expanded that explanation.
And I still don't really understand. After this patchset, there is still code nearly identical to the above (doing a temporary mapping just for a memcpy) that would still be using kmap_atomic().
I don't understand. You mean there would be other call sites calling:
kmap_atomic() memcpy() kunmap_atomic()
?
Is the idea that later, such code will be converted to use kmap_thread() instead? If not, why use one over the other?
The reason for the new call is that with PKS added behind kmap we have 3 levels of mapping we want.
global kmap (can span threads and sleep) 'thread' kmap (can sleep but not span threads) 'atomic' kmap (can't sleep nor span threads [by definition])
As Matthew said perhaps 'global kmaps' may be best changed to vmaps? I just don't know the details of every call site.
And since I don't know the call site details if there are kmap_thread() calls which are better off as kmap_atomic() calls I think it is worth converting them. But I made the assumption that kmap users would already be calling kmap_atomic() if they could (because it is more efficient).
Ira