From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4409BCD3420 for ; Tue, 3 Sep 2024 08:57:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CB4B68D014C; Tue, 3 Sep 2024 04:57:44 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C64B18D0139; Tue, 3 Sep 2024 04:57:44 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B2C9C8D014C; Tue, 3 Sep 2024 04:57:44 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 94AFD8D0139 for ; Tue, 3 Sep 2024 04:57:44 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 2CFC280429 for ; Tue, 3 Sep 2024 08:57:44 +0000 (UTC) X-FDA: 82522824048.01.C2A61B7 Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf16.hostedemail.com (Postfix) with ESMTP id 7537218000E for ; Tue, 3 Sep 2024 08:57:41 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=linuxfoundation.org header.s=korg header.b="VI/EIBrH"; spf=pass (imf16.hostedemail.com: domain of gregkh@linuxfoundation.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=gregkh@linuxfoundation.org; dmarc=pass (policy=none) header.from=linuxfoundation.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1725353813; a=rsa-sha256; cv=none; b=pgQydIZnNhs3I2Y9r3nik5R6TYWV3sYTSt6aaIf1GwZ+8qDfOu+lQNDgCZKbUUiC1ZkebZ 65/qxtaN2J4PRaGzOuRsELWzZCaB/+WreL27Cbxp6PeoAbHaF8+sFOBV14FezGY1nl8/2a CNV+5rhm7gYbO5D9qRZohjTvoQbv+ok= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=linuxfoundation.org header.s=korg header.b="VI/EIBrH"; spf=pass (imf16.hostedemail.com: domain of gregkh@linuxfoundation.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=gregkh@linuxfoundation.org; dmarc=pass (policy=none) header.from=linuxfoundation.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1725353813; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=FcYkjFa6DN4L6+CVXLimEtsCutbox7Uf9vZ/5SL8H5c=; b=Gqk2/FVChb2rLNDb+MSGujYZ65uE6aTrr0OAZg1OaKFC+gZ9UXaDqVAUlS/dsLHAj43e10 EmbWAKLhmihy2vWAcLbs2JCh1iZWpVbxe1mcoV3D1pIS7eLRJRarC+XtaN7t0RvoYORwXA DnPatjHUCq8qynsJlNMP/hEdKe0SZUg= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 6DA81A415D9; Tue, 3 Sep 2024 08:57:33 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9BF3EC4CEC4; Tue, 3 Sep 2024 08:57:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1725353860; bh=tVRH57/TZY4TyvdZgVZLBbfd3Au67M0mzxPujgYYjV4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=VI/EIBrHrTzju1odFrLiNf7NVBrsC0aV4noXANAMZBNBJJejNxI/WAysFIvfUkVJT U9nGh7vFjX2X1XO96tAa8v7JV51FQcrk/3eHHoNg07iD/ilMVB5qddXFjunDcEcTNX vZnWYqYtQ2xLoV4b8zaNK+GDkdso6UzRglN8YJHg= Date: Tue, 3 Sep 2024 10:57:32 +0200 From: Greg Kroah-Hartman To: Barry Song <21cnbao@gmail.com> Cc: akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Barry Song , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Todd Kjos , Martijn Coenen , Joel Fernandes , Christian Brauner , Carlos Llamas , Suren Baghdasaryan , Tangquan Zheng Subject: Re: [PATCH] binder_alloc: Move alloc_page() out of mmap_rwsem to reduce the lock duration Message-ID: <2024090325-sublet-unsworn-b6a3@gregkh> References: <20240902225009.34576-1-21cnbao@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240902225009.34576-1-21cnbao@gmail.com> X-Stat-Signature: i5fnj7aio1ckroxzbd3dbkon1jint7dm X-Rspamd-Queue-Id: 7537218000E X-Rspam-User: X-Rspamd-Server: rspam10 X-HE-Tag: 1725353861-967946 X-HE-Meta: U2FsdGVkX18t80DME9qBP0vHfT6rm5mXYIF2Usj5w/UaTIQGdgwUVeMvComzHE92iTW2Bn1qZIddTkDMJ4CchSyLboEie6hnmqdqmGZeYHWivFb6tn0ntCIy/hsW8pVkecfKbm5d6fzr5SAAJha2ZpSKMh85aVHJeY2VwckSBduQb9iJBbS4yLqKu8eyMqS+OS85PNq2dgAjgE9mxH5TGY4+psjyzrvZEQaLrDK+964hn9Pr4ZONLQpKWGbVQniKEhLkQHVOPwOm2bhhI0SEyhaTk4B3tO+a3qrDuEjo//IN4WAB1WXa1ZJ2wU0vrWVOeRH4PUjwfSHjjtYWbb3zSIbYBSjViyTEjqtoFdPfcftIz/AxJI3Tx+BFuVdIP0N4c2FAskq4DVieABG7AUYbALJB2BJA7xmnrAwpObMB+B7/hH5wPHimKPObNqLOEaIBKg9qRWF+Fy3a8B5VlAc9wMYze8LOzprbTtHkYYVH0sngHcOz9IIAOLcnFa3eeMnxSxBM0/+bycSMLLXQBu1k8W18URnD9d3MrlRHywGYJopyp1b8PuUEJajCN3LF6+y36L4kij8uxtJdl1TGpDI1O3SYlJNvrSNARItDGkznV1V1lE17MhtCvg0KdisVWBAQPXnJsM7dsa8PV6zfiGirzLbTWXu2a/a3mYv38YIQfNtS/NFbueqfK6mD0YYJGwg8T41uGBxa/rqUkHEDZuYPVD/2ZX/tg8s738+p2EA3L9bQ/jwicdCfRWh5XS1xJNFG34KXvA+lwDEkJ4sYXPnAjKOwodcwt05o37pAZYJzoL6SRxWSyX7JDsWW6llX5rTbnE9XdhOF+Q38N5iIt0Q5aCzBIEt5JP7jjAocZiA404qZP5W1C9AHBs1XyVAngYPXhJsXcPXU+h0pe10DY3TSPIISp+rmZaq1e3kEubR9Z5NhP/7pi1fvZVrDGFQVBOg2enbZGyfjo1krKpc0aep 0Bu8m2sI NU+29Yv+rJZuq5DJr/IX41YvSyCCalb8Gqz9i49Lo+ZDeAjFpsXc2qgha+vA9XXVTy2WO2YZmRLqvgmpgcTnt+lgfld83TLD7ePh5vR0it/6yL42bn6Ua2j8rfT7Nyi4WBNOA8LveACSli6sBfLKUXl37ttxY5ktl/A9iBCqTcixAYBqJNnv85inCzArtnpqlhzIXqqaTPl7UUSt0hUilHXND2ZO/CxMk6YcXFLYMyZawND5E7WY2xPgs1X9DmyXItlxJxUACejFpxB8eYavY/EOGAacpmktVV+5XsgzPjgG7GEHJx2XIXZ75rLZRbDGMQa3fjkT9+1DikC5qzs5ZRz5KJIu8SttZF3iRjzJiWjvlAM9zaQlzLehloyN4r1HxuHOIfrWaWHpVYNXRc4nWV9dZvciNoMOfM6aV0wBSH7G0nVlGLnVkGKu2l/S+86ry5FLvhkRO+A9VaNzXGl21BUFrHY5SMFFaJmwGukaY1fYxu3h0vbLsClStByaJKDsjBeCo4z3dfMYKZAMc9UjlmF1WArxB1sHKonqD5zO7LnsVKj6SK7xpu6aXz4DdM/s6tZIY4hb+P6nYWmqbJ68fVmTOontmCppUJVpl X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Tue, Sep 03, 2024 at 10:50:09AM +1200, Barry Song wrote: > From: Barry Song > > The mmap_write_lock() can block all access to the VMAs, for example page > faults. Performing memory allocation while holding this lock may trigger > direct reclamation, leading to others being queued in the rwsem for an > extended period. > We've observed that the allocation can sometimes take more than 300ms, > significantly blocking other threads. The user interface sometimes > becomes less responsive as a result. To prevent this, let's move the > allocation outside of the write lock. > A potential side effect could be an extra alloc_page() for the second > thread executing binder_install_single_page() while the first thread > has done it earlier. However, according to Tangquan's 48-hour profiling > using monkey, the likelihood of this occurring is minimal, with a ratio > of only 1 in 2400. Compared to the significantly costly rwsem, this is > negligible. > On the other hand, holding a write lock without making any VMA > modifications appears questionable and likely incorrect. While this > patch focuses on reducing the lock duration, future updates may aim > to eliminate the write lock entirely. > > Cc: Greg Kroah-Hartman > Cc: "Arve Hjønnevåg" > Cc: Todd Kjos > Cc: Martijn Coenen > Cc: Joel Fernandes > Cc: Christian Brauner > Cc: Carlos Llamas > Cc: Suren Baghdasaryan > Tested-by: Tangquan Zheng > Signed-off-by: Barry Song > --- > drivers/android/binder_alloc.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c > index b3acbc4174fb..f20074e23a7c 100644 > --- a/drivers/android/binder_alloc.c > +++ b/drivers/android/binder_alloc.c > @@ -227,13 +227,23 @@ static int binder_install_single_page(struct binder_alloc *alloc, > if (!mmget_not_zero(alloc->mm)) > return -ESRCH; > > + /* > + * Don't allocate page in mmap_write_lock, this can block > + * mmap_rwsem for a long time; Meanwhile, allocation failure > + * doesn't necessarily need to return -ENOMEM, if lru_page > + * has been installed, we can still return 0(success). > + */ > + page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO); But now you are allocating new pages even if binder_get_installed_page() is an error, right? Doesn't that slow things down? How was this benchmarked? > + > /* > * Protected with mmap_sem in write mode as multiple tasks > * might race to install the same page. > */ > mmap_write_lock(alloc->mm); > - if (binder_get_installed_page(lru_page)) > + if (binder_get_installed_page(lru_page)) { > + ret = 1; That is not a valid error value :( > goto out; > + } > > if (!alloc->vma) { > pr_err("%d: %s failed, no vma\n", alloc->pid, __func__); > @@ -241,7 +251,6 @@ static int binder_install_single_page(struct binder_alloc *alloc, > goto out; > } > > - page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO); > if (!page) { > pr_err("%d: failed to allocate page\n", alloc->pid); > ret = -ENOMEM; > @@ -252,7 +261,6 @@ static int binder_install_single_page(struct binder_alloc *alloc, > if (ret) { > pr_err("%d: %s failed to insert page at offset %lx with %d\n", > alloc->pid, __func__, addr - alloc->buffer, ret); > - __free_page(page); > ret = -ENOMEM; > goto out; > } > @@ -262,7 +270,9 @@ static int binder_install_single_page(struct binder_alloc *alloc, > out: > mmap_write_unlock(alloc->mm); > mmput_async(alloc->mm); > - return ret; > + if (ret && page) > + __free_page(page); > + return ret < 0 ? ret : 0; Please only use ? : for when you have to, otherwise please spell it out with a normal if statement: if (ret < 0) return ret; return 0; But, this is abusing the fact that you set "ret = 1" above, which is going to trip someone up in the future as that is NOT a normal coding pattern we have in the kernel, sorry. If you insist on this change, please rework it to not have that type of "positive means one thing, 0 means another, and negative means yet something else" please. thanks, greg k-h