On Thursday, September 29, 2022 6:06:46 PM CEST Kristen Carlson Accardi wrote: > It is not necessary to disable page faults or preemption when > using kmap calls Do you refer to the page faults disabling that kmap_atomic() provides as a side effect? Can you please clarify a little more? kmap_atomic() disables always only page faults, instead it might not disable preemption; it depends on CONFIG_PREEMPT_RT. Therefore, why are you also talking about preemption? Are you converting code which runs in atomic context regardless kmap_atomic()? If so, between kmap() and kmap_atomic(), the author(s) had only one choice, it correctly was kmap_atomic(), otherwise we might end up with a perfect recipe for triggering SAC bugs. kmap() were not suited in those cases because it might sleep. If the intents of the author are simply map a page while in atomic, so to avoid sleeping in atomic bugs, your conversions looks good. For the reasons above, can you please say something more about why this code needed a kmap_atomic() instead of calling kmap()? A different case is in choosing kmap_atomic() is there because of its side effects, despite the code is running in non atomic context until the mapping, but it needs to disable pagefaults only somewhere between the mapping and unmapping. This is a trickier case than the above-mentioned one because along with conversion developers should at least disable the pagefaults and probably, although not necessarily, also disable preemption. > , so replace kmap_atomic() and kunmap_atomic() > calls with more the more appropriate kmap_local_page() and > kunmap_local() calls. Why is kmap_local_page() better suited in general and is safe in this specific case? I think that we should provide the maintainer with well supported reasons why they should change any piece of code which is still doing what it is thought for. A mere deprecation in favour of a newer API may not be enough to change code that is still working properly (like in the old "if it's not broken, don't fix it!", or something like this :)). Thanks, Fabio > > Signed-off-by: Kristen Carlson Accardi <kristen@xxxxxxxxxxxxxxx> > --- > arch/x86/kernel/cpu/sgx/encl.c | 12 ++++++------ > arch/x86/kernel/cpu/sgx/ioctl.c | 4 ++-- > arch/x86/kernel/cpu/sgx/main.c | 8 ++++---- > 3 files changed, 12 insertions(+), 12 deletions(-) > [snip]
