| Message ID | 20201009195033.3208459-23-ira.weiny@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | PMEM: Introduce stray write protection for PMEM | expand |
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. 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(). Is the idea that later, such code will be converted to use kmap_thread() instead? If not, why use one over the other? - Eric
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
On Sun, Oct 11, 2020 at 11:56:35PM -0700, Ira Weiny wrote: > > > > 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() Yes, there are tons of places that do this. Try 'git grep -A6 kmap_atomic' and look for memcpy(). Hence why I'm asking what will be the "recommended" way to do this... kunmap_thread() or kmap_atomic()? > 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). Not necessarily. In cases where either one is correct, people might not have put much thought into which of kmap() and kmap_atomic() they are using. - Eric
On 10/12/20 9:19 AM, Eric Biggers wrote: > On Sun, Oct 11, 2020 at 11:56:35PM -0700, Ira Weiny wrote: >>> 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() > Yes, there are tons of places that do this. Try 'git grep -A6 kmap_atomic' > and look for memcpy(). > > Hence why I'm asking what will be the "recommended" way to do this... > kunmap_thread() or kmap_atomic()? kmap_atomic() is always preferred over kmap()/kmap_thread(). kmap_atomic() is _much_ more lightweight since its TLB invalidation is always CPU-local and never broadcast. So, basically, unless you *must* sleep while the mapping is in place, kmap_atomic() is preferred.
On Mon, Oct 12, 2020 at 09:28:29AM -0700, Dave Hansen wrote: > kmap_atomic() is always preferred over kmap()/kmap_thread(). > kmap_atomic() is _much_ more lightweight since its TLB invalidation is > always CPU-local and never broadcast. > > So, basically, unless you *must* sleep while the mapping is in place, > kmap_atomic() is preferred. But kmap_atomic() disables preemption, so the _ideal_ interface would map it only locally, then on preemption make it global. I don't even know if that _can_ be done. But this email makes it seem like kmap_atomic() has no downsides.
On Mon, Oct 12, 2020 at 12:53:54PM -0700, Ira Weiny wrote: > On Mon, Oct 12, 2020 at 05:44:38PM +0100, Matthew Wilcox wrote: > > On Mon, Oct 12, 2020 at 09:28:29AM -0700, Dave Hansen wrote: > > > kmap_atomic() is always preferred over kmap()/kmap_thread(). > > > kmap_atomic() is _much_ more lightweight since its TLB invalidation is > > > always CPU-local and never broadcast. > > > > > > So, basically, unless you *must* sleep while the mapping is in place, > > > kmap_atomic() is preferred. > > > > But kmap_atomic() disables preemption, so the _ideal_ interface would map > > it only locally, then on preemption make it global. I don't even know > > if that _can_ be done. But this email makes it seem like kmap_atomic() > > has no downsides. > > And that is IIUC what Thomas was trying to solve. > > Also, Linus brought up that kmap_atomic() has quirks in nesting.[1] > > >From what I can see all of these discussions support the need to have something > between kmap() and kmap_atomic(). > > However, the reason behind converting call sites to kmap_thread() are different > between Thomas' patch set and mine. Both require more kmap granularity. > However, they do so with different reasons and underlying implementations but > with the _same_ resulting semantics; a thread local mapping which is > preemptable.[2] Therefore they each focus on changing different call sites. > > While this patch set is huge I think it serves a valuable purpose to identify a > large number of call sites which are candidates for this new semantic. Yes, I agree. My problem with this patch-set is that it ties it to some Intel feature that almost nobody cares about. Maybe we should care about it, but you didn't try very hard to make anyone care about it in the cover letter. For a future patch-set, I'd like to see you just introduce the new API. Then you can optimise the Intel implementation of it afterwards. Those patch-sets have entirely different reviewers.
On Mon, Oct 12, 2020 at 09:02:54PM +0100, Matthew Wilcox wrote: > On Mon, Oct 12, 2020 at 12:53:54PM -0700, Ira Weiny wrote: > > On Mon, Oct 12, 2020 at 05:44:38PM +0100, Matthew Wilcox wrote: > > > On Mon, Oct 12, 2020 at 09:28:29AM -0700, Dave Hansen wrote: > > > > kmap_atomic() is always preferred over kmap()/kmap_thread(). > > > > kmap_atomic() is _much_ more lightweight since its TLB invalidation is > > > > always CPU-local and never broadcast. > > > > > > > > So, basically, unless you *must* sleep while the mapping is in place, > > > > kmap_atomic() is preferred. > > > > > > But kmap_atomic() disables preemption, so the _ideal_ interface would map > > > it only locally, then on preemption make it global. I don't even know > > > if that _can_ be done. But this email makes it seem like kmap_atomic() > > > has no downsides. > > > > And that is IIUC what Thomas was trying to solve. > > > > Also, Linus brought up that kmap_atomic() has quirks in nesting.[1] > > > > >From what I can see all of these discussions support the need to have something > > between kmap() and kmap_atomic(). > > > > However, the reason behind converting call sites to kmap_thread() are different > > between Thomas' patch set and mine. Both require more kmap granularity. > > However, they do so with different reasons and underlying implementations but > > with the _same_ resulting semantics; a thread local mapping which is > > preemptable.[2] Therefore they each focus on changing different call sites. > > > > While this patch set is huge I think it serves a valuable purpose to identify a > > large number of call sites which are candidates for this new semantic. > > Yes, I agree. My problem with this patch-set is that it ties it to > some Intel feature that almost nobody cares about. I humbly disagree. At this level the only thing this is tied to is the idea that there are additional memory protections available which can be enabled quickly on a per-thread basis. PKS on Intel is but 1 implementation of that. Even the kmap code only has knowledge that there is something which needs to be done special on a devm page. > > Maybe we should > care about it, but you didn't try very hard to make anyone care about > it in the cover letter. Ok my bad. We have customers who care very much about restricting access to the PMEM pages to prevent bugs in the kernel from causing permanent damage to their data/file systems. I'll reword the cover letter better. > > For a future patch-set, I'd like to see you just introduce the new > API. Then you can optimise the Intel implementation of it afterwards. > Those patch-sets have entirely different reviewers. I considered doing this. But this seemed more logical because the feature is being driven by PMEM which is behind the kmap interface not by the users of the API. I can introduce a patch set with a kmap_thread() call which does nothing if that is more palatable but it seems wrong to me to do so. Ira
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index d9e52a7f3702..ff72a45a577e 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -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); } static inline void f2fs_put_page(struct page *page, int unlock)