On Sat, Sep 19, 2020 at 10:18:54AM -0700, Linus Torvalds wrote: > On Sat, Sep 19, 2020 at 2:50 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote: > > > > this provides a preemptible variant of kmap_atomic & related > > interfaces. This is achieved by: > > Ack. This looks really nice, even apart from the new capability. > > The only thing I really reacted to is that the name doesn't make sense > to me: "kmap_temporary()" seems a bit odd. > > Particularly for an interface that really is basically meant as a > better replacement of "kmap_atomic()" (but is perhaps also a better > replacement for "kmap()"). > > I think I understand how the name came about: I think the "temporary" > is there as a distinction from the "longterm" regular kmap(). So I > think it makes some sense from an internal implementation angle, but I > don't think it makes a lot of sense from an interface name. > > I don't know what might be a better name, but if we want to emphasize > that it's thread-private and a one-off, maybe "local" would be a > better naming, and make it distinct from the "global" nature of the > old kmap() interface? > > However, another solution might be to just use this new preemptible > "local" kmap(), and remove the old global one entirely. Yes, the old > global one caches the page table mapping and that sounds really > efficient and nice. But it's actually horribly horribly bad, because > it means that we need to use locking for them. Your new "temporary" > implementation seems to be fundamentally better locking-wise, and only > need preemption disabling as locking (and is equally fast for the > non-highmem case). > > So I wonder if the single-page TLB flush isn't a better model, and > whether it wouldn't be a lot simpler to just get rid of the old > complex kmap() entirely, and replace it with this? > > I agree we can't replace the kmap_atomic() version, because maybe > people depend on the preemption disabling it also implied. But what > about replacing the non-atomic kmap()? My concern with that is people might use kmap() and then pass the address to a different task. So we need to audit the current users of kmap() and convert any that do that into using vmap() instead. I like kmap_local(). Or kmap_thread().
