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
