Message ID | 20231201172212.1813387-24-cmllamas@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp1287542vqy; Fri, 1 Dec 2023 09:25:00 -0800 (PST) X-Google-Smtp-Source: AGHT+IFg8GtLJ7rR2VRLtWCbsvNBvuP3N7MIejmFxCwaxYj7JfVrKR/N7TXpOSrDti10kYBI6hvz X-Received: by 2002:a17:90b:b15:b0:286:5252:e49e with SMTP id bf21-20020a17090b0b1500b002865252e49emr4256423pjb.12.1701451499715; Fri, 01 Dec 2023 09:24:59 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701451499; cv=none; d=google.com; s=arc-20160816; b=S3GbHgOBWBckA78MzyAruECefctEhmVfBTscQj9l1qss6IWW++cCw2rUT1fLU6xwsL WJH6IJj5ckYD7JcU1rHjxduF53er6moOxJkKC1CviJHnAJjpgHqmoPKWHzERE4iwzueZ iy37W55SKhlQjKRKadW1OgZ/sx2VhK6/q7BKyVryu1WvdiLpaMqKWg4kakrisdev57bQ /U93K1+Y9zNwzrCxhCfqddbdC5DMQZMHD0bZtgx57fyfTVuESUzFg+ftioDCNKEODDE3 K0ySzO/+9I3MghR5PptyZFqnMF+FBA67NFuyCnSQbBhvXwya3JWp266TRUMVtjzGPMaV G22g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:dkim-signature; bh=UowIgf2Hkvsym+UG5nPcinbceA5kAaZjExn8YUFB188=; fh=CZn2LCtibpDsl0Zo9a9tnbwjfnRU9o2A0KEGhAadkDo=; b=QcBR7GJkXnLwI8jlBvutVMvpKu+BI3fLCS1df68AM3rZRl3PlV5lV+FAwEPanzS6q5 KQTkACAZ+kuYAR9gpIM8ATYH2cOi0WTzuke5SrVV93tfJXVZyEpX7hJVtCw/WBH6ctMC I32DThQDrda9raodACyDfhCFmto1n9dE/3ltdP5yJB18BtrbLVVarpTg+nLWM1+Udavy CUMeq3Wnk9smQKhrPQt5Fb7fFQeQLLZARlxWieJ4nkSbMfx9cQ2WpJpxyen4+XGE6Cf8 3M8XsUf6Z5EgDSUQV1X6lgowaiL5k6LVgegCCVNn899DEGR0vZkdXbBwpr4I73yKTThr vuEQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=JeJgN5TH; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from agentk.vger.email (agentk.vger.email. [2620:137:e000::3:2]) by mx.google.com with ESMTPS id l22-20020a17090b079600b0028515d4c7f9si3788192pjz.45.2023.12.01.09.24.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 01 Dec 2023 09:24:59 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=JeJgN5TH; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id EA9348115AA3; Fri, 1 Dec 2023 09:24:55 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230036AbjLARYn (ORCPT <rfc822;ruipengqi7@gmail.com> + 99 others); Fri, 1 Dec 2023 12:24:43 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35684 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1379023AbjLARYM (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 1 Dec 2023 12:24:12 -0500 Received: from mail-yb1-xb49.google.com (mail-yb1-xb49.google.com [IPv6:2607:f8b0:4864:20::b49]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5C3771FCA for <linux-kernel@vger.kernel.org>; Fri, 1 Dec 2023 09:23:26 -0800 (PST) Received: by mail-yb1-xb49.google.com with SMTP id 3f1490d57ef6-db4038d7cfdso991283276.2 for <linux-kernel@vger.kernel.org>; Fri, 01 Dec 2023 09:23:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1701451405; x=1702056205; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=UowIgf2Hkvsym+UG5nPcinbceA5kAaZjExn8YUFB188=; b=JeJgN5THSgb9SB3EgUpbUjM/6cX43vwHFvAQM+2kTreoVPNSKs7NccJSYHolJNlb5d yt9a4DkrKkrHftyD2M3i0jY004t/HkldkZ72/wt7Wm/6ULUdIiC/v5/VCR985Q/gK66+ PEmzG7hgHjOr4CxHtNf0JdmFqJ48w8lBGbaEzTmxLJYByumsjUYbaQYuNLU0ze6t4aOp dkrwAqSFwORW4IJdS2RXLUIXoAAgjcCkkHiZs4fRkCMmzqHEdP7teT4pUmGkbHEDQcAB cXiytKUpONJXeJe9z9TIqpQUTomU0Zr43TBY7Cw6ZkvMQqf/BRArlxNiPlhV5wVAOnef nQLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701451405; x=1702056205; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=UowIgf2Hkvsym+UG5nPcinbceA5kAaZjExn8YUFB188=; b=KLir2x/eI1nZ18wRYGN7GA8QUATs2+Ke4ncjX1xiKvcwFYwWFGudv4O5QU6CzlMzAW aYr3OIizSamRRcA9dE/yz5Qmct3pV7orvzpBCf0IQRiZ5EBUw8wc7bdFBX84vOEBydPa uQe8LlBTUrwQEtWM2OORJdf68XUMAKf+4xrctkr7N9gmiN3uEUI0UUB64xRn/i8rR/zy FJmoyMXW2g/e09mPsa0I/4PONr3JG0bi0a0LqG29PninY/3FaP11LLCR560R+wJwinrI Kgvz+HQhLbSx9IceIbPoyKp+RvD8s5XLS5hR0ZITVIB6I+BZX2AjS0yIkUYMKIfdQBE5 0HeQ== X-Gm-Message-State: AOJu0Yyu/dkpJDldX7u40qbwRNnch92In5LlM3POagpbxCn6z2pqdePz OucNrudtg+tUI3xkL4TiwrWoW8yLku4+vQ== X-Received: from xllamas.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5070]) (user=cmllamas job=sendgmr) by 2002:a25:ccc1:0:b0:db5:f536:17d4 with SMTP id l184-20020a25ccc1000000b00db5f53617d4mr77092ybf.11.1701451405614; Fri, 01 Dec 2023 09:23:25 -0800 (PST) Date: Fri, 1 Dec 2023 17:21:52 +0000 In-Reply-To: <20231201172212.1813387-1-cmllamas@google.com> Mime-Version: 1.0 References: <20231201172212.1813387-1-cmllamas@google.com> X-Mailer: git-send-email 2.43.0.rc2.451.g8631bc7472-goog Message-ID: <20231201172212.1813387-24-cmllamas@google.com> Subject: [PATCH v2 23/28] binder: document the final page calculation From: Carlos Llamas <cmllamas@google.com> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, " =?utf-8?q?Arve_Hj?= =?utf-8?q?=C3=B8nnev=C3=A5g?= " <arve@android.com>, Todd Kjos <tkjos@android.com>, Martijn Coenen <maco@android.com>, Joel Fernandes <joel@joelfernandes.org>, Christian Brauner <brauner@kernel.org>, Carlos Llamas <cmllamas@google.com>, Suren Baghdasaryan <surenb@google.com> Cc: linux-kernel@vger.kernel.org, kernel-team@android.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE, USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Fri, 01 Dec 2023 09:24:56 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784101207664551991 X-GMAIL-MSGID: 1784101207664551991 |
Series |
binder: convert alloc->mutex to spinlock
|
|
Commit Message
Carlos Llamas
Dec. 1, 2023, 5:21 p.m. UTC
The code to determine the page range for binder_lru_freelist_del() is
quite obscure. It leverages the buffer_size calculated before doing an
oversized buffer split. This is used to figure out if the last page is
being shared with another active buffer. If so, the page gets trimmed
out of the range as it has been previously removed from the freelist.
This would be equivalent to getting the start page of the next in-use
buffer explicitly. However, the code for this is much larger as we can
see in binder_free_buf_locked() routine. Instead, lets settle on
documenting the tricky step and using better names for now.
I believe an ideal solution would be to count the binder_page->users to
determine when a page should be added or removed from the freelist.
However, this is a much bigger change than what I'm willing to risk at
this time.
Signed-off-by: Carlos Llamas <cmllamas@google.com>
---
drivers/android/binder_alloc.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
Comments
> The code to determine the page range for binder_lru_freelist_del() is > quite obscure. It leverages the buffer_size calculated before doing an > oversized buffer split. This is used to figure out if the last page is > being shared with another active buffer. If so, the page gets trimmed > out of the range as it has been previously removed from the freelist. > > This would be equivalent to getting the start page of the next in-use > buffer explicitly. However, the code for this is much larger as we can > see in binder_free_buf_locked() routine. Instead, lets settle on > documenting the tricky step and using better names for now. > > I believe an ideal solution would be to count the binder_page->users to > determine when a page should be added or removed from the freelist. > However, this is a much bigger change than what I'm willing to risk at > this time. > > Signed-off-by: Carlos Llamas <cmllamas@google.com> Yes, this does help somewhat. However, `curr_last_page` is actually not the last page. It's the last page plus one, since `binder_lru_freelist_del` is exclusive on this argument. Maybe rename it to `curr_after_last_page` or something like that? Or maybe even just `curr_last_page_plus_one`. Alice
On Mon, Dec 04, 2023 at 11:57:27AM +0000, Alice Ryhl wrote: > > The code to determine the page range for binder_lru_freelist_del() is > > quite obscure. It leverages the buffer_size calculated before doing an > > oversized buffer split. This is used to figure out if the last page is > > being shared with another active buffer. If so, the page gets trimmed > > out of the range as it has been previously removed from the freelist. > > > > This would be equivalent to getting the start page of the next in-use > > buffer explicitly. However, the code for this is much larger as we can > > see in binder_free_buf_locked() routine. Instead, lets settle on > > documenting the tricky step and using better names for now. > > > > I believe an ideal solution would be to count the binder_page->users to > > determine when a page should be added or removed from the freelist. > > However, this is a much bigger change than what I'm willing to risk at > > this time. > > > > Signed-off-by: Carlos Llamas <cmllamas@google.com> > > Yes, this does help somewhat. > > However, `curr_last_page` is actually not the last page. It's the last > page plus one, since `binder_lru_freelist_del` is exclusive on this > argument. Maybe rename it to `curr_after_last_page` or something like > that? Or maybe even just `curr_last_page_plus_one`. hmmm, I don't know. I think this could be more confusing, the plus-one is only because of the way that binder_lru_freelist_del() processes the final page. So you could interpret the name both ways. Do we _really_ need the extra comments to make it clear? This solution is too complex anyway, it should really be replaced with a binder_page->nr_users to determine when to add/remove from the lru. -- Carlos Llamas
On Mon, Dec 4, 2023 at 3:39 PM Carlos Llamas <cmllamas@google.com> wrote: > > On Mon, Dec 04, 2023 at 11:57:27AM +0000, Alice Ryhl wrote: > > > The code to determine the page range for binder_lru_freelist_del() is > > > quite obscure. It leverages the buffer_size calculated before doing an > > > oversized buffer split. This is used to figure out if the last page is > > > being shared with another active buffer. If so, the page gets trimmed > > > out of the range as it has been previously removed from the freelist. > > > > > > This would be equivalent to getting the start page of the next in-use > > > buffer explicitly. However, the code for this is much larger as we can > > > see in binder_free_buf_locked() routine. Instead, lets settle on > > > documenting the tricky step and using better names for now. > > > > > > I believe an ideal solution would be to count the binder_page->users to > > > determine when a page should be added or removed from the freelist. > > > However, this is a much bigger change than what I'm willing to risk at > > > this time. > > > > > > Signed-off-by: Carlos Llamas <cmllamas@google.com> > > > > Yes, this does help somewhat. > > > > However, `curr_last_page` is actually not the last page. It's the last > > page plus one, since `binder_lru_freelist_del` is exclusive on this > > argument. Maybe rename it to `curr_after_last_page` or something like > > that? Or maybe even just `curr_last_page_plus_one`. > > hmmm, I don't know. I think this could be more confusing, the plus-one > is only because of the way that binder_lru_freelist_del() processes the > final page. So you could interpret the name both ways. Do we _really_ > need the extra comments to make it clear? > > This solution is too complex anyway, it should really be replaced with a > binder_page->nr_users to determine when to add/remove from the lru. You could also just remove the `next_used_page` part entirely. This means that you will sometimes call `binder_lru_freelist_del` on a page that's in use, but that's just a no-op. Alice
On Mon, Dec 04, 2023 at 03:43:54PM +0100, Alice Ryhl wrote: > You could also just remove the `next_used_page` part entirely. This > means that you will sometimes call `binder_lru_freelist_del` on a page > that's in use, but that's just a no-op. Yes, that is right and I did look into this. The WARN_ON(!on_lru) checks are a bit confusing when it's a no-op as you mention. The purpose though is to assert the "algorithm" of add/del pages from the lru is somewhat correct (particularly the page range). -- Carlos Llamas
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index 052a8c3b0ce1..edd9714ec9f5 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -446,8 +446,8 @@ static struct binder_buffer *binder_alloc_new_buf_locked( struct rb_node *n = alloc->free_buffers.rb_node; struct rb_node *best_fit = NULL; struct binder_buffer *buffer; - unsigned long has_page_addr; - unsigned long end_page_addr; + unsigned long next_used_page; + unsigned long curr_last_page; size_t buffer_size; if (is_async && alloc->free_async_space < size) { @@ -500,12 +500,16 @@ static struct binder_buffer *binder_alloc_new_buf_locked( "%d: binder_alloc_buf size %zd got buffer %pK size %zd\n", alloc->pid, size, buffer, buffer_size); - has_page_addr = (buffer->user_data + buffer_size) & PAGE_MASK; - end_page_addr = PAGE_ALIGN(buffer->user_data + size); - if (end_page_addr > has_page_addr) - end_page_addr = has_page_addr; + /* + * Now we remove the pages from the freelist. A clever calculation + * with buffer_size determines if the last page is shared with an + * adjacent in-use buffer. In such case, the page has been already + * removed from the freelist so we trim our range short. + */ + next_used_page = (buffer->user_data + buffer_size) & PAGE_MASK; + curr_last_page = PAGE_ALIGN(buffer->user_data + size); binder_lru_freelist_del(alloc, PAGE_ALIGN(buffer->user_data), - end_page_addr); + min(next_used_page, curr_last_page)); rb_erase(&buffer->rb_node, &alloc->free_buffers); buffer->free = 0;