On Tue, Jan 28, 2025 at 09:37:20AM +0900, Sergey Senozhatsky wrote: > On (25/01/27 21:26), Yosry Ahmed wrote: > > On Mon, Jan 27, 2025 at 04:59:30PM +0900, Sergey Senozhatsky wrote: > > > Introduce new API to map/unmap zsmalloc handle/object. The key > > > difference is that this API does not impose atomicity restrictions > > > on its users, unlike zs_map_object() which returns with page-faults > > > and preemption disabled > > > > I think that's not entirely accurate, see below. > > Preemption is disabled via zspage-s rwlock_t - zs_map_object() returns > with it being locked and it's being unlocked in zs_unmap_object(). Then > the function disables pagefaults and per-CPU local lock (protects per-CPU > vm-area) additionally disables preemption. Right, I meant it does not always disable page faults. > > > [..] > > > @@ -1309,12 +1297,14 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle, > > > goto out; > > > } > > > > > > - /* this object spans two pages */ > > > - zpdescs[0] = zpdesc; > > > - zpdescs[1] = get_next_zpdesc(zpdesc); > > > - BUG_ON(!zpdescs[1]); > > > + ret = area->vm_buf; > > > + /* disable page faults to match kmap_local_page() return conditions */ > > > + pagefault_disable(); > > > > Is this accurate/necessary? I am looking at kmap_local_page() and I > > don't see it. Maybe that's remnant from the old code using > > kmap_atomic()? > > No, this does not look accuare nor neccesary to me. I asume that's from > a very long time ago, but regardless of that I don't really understand > why that API wants to resemblwe kmap_atomic() (I think that was the > intention). This interface if expected to be gone so I didn't want > to dig into it and fix it. My assumption has been that back when we were using kmap_atomic(), which disables page faults, we wanted to make this API's behavior consistent for users where or not we called kmap_atomic() -- so this makes sure it always disables page faults. Now that we switched to kmap_local_page(), which doesn't disable page faults, this was left behind, ulitmately making the interface inconsistent and contradicting the purpose of its existence. This is 100% speculation on my end :) Anyway, if this function will be removed soon then it's not worth revisiting it now.
