From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-184.mta1.migadu.com (out-184.mta1.migadu.com [95.215.58.184]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0976A78C91 for ; Tue, 28 Jan 2025 00:49:58 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.184 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738025401; cv=none; b=XXSOpqIdOLcSPjABXH7SDMVrCkkfOhTXtb8mFxBhmrcViaBmC5WGmNkFcS39PKh4jmUiDR8PhT31E6S3oNLsyyfLLRRITB5vOrx3qQCjrc8cjgbxGMd5iOZD/5nOCLvyxlAO5iOoHe+S647pYXVZN1aPccoTbVk+QNKuFGC2Uco= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738025401; c=relaxed/simple; bh=7iJ5FnIyAabgd1lq7F2thBcRUWxRC6Pe+oFF8b7EIn8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=BUDFsP+IJI73VHXA7dZrxWTPR/JYKeUIVk9gQr2Z5N1BWRcSZGJQYUF82kX4owZYJPAk5uSrLpNRjQW4hVcHZZpFuaXSIYz4dtp4XVNeBFbeAF6bhvV0I1BalOWc4rEGGB5+Uix0WbzQHS0yeFLkxFeV0Ca0C7lXRCXX4B5uA60= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=fBtaD+NG; arc=none smtp.client-ip=95.215.58.184 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="fBtaD+NG" Date: Tue, 28 Jan 2025 00:49:51 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1738025396; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=3+M2viXR3QWyg4+/xPoeU8KN8F4xQf41rmON2IdtU7M=; b=fBtaD+NG/2sfPKu5QsEmlCxIvGJnruwjyDLgUiuBuUzVHCau7y1EFbxwnsyYGjbOT5ROiI TiSkh0BAR7DLg5EELOb5TuIciHEBH8LfsrdrP0UQ0TDzfCWAqIDB83j5OmDN6ysCT7xKvJ 7i7msW2EdyBOq6Sr9PESq2pODTJ6kIA= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yosry Ahmed To: Sergey Senozhatsky Cc: Andrew Morton , Minchan Kim , Johannes Weiner , Nhat Pham , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 5/6] zsmalloc: introduce handle mapping API Message-ID: References: <20250127080254.1302026-1-senozhatsky@chromium.org> <20250127080254.1302026-6-senozhatsky@chromium.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Migadu-Flow: FLOW_OUT 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.