Re: [PATCH] x86/sgx: Replace kmap/kunmap_atomic calls

[Fabio M. De Francesco]

On Thursday, October 6, 2022 10:45:56 PM CEST Dave Hansen wrote:
> On 10/6/22 13:37, Fabio M. De Francesco wrote:
> > 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()?
> 
> This question is backwards.  kmap_atomic() is the default that folks
> use.

Sorry, I can't understand why kmap_atomic() is the default. What advantage we 
get from disabling pagefaults and probably also preemption (it depends on !
PREEMPT_RT also when we don't need that the kernel runs in atomic?

Do you mean that the more code run in atomic, the less pagefaults we allow, 
the less preemption we allow, and so on, the better we get from Linux?

Do you remember that what I say above happens both on 64 bits systems as well 
as in 32 bits?

I'm a newcomer so I may be wrong, however I think that in 64 bits systems we 
gain from simply returning page_address() and from finer granularity
 (less atomic, less critical sections, instead more preemption and / or 
migration).

Why shouldn't be kmap() the default choice in a preemptible kernel if sections 
don't need that disabling of pagefaults, along with preemption (which get 
disabled many more times that only migration)?

Am I still missing anything fundamental?

> You use kmap_atomic() *always* unless you _need_ to sleep or one
> of the other kmap()-only things.

What would happen if you rely on switching in atomic as a side effect of 
kmap_atomic() and then you convert to kmap_local_page() without explicitly 
disabling, for example, preemption since who converts don't care to know if 
the code is in atomic before calling kmap_atomic() before or after the call 
(as I said there may be cases where non atomic execution must disable 
preemption for some reasons only between the mapping and the unmapping?

If I were a maintainer I wouldn't trust changes that let me think that the 
developer can't tell if we need to disable something while converting to 
kmap_local_page().

I hope this time I've been to convey more clearly my thoughts. I'm sorry for 
my scarce knowledge of English.

Thanks,

Fabio

> 
> Folks don't and shouldn't have to explain why this was using kmap_atomic().
>