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 11571CD37B5 for ; Tue, 3 Sep 2024 22:23:43 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8B25E8D01E8; Tue, 3 Sep 2024 18:23:43 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 83B5B8D01E4; Tue, 3 Sep 2024 18:23:43 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6DB348D01E8; Tue, 3 Sep 2024 18:23:43 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 4C49D8D01E4 for ; Tue, 3 Sep 2024 18:23:43 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id D73E6160AC7 for ; Tue, 3 Sep 2024 22:23:42 +0000 (UTC) X-FDA: 82524855084.18.0C66B11 Received: from mail-pl1-f177.google.com (mail-pl1-f177.google.com [209.85.214.177]) by imf23.hostedemail.com (Postfix) with ESMTP id 005C9140007 for ; Tue, 3 Sep 2024 22:23:39 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=bLFg0hyx; spf=pass (imf23.hostedemail.com: domain of cmllamas@google.com designates 209.85.214.177 as permitted sender) smtp.mailfrom=cmllamas@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1725402124; 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=NroSIqwal6C0TPK35/SdmH0JtLPdZfaKT8kFABhfjM0=; b=eeC97Lm/18hHdzz3oNm8oZld/nVHEL1QjX1lxO7SgVDnoAFAwyiE/8FPUiRF2uHf0gkGN2 KNqhw4an6tmnQdbtxU4mGoTPOvnF0OA8/A/kBqFf0Ce3ZDZBFC8n6VjM5LwFYUGAd4kfuH eR3HE2Q7ysonblkDvDoidXnfVAJADps= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1725402124; a=rsa-sha256; cv=none; b=osYDyqnpkbfbJsn2g5UvsvdMLpCYmqfSkpnGXDdD09MxNzmgihf4gHEoNT5wCrnZg3czx8 vL1fvmZqmzn6aTmaoDdRhpLeHJ8e2Q5+cLMFi/puTAz1HpRRyl+HP7PoESKQQOJDaFmMi1 X8lnIB2Sw6rpsW/m0PpCOmMbymMn6Dg= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=bLFg0hyx; spf=pass (imf23.hostedemail.com: domain of cmllamas@google.com designates 209.85.214.177 as permitted sender) smtp.mailfrom=cmllamas@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-pl1-f177.google.com with SMTP id d9443c01a7336-20546b8e754so33425ad.1 for ; Tue, 03 Sep 2024 15:23:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1725402219; x=1726007019; darn=kvack.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=NroSIqwal6C0TPK35/SdmH0JtLPdZfaKT8kFABhfjM0=; b=bLFg0hyxrCvROlsZvac1LllgdPrYnktdGgZF3ZLnivK4I2rBfnE978inLVeIShzq46 8DBboYtWc66V7I1+psSa6WT9T6hkKSimTIdsh/1kjiVdtFs9h4Bwqk2S//Xn9dPup1QP b0cyZKh09lNv2yCcpOj/QGYM3LM7kWibqRzwMg2tJPVcX1C7s1hc0BsMHtf9iwL5fx5W m4muDcYQoM+UAzrI7gh5Lxzrlx9rAmwyeiqRspTdXAtEcHfMQVZEVE+GfaWTFoeAY4u1 AAsoX5sCBAtV6vaYQqlikjCUKq5X9ScxzRysdlCbkUquGbWJrGuILzE0gAE6iPEmefsE Vu8g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725402219; x=1726007019; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=NroSIqwal6C0TPK35/SdmH0JtLPdZfaKT8kFABhfjM0=; b=Xr0Cp3dYSF4BodvfIdY2MAl7dF0wupuagDmGV3/rfGr9Bp/KOJ16ultG9fhQAunEtd PhazMvuAABooUEZ65cqKBaY6eZjD5M0M+FPa3d1QgW+wyKPXRkb8HJltvjCWgoAXDGMW jBT+/Od1IyXYUxFdnPy+xG348HyXPant27E+phzZfDLJeYfZ3dIBe1o6Q7J2L4KPk33F 0ACpi0GQrylb13dDtv7vsc6a9IncWFyM/5Vo/qsg6VLVbs63NyNACzrDq0HyvAtYRkU9 8PRdJIat3vQfSGln/k4vXtfBPvxMZS+925zp6/E5eXfZRaEE9SYdKhWi6o66iyMLCk1Z d1qw== X-Forwarded-Encrypted: i=1; AJvYcCVRZbWqcUmti+ZlFCIdCEALy8zmK4XubKYN6GrGny86wgGqBh15sxN+Xe8/rU6KYH6CqKXGyqJxFA==@kvack.org X-Gm-Message-State: AOJu0YxTGuXlYo+On+S89l+dehtu2xZqqjUFJuamvO5o1okVFbRCxKjP zvf2vS343SdEj/6Kr4UDF2MVtbNjnKAom92AIH9G5sQLoFySxhycNx0sAJtEGA== X-Google-Smtp-Source: AGHT+IGPK+Sty2egGVVQYW1P+X2yQYVnvIv8jHjlO6wDgkw0UaUcxded/EZRq+TQYfvI+DIfmARohw== X-Received: by 2002:a17:902:f546:b0:206:ae39:9f9 with SMTP id d9443c01a7336-206b58bdcf2mr424035ad.21.1725402218366; Tue, 03 Sep 2024 15:23:38 -0700 (PDT) Received: from google.com (55.212.185.35.bc.googleusercontent.com. [35.185.212.55]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2d8d38ffff9sm5300801a91.39.2024.09.03.15.23.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 Sep 2024 15:23:37 -0700 (PDT) Date: Tue, 3 Sep 2024 22:23:32 +0000 From: Carlos Llamas To: Barry Song <21cnbao@gmail.com> Cc: Hillf Danton , Greg Kroah-Hartman , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Barry Song , Suren Baghdasaryan , Michal Hocko , Tangquan Zheng Subject: Re: [PATCH] binder_alloc: Move alloc_page() out of mmap_rwsem to reduce the lock duration Message-ID: References: <20240902225009.34576-1-21cnbao@gmail.com> <20240903110109.1696-1-hdanton@sina.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 005C9140007 X-Stat-Signature: cknfgmnaphtqsee31cdft7mgcfj58imq X-HE-Tag: 1725402219-105145 X-HE-Meta: U2FsdGVkX18KOns5RJzEEH59199LuAezAIW6Spd713KP7n0pXFk5mNdrUfrMwhpQgX+3YQgEfSJeqTyFrWyYtx41Sy86goKD954Z/qWXF3whlIOc1ev47kNHvPHzTawQdgqkmcNJYGbHF6FJmdOVKiX/wBizxreCBn/vAi4Vp0Dkg8nv0IfoQRk0gFaNGe/xm0JeIwfEdFUirvmkJKdisaql4CEGJupSSxnO/38hwX4p+uTpkS63vPZ1MKPC87I4QzSGSc5slvYc2prd2i77AAHrm9rdotKlMNuhTuEMd5kEPJ3YDp+TofaLIOGSEsuuluG/6VCuuZco89PhcFaehSxeHDz/mScLf4NpqcxvPXtPmXRDGH4nCA8ppxDL9SgUG1GKaXatyjZsqsUxmZ3V4BuoQlbUapGppV87OQ5QuOvS79sT7ubu62T2jMkZkXtp0rypgSegmhLhTTwgmQyH3S4mfZx/4N0C2dn3fjhdYoFzMGoAyo0qf/0ZKwda9kFihlr4YdWVYxH1H3KQkS752jIEXOB6bRTOiGH7uBHrYHXq66POPduehbSK3wSJnxlYMOQg2HKs5f5mTSEyUSKePMAF0U8YCt7NHZwLC8WlHwMnOG+fFq6D3rUAMAGv7sJxaEg5gUp5ZAarDQeaGHxAnTIiI+CdW0zGjEQKNh8SQhw07BgF0k2PTqWwcPsonaOx642pvn6CVqeGcQScnG0Z9a9p3h5aaHu19waMOyr5LjqlaPdSejOOes5K0SmrNCn8eD2NCCmuW00tka2N23EyhxwHbAyzMf55ZNciOTWJvwZgywbwsO0RBZjadavKQdykgxQqJVEFE3/EoiazNLZiGUlZ9LQwZFvDVUGzKgx5OAi6ixO/7JIyPH/Bv+md2OzA25layZjo/LtQBGlh3vtQcpm5qIghDBbcdJ2BWPoWnnO/ymGRfPv6mYW/rBJCdm+qtx3L1Y2WWOO3nZLtWJL lDWq9BPz z5XxU7iPRhhHj+hW/gTl33MoUpacOcXysjKfERVjv5o3cnKwG8a9I3yT+GxDKVIfuEXgCxZm2aSM+JQIziULCzFMbnjxfbdhvjk3aoIJXHlKhJiko6dFqOmYa87VOMTnTckYLuFcUrZEb/OTZaSOFk5T3iriY/iUKqB3WWdbRpLXnQD1SzQsXB/YZ2LMD87Oeb79YedxqOy2qsKQHFrmhCLU1RX2MrRAb1ZkwW9wXY5Q/aeRclR/xw1ARc/oo4zsfpXm5Ot0Y+mefzvLJMPQRm/ipNfFOSijiQxEn1iVPJqE61PuE/QF9ixMOmNztjYeQJdRZPGEkyZ1jXlQuzobltVtOY9HTFTLstaTXgwSQHSLRgKK96idLJsquRRuI1jeo14oWuPw4SxpQy6Hvgz5l+lKXD91yuINMChDWFou926+KiN9rGMSIarKgZtKDtO/efWquFUmVI88kHpY= 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 Wed, Sep 04, 2024 at 09:10:20AM +1200, Barry Song wrote: > However, I'm not entirely convinced that this is a problem :-) Concurrent > allocations like this can occur in many places, especially in PFs. Reclamation > is not useless because it helps free up memory for others; it's not > without value. Yeah, for binder though there is a high chance of multiple callers trying to allocate the _same_ page concurrently. What I observed is that pages being reclaimed "in vain" are often in the same vma and this means subsequent callers will need to allocate these pages. But even if this wasn't an issue, the correct solution would still be to support concurrent faults. So, in reality it doesn't matter and we still need to refactor the call to be non-exclusive. > I also don't believe binder is one of the largest users executing concurrent > allocations. Oh, I have no statistics on this. > Awesome! I’m eager to see your patch, and we’re ready to help with testing. > I strongly recommend dropping the write lock entirely. Using > `mmap_write_lock()` isn’t just a binder-specific concern; it has the > potential to affect the entire Android system. > > In patch 3, I experimented with using `per_vma_lock` as well. I’m _not_ > proposing it for merging since you’re already working on it, but I wanted > to share the idea. (just like patch2, it has only passed build-test) Yeap, I'm using the per-vma-lock too per Suren's suggestion. > > [PATCH] binder_alloc: Further move to per_vma_lock from mmap_read_lock > > To further reduce the read lock duration, let's try using per_vma_lock > first. If that fails, we can take the read lock, similar to how page > fault handlers operate. > > 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 a2281dfacbbc..b40a5dd650c8 100644 > --- a/drivers/android/binder_alloc.c > +++ b/drivers/android/binder_alloc.c > @@ -221,6 +221,8 @@ static int binder_install_single_page(struct > binder_alloc *alloc, > struct binder_lru_page *lru_page, > unsigned long addr) > { > + struct vm_area_struct *vma; > + bool per_vma_lock = true; > struct page *page; > int ret = 0; > > @@ -235,10 +237,15 @@ static int binder_install_single_page(struct > binder_alloc *alloc, > */ > page = alloc_page(GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO); > > - mmap_read_lock(alloc->mm); > + vma = lock_vma_under_rcu(alloc->mm, addr); > + if (!vma) { > + per_vma_lock = false; > + mmap_read_lock(alloc->mm); > + vma = find_vma(alloc->mm, addr); > + } > > - /* vma might have been dropped or deattached */ > - if (!alloc->vma || !find_vma(alloc->mm, addr)) { > + /* vma might have been dropped, deattached or changed to new one */ > + if (!alloc->vma || !vma || vma != alloc->vma) { > pr_err("%d: %s failed, no vma\n", alloc->pid, __func__); > ret = -ESRCH; > goto out; > @@ -270,7 +277,10 @@ static int binder_install_single_page(struct > binder_alloc *alloc, > binder_set_installed_page(lru_page, page); > spin_unlock(&alloc->lock); > out: > - mmap_read_unlock(alloc->mm); > + if (per_vma_lock) > + vma_end_read(vma); > + else > + mmap_read_unlock(alloc->mm); > mmput_async(alloc->mm); > if (ret && page) > __free_page(page); > -- > 2.39.3 (Apple Git-146) This looks fairly similar to my patch. However, you would need to handle the case were vm_insert_page() returns -EBUSY (success that raced) and also sync with the shrinker callbacks in binder_alloc_free_page() which is the biggest concern. Let's not duplicate efforts though. Can we please wait for my patch? I'll add you as Co-Developed-by, since you've posted this already? Regards, Carlos Llamas