| Message ID | 20221206070029.7342-1-fmdefrancesco@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/highmem: Add notes about conversions from kmap{,_atomic}() | expand |
On 2022-12-06 08:00:29 [+0100], Fabio M. De Francesco wrote: > diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rst > index 0f731d9196b0..9523e92299f6 100644 > --- a/Documentation/mm/highmem.rst > +++ b/Documentation/mm/highmem.rst > @@ -100,10 +101,21 @@ list shows them in order of preference of use. > (included in the "Functions" section) for details on how to manage nested > mappings. > > -* kmap_atomic(). This permits a very short duration mapping of a single > - page. Since the mapping is restricted to the CPU that issued it, it > - performs well, but the issuing task is therefore required to stay on that > - CPU until it has finished, lest some other task displace its mappings. > +* kmap_atomic(). This function has been deprecated; use kmap_local_page(). > + > + NOTE: Conversions to kmap_local_page() must take care to follow the mapping > + restrictions imposed on kmap_local_page(). Furthermore, code between the > + map/unmap operations may implicitly depended on the side effects of > + kmap_atomic(), such as disabling pagefaults, migration, and/or preemption. > + Such conversions should be changed to make explicit calls for those > + requirements. Furthermore, code between the kmap_atomic() and kunmap_atomic() functions may implicitly depended on the side effects of kmap_atomic() namely disabling pagefaults or preemption or both. > + [Legacy documentation] > + > + This permits a very short duration mapping of a single page. Since the > + mapping is restricted to the CPU that issued it, it performs well, but > + the issuing task is therefore required to stay on that CPU until it has > + finished, lest some other task displace its mappings. > > kmap_atomic() may also be used by interrupt contexts, since it does not > sleep and the callers too may not sleep until after kunmap_atomic() is Sebastian
On martedì 6 dicembre 2022 09:00:10 CET Sebastian Andrzej Siewior wrote: > On 2022-12-06 08:00:29 [+0100], Fabio M. De Francesco wrote: > > diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rst > > index 0f731d9196b0..9523e92299f6 100644 > > --- a/Documentation/mm/highmem.rst > > +++ b/Documentation/mm/highmem.rst > > @@ -100,10 +101,21 @@ list shows them in order of preference of use. > > > > (included in the "Functions" section) for details on how to manage nested > > mappings. > > > > -* kmap_atomic(). This permits a very short duration mapping of a single > > - page. Since the mapping is restricted to the CPU that issued it, it > > - performs well, but the issuing task is therefore required to stay on that > > - CPU until it has finished, lest some other task displace its mappings. > > +* kmap_atomic(). This function has been deprecated; use kmap_local_page(). > > + > > + NOTE: Conversions to kmap_local_page() must take care to follow the > > mapping + restrictions imposed on kmap_local_page(). Furthermore, code > > between the + map/unmap operations may implicitly depended on the side > > effects of + kmap_atomic(), such as disabling pagefaults, migration, > > and/or preemption. + Such conversions should be changed to make explicit > > calls for those + requirements. Sebastian, thanks for taking a look at my patch and replying. > Furthermore, code between the kmap_atomic() and kunmap_atomic() > functions may implicitly depended I suppose it should be "depend"? Shouldn't it? > on the side effects of kmap_atomic() > namely disabling pagefaults or preemption or both. I agree with you for rephrasing, mainly because it is written in poor English. However, I still have doubts about why you deleted "migration". AFAIK, __kmap_local_pfn_prot() always takes care of disabling migration for HIGHMEM enabled kernels. How about !HIGHMEM, where kmap_local_page() is an indirect call to page_address()? Did you mean that, if the code between kmap_atomic() and kunmap_atomic() depended on migrate_disable() (in PREEMPT_RT) we should always just stay safe and call preempt_disable() together with conversion to kmap_local_page()? If so, I understand and I again agree with you. If not, I'm missing something; so please let me understand properly. Aside from the above, I'm not sure whether you deleted the last phrase before your suggestion. What about making it to become "For the above-mentioned cases, conversions should also explicitly disable page-faults and/or preemption"? Thanks again for noticing my mistakes. Fabio > > > + [Legacy documentation] > > + > > + This permits a very short duration mapping of a single page. Since the > > + mapping is restricted to the CPU that issued it, it performs well, but > > + the issuing task is therefore required to stay on that CPU until it has > > + finished, lest some other task displace its mappings. > > > > kmap_atomic() may also be used by interrupt contexts, since it does not > > sleep and the callers too may not sleep until after kunmap_atomic() is > > Sebastian
On martedì 6 dicembre 2022 09:00:10 CET Sebastian Andrzej Siewior wrote: > On 2022-12-06 08:00:29 [+0100], Fabio M. De Francesco wrote: > > diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rst > > index 0f731d9196b0..9523e92299f6 100644 > > --- a/Documentation/mm/highmem.rst > > +++ b/Documentation/mm/highmem.rst > > @@ -100,10 +101,21 @@ list shows them in order of preference of use. > > > > (included in the "Functions" section) for details on how to manage nested > > mappings. > > > > -* kmap_atomic(). This permits a very short duration mapping of a single > > - page. Since the mapping is restricted to the CPU that issued it, it > > - performs well, but the issuing task is therefore required to stay on that > > - CPU until it has finished, lest some other task displace its mappings. > > +* kmap_atomic(). This function has been deprecated; use kmap_local_page(). > > + > > + NOTE: Conversions to kmap_local_page() must take care to follow the > > mapping + restrictions imposed on kmap_local_page(). Furthermore, code > > between the + map/unmap operations may implicitly depended on the side > > effects of + kmap_atomic(), such as disabling pagefaults, migration, > > and/or preemption. + Such conversions should be changed to make explicit > > calls for those + requirements. Sebastian, thanks for taking a look at my patch and replying. > Furthermore, code between the kmap_atomic() and kunmap_atomic() > functions may implicitly depended I suppose it should be "depend"? Shouldn't it? > on the side effects of kmap_atomic() > namely disabling pagefaults or preemption or both. I agree with you for rephrasing, mainly because it is written in poor English. However, I still have doubts about why you deleted "migration". AFAIK, __kmap_local_pfn_prot() always takes care of disabling migration for HIGHMEM enabled kernels. How about !HIGHMEM, where kmap_local_page() is an indirect call to page_address()? Did you mean that, if the code between kmap_atomic() and kunmap_atomic() depended on migrate_disable() (in PREEMPT_RT) we should always just stay safe and call preempt_disable() together with conversion to kmap_local_page()? If so, I understand and I again agree with you. If not, I'm missing something; so please let me understand properly. Aside from the above, I'm not sure whether you deleted the last phrase before your suggestion. What about making it to become "For the above-mentioned cases, conversions should also explicitly disable page-faults and/or preemption"? Thanks again for noticing my mistakes. Fabio > > > + [Legacy documentation] > > + > > + This permits a very short duration mapping of a single page. Since the > > + mapping is restricted to the CPU that issued it, it performs well, but > > + the issuing task is therefore required to stay on that CPU until it has > > + finished, lest some other task displace its mappings. > > > > kmap_atomic() may also be used by interrupt contexts, since it does not > > sleep and the callers too may not sleep until after kunmap_atomic() is > > Sebastian
On 2022-12-06 20:12:13 [+0100], Fabio M. De Francesco wrote: > > Furthermore, code between the kmap_atomic() and kunmap_atomic() > > functions may implicitly depended > > I suppose it should be "depend"? Shouldn't it? Ehm, yes, correct. > > on the side effects of kmap_atomic() > > namely disabling pagefaults or preemption or both. > > I agree with you for rephrasing, mainly because it is > written in poor English. > > However, I still have doubts about why you deleted "migration". > AFAIK, __kmap_local_pfn_prot() always takes care of disabling migration for > HIGHMEM enabled kernels. That is correct. Historically kmap_atomic() never had a migrate_disable() statement - only preempt_disable(). With disabled preemption the task migration is implicitly disabled. > How about !HIGHMEM, where kmap_local_page() is an indirect call to > page_address()? Did you mean that, if the code between kmap_atomic() and > kunmap_atomic() depended on migrate_disable() (in PREEMPT_RT) we should always > just stay safe and call preempt_disable() together with conversion to > kmap_local_page()? Even in the !HIGHMEM case it always uses preempt_disable(). With PREEMPT_RT it is different as it never disabled preemption and always did a migrate_disable() instead. If you talk about what needs to be considered while migrating away from kmap_atomic() then I wouldn't add the PREEMPT_RT bits to it since it was never in the picture while the code (using kmap_atomic()) was originally written. > If so, I understand and I again agree with you. If not, I'm missing something; > so please let me understand properly. > > Aside from the above, I'm not sure whether you deleted the last phrase before > your suggestion. What about making it to become "For the above-mentioned > cases, conversions should also explicitly disable page-faults and/or > preemption"? They need to disable preemption or page-faults or both if it is needed (not unconditionally) and where it is needed. This means not unconditionally over the whole kmap-ed section. > Thanks again for noticing my mistakes. > > Fabio Sebastian
On mercoledì 7 dicembre 2022 09:00:29 CET Sebastian Andrzej Siewior wrote: > On 2022-12-06 20:12:13 [+0100], Fabio M. De Francesco wrote: > > > Furthermore, code between the kmap_atomic() and kunmap_atomic() > > > functions may implicitly depended > > > > I suppose it should be "depend"? Shouldn't it? > > Ehm, yes, correct. > > > > on the side effects of kmap_atomic() > > > namely disabling pagefaults or preemption or both. > > > > I agree with you for rephrasing, mainly because it is > > written in poor English. > > > > However, I still have doubts about why you deleted "migration". > > AFAIK, __kmap_local_pfn_prot() always takes care of disabling migration for > > HIGHMEM enabled kernels. > > That is correct. Historically kmap_atomic() never had a > migrate_disable() statement - only preempt_disable(). With disabled > preemption the task migration is implicitly disabled. Sure, I understand this mechanism: task migration is implicitly disabled with disabled preemption. > > > How about !HIGHMEM, where kmap_local_page() is an indirect call to > > page_address()? Did you mean that, if the code between kmap_atomic() and > > kunmap_atomic() depended on migrate_disable() (in PREEMPT_RT) we should > > always just stay safe and call preempt_disable() together with conversion > > to kmap_local_page()? > > Even in the !HIGHMEM case it always uses preempt_disable(). With the only exception of PREEMPT_RT kernels, which instead use migrate_disable(). > With > PREEMPT_RT it is different as it never disabled preemption and always > did a migrate_disable() instead. OK, I see that I'm recalling correctly :-) > If you talk about what needs to be > considered while migrating away from kmap_atomic() Yes, I'm trying to explain what needs to be considered while converting from kmap_atomic() by looking at all the different cases. > then I wouldn't add > the PREEMPT_RT bits to it since it was never in the picture while the > code (using kmap_atomic()) was originally written. Ah, OK. Now I understand why you changed my last phrase. I agree with you, so I won't add anything about the special PREEMPT_RT case. > > If so, I understand and I again agree with you. If not, I'm missing > > something; so please let me understand properly. > > > > Aside from the above, I'm not sure whether you deleted the last phrase > > before > > your suggestion. What about making it to become "For the above-mentioned > > cases, conversions should also explicitly disable page-faults and/or > > preemption"? > > They need to disable preemption or page-faults or both if it is needed > (not unconditionally) and where it is needed. This means not > unconditionally over the whole kmap-ed section. I never meant to suggest to _unconditionally_ disable page-faults and/or preemption. I was only trying to say that developers must carefully check whether or not the whole kmap-ed section depended on those side effects. If so, they must _explicitly_ disable preemption or page-faults or both together with the use of kmap_local_page(). Instead, if the section doesn't depend on preemption and/or page-faults disabling, they must only replace kmap_atomic() with kmap_local_page(). I had probably used a bad wording when trying to say the same things that you wrote much more clearly. Thanks, Fabio > > > Thanks again for noticing my mistakes. > > > > Fabio > > Sebastian
On 2022-12-07 14:01:50 [+0100], Fabio M. De Francesco wrote: > > > If so, I understand and I again agree with you. If not, I'm missing > > > something; so please let me understand properly. > > > > > > Aside from the above, I'm not sure whether you deleted the last phrase > > > before > > > your suggestion. What about making it to become "For the above-mentioned > > > cases, conversions should also explicitly disable page-faults and/or > > > preemption"? > > > > They need to disable preemption or page-faults or both if it is needed > > (not unconditionally) and where it is needed. This means not > > unconditionally over the whole kmap-ed section. > > I never meant to suggest to _unconditionally_ disable page-faults > and/or preemption. I was only trying to say that developers must carefully > check whether or not the whole kmap-ed section depended on those side effects. I know. That are the two condition that should be checked/ kept in mind while replacing the code. Maybe I read it wrongly… > If so, they must _explicitly_ disable preemption or page-faults or both > together with the use of kmap_local_page(). Right. The requirement for it should be probably documented in case it is not obvious. For PREEMPT_RT it will become a problem if the preempt disabled section additionally acquired a spinlock_t or allocated memory. So ideally it won't be used ;) > Instead, if the section doesn't > depend on preemption and/or page-faults disabling, they must only replace > kmap_atomic() with kmap_local_page(). Correct and I assumed that you know all this. > I had probably used a bad wording when trying to say the same things that you > wrote much more clearly. Write it as you wish I just made a recommendation. If the wording is crystal clear then there is less room for interpretations. > Thanks, > > Fabio Sebastian
On mercoledì 7 dicembre 2022 14:51:00 CET Sebastian Andrzej Siewior wrote: > On 2022-12-07 14:01:50 [+0100], Fabio M. De Francesco wrote: > > > > If so, I understand and I again agree with you. If not, I'm missing > > > > something; so please let me understand properly. > > > > > > > > Aside from the above, I'm not sure whether you deleted the last phrase > > > > before > > > > your suggestion. What about making it to become "For the above- mentioned > > > > cases, conversions should also explicitly disable page-faults and/or > > > > preemption"? > > > > > > They need to disable preemption or page-faults or both if it is needed > > > (not unconditionally) and where it is needed. This means not > > > unconditionally over the whole kmap-ed section. > > > > I never meant to suggest to _unconditionally_ disable page-faults > > and/or preemption. I was only trying to say that developers must carefully > > check whether or not the whole kmap-ed section depended on those side > > effects. > I know. That are the two condition that should be checked/ kept in mind > while replacing the code. Maybe I read it wrongly… > > > If so, they must _explicitly_ disable preemption or page-faults or both > > together with the use of kmap_local_page(). > > Right. The requirement for it should be probably documented in case it > is not obvious. For PREEMPT_RT it will become a problem if the preempt > disabled section additionally acquired a spinlock_t or allocated memory. > So ideally it won't be used ;) > > > Instead, if the section doesn't > > > > depend on preemption and/or page-faults disabling, they must only replace > > kmap_atomic() with kmap_local_page(). > > Correct and I assumed that you know all this. > > > I had probably used a bad wording when trying to say the same things that > > you > > wrote much more clearly. > > Write it as you wish I just made a recommendation. If the wording is > crystal clear then there is less room for interpretations. I just sent v2 of this patch.[1] I hope that now I left less room for potential misinterpretation by merging your suggestion with the old text. Again thanks for helping, Fabio [1] https://lore.kernel.org/lkml/20221207225308.8290-1-fmdefrancesco@gmail.com/T/
diff --git a/Documentation/mm/highmem.rst b/Documentation/mm/highmem.rst index 0f731d9196b0..9523e92299f6 100644 --- a/Documentation/mm/highmem.rst +++ b/Documentation/mm/highmem.rst @@ -57,7 +57,8 @@ list shows them in order of preference of use. It can be invoked from any context (including interrupts) but the mappings can only be used in the context which acquired them. - This function should be preferred, where feasible, over all the others. + This function should always be used. kmap_atomic() and kmap() have been + deprecated. These mappings are thread-local and CPU-local, meaning that the mapping can only be accessed from within this thread and the thread is bound to the @@ -100,10 +101,21 @@ list shows them in order of preference of use. (included in the "Functions" section) for details on how to manage nested mappings. -* kmap_atomic(). This permits a very short duration mapping of a single - page. Since the mapping is restricted to the CPU that issued it, it - performs well, but the issuing task is therefore required to stay on that - CPU until it has finished, lest some other task displace its mappings. +* kmap_atomic(). This function has been deprecated; use kmap_local_page(). + + NOTE: Conversions to kmap_local_page() must take care to follow the mapping + restrictions imposed on kmap_local_page(). Furthermore, code between the + map/unmap operations may implicitly depended on the side effects of + kmap_atomic(), such as disabling pagefaults, migration, and/or preemption. + Such conversions should be changed to make explicit calls for those + requirements. + + [Legacy documentation] + + This permits a very short duration mapping of a single page. Since the + mapping is restricted to the CPU that issued it, it performs well, but + the issuing task is therefore required to stay on that CPU until it has + finished, lest some other task displace its mappings. kmap_atomic() may also be used by interrupt contexts, since it does not sleep and the callers too may not sleep until after kunmap_atomic() is @@ -115,11 +127,20 @@ list shows them in order of preference of use. It is assumed that k[un]map_atomic() won't fail. -* kmap(). This should be used to make short duration mapping of a single - page with no restrictions on preemption or migration. It comes with an - overhead as mapping space is restricted and protected by a global lock - for synchronization. When mapping is no longer needed, the address that - the page was mapped to must be released with kunmap(). +* kmap(). This function has been deprecated; use kmap_local_page(). + + NOTE: Conversions to kmap_local_page() must take care to follow the mapping + restrictions imposed on kmap_local_page(). In particular, it is necessary to + make sure that the kernel virtual memory pointer is only valid in the thread + that obtained it. + + [Legacy documentation] + + This should be used to make short duration mapping of a single page with no + restrictions on preemption or migration. It comes with an overhead as mapping + space is restricted and protected by a global lock for synchronization. When + mapping is no longer needed, the address that the page was mapped to must be + released with kunmap(). Mapping changes must be propagated across all the CPUs. kmap() also requires global TLB invalidation when the kmap's pool wraps and it might
kmap() and kmap_atomic() have been deprecated. kmap_local_page() should always be used in new code and the call sites of the two deprecated functions should be converted. This latter task can lead to errors if it is not carried out with the necessary attention to the context around and between the maps and unmaps. Therefore, add further information to the Highmem's documentation for the purpose to make it clearer that (1) kmap() and kmap_atomic() must not any longer be called in new code and (2) developers doing conversions from kmap() amd kmap_atomic() are expected to take care of the context around and between the maps and unmaps, in order to not break the code. Relevant parts of this patch have been taken from messages exchanged privately with Ira Weiny (thanks!). Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Ira Weiny <ira.weiny@intel.com> Cc: Jonathan Corbet <corbet@lwn.net> Cc: Mike Rapoport <rppt@linux.ibm.com> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> --- Documentation/mm/highmem.rst | 41 +++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 10 deletions(-)