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 21D09C47073 for ; Thu, 4 Jan 2024 09:26:22 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 73C9C6B03A6; Thu, 4 Jan 2024 04:26:21 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 6EDAA6B03A8; Thu, 4 Jan 2024 04:26:21 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5B39E6B03A9; Thu, 4 Jan 2024 04:26:21 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 494206B03A6 for ; Thu, 4 Jan 2024 04:26:21 -0500 (EST) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 1C3EE1A0955 for ; Thu, 4 Jan 2024 09:26:21 +0000 (UTC) X-FDA: 81641097762.15.9A77FDE Received: from mail-vk1-f175.google.com (mail-vk1-f175.google.com [209.85.221.175]) by imf14.hostedemail.com (Postfix) with ESMTP id 50444100008 for ; Thu, 4 Jan 2024 09:26:19 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=qsgdcAoR; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf14.hostedemail.com: domain of elver@google.com designates 209.85.221.175 as permitted sender) smtp.mailfrom=elver@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1704360379; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=JGeZ8w8hYuhNSoX/scf5I633EqoYMFGVRc1Mgn4PFeU=; b=iA0pEWhEZuatFCxXETpROZK9GE8lrElacMCPKzoZy44xE/n7i2EvZ6OnhVz3/6HFd07yJo hoR7IpxIMoNI5cKvOpYX4KzHZO3OUkRl/T0AvJKeyIMMpvUdarXabY/MNy53aEZmrLaf5Y 7TJBiQ/QYw7uY4qLOgXDF9ECx2TRBmA= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=qsgdcAoR; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf14.hostedemail.com: domain of elver@google.com designates 209.85.221.175 as permitted sender) smtp.mailfrom=elver@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1704360379; a=rsa-sha256; cv=none; b=NiMZ2CfH3+iO+enfLmPxz/PUC4xNy1CF6RjbR5rKMBQ4itn2fuygvr983elggXdYc8Vqte uWdvQw3dUa7kKM7leAP7XoKqMEVx2QM/Qv3nMJd/D23fZqB5L+nibzEUBTA+bz0Iund19+ wBVpwDYnQAILoJhRg6pAjBr+GLtbZu4= Received: by mail-vk1-f175.google.com with SMTP id 71dfb90a1353d-4b731a13d76so96890e0c.1 for ; Thu, 04 Jan 2024 01:26:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1704360378; x=1704965178; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=JGeZ8w8hYuhNSoX/scf5I633EqoYMFGVRc1Mgn4PFeU=; b=qsgdcAoRQtAN3txv4X0V3IHHDgQ/R1VlGEBhgQ6fVbiVQoMIaLsQKDFzpVUy3XND3K vwg7V334XXkmvrTuoUs1Il4L3AprCERUfjm105NtdgAwp4Of0gT7ijefVy4J7NYKox6N Cs9lcL7vbKtH9yI5YibiyYtwdcFYNysnTc6IOkUP3hig8TjMX5SkYEU4vozKN9EhaizI uYuD9zlaE2dH0oTHnLJpdJAOCVs6E1ZoMMA3KXL7+g5+wAusv99K0ZPLZtSb0vHjj/nl PVhTkrHSfSf34D+t/sIMM9k1r8f+7/e7prCFH3ZxBmUFY7arorzQznUpvprAw8pokzD1 53Pg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704360378; x=1704965178; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=JGeZ8w8hYuhNSoX/scf5I633EqoYMFGVRc1Mgn4PFeU=; b=oQfYzpAfqloJx3kt81j70uJfDr0ZVU0uQQRyfQiISI1KHGgNNRT3vqfImVEIum+2vu RRjAhm+HwOc6QxHd+SRJb2abfdCv+qrRH8WWvWSQ4IpWrW2Xb6JQ96IRNFEjSuH/bNpW 0uMATJhEA+eBFcQ62AS/rsXYAFZJ5h8KXLgIIs4I2Z0O2k01T+N2OE/XpzDc1Go9UR6Y zcYvwlko2vLIUtrjcXiuA862Guqzzbemf7K5NuTDS8NI3x1IHZ6Ez+Di/uYGsT2IVWBI T1lEgzouiLtOiTRW1CwADIQ8aCdCBiuI9Xgfggu8ZzDTqpgWO6r060Pmwb+DVt/ZBgZ6 X+WA== X-Gm-Message-State: AOJu0YwwLVM9ekvZqFrbSbpZXLs+BOAUmKNdQCCWr2qY9wJPmyDQDudy SBSdk8aPvMCU95YBjimE3rG1w9kZF9gzhH56q2uAU5P9jV9d X-Google-Smtp-Source: AGHT+IGpmpV004FLzbQaPdXQqyI6lDoA20N0jgFhbKP/sfmLQ9yB4AyspNgLN3JiX7lkWl+zSjEVsZ8iJz2R9kD/zfk= X-Received: by 2002:a05:6122:45aa:b0:4b6:d5f1:7c8a with SMTP id de42-20020a05612245aa00b004b6d5f17c8amr245659vkb.4.1704360378266; Thu, 04 Jan 2024 01:26:18 -0800 (PST) MIME-Version: 1.0 References: <1d1ad5692ee43d4fc2b3fd9d221331d30b36123f.1700502145.git.andreyknvl@google.com> In-Reply-To: From: Marco Elver Date: Thu, 4 Jan 2024 10:25:40 +0100 Message-ID: Subject: Re: [PATCH v4 17/22] lib/stackdepot: allow users to evict stack traces To: Oscar Salvador Cc: andrey.konovalov@linux.dev, Andrew Morton , Andrey Konovalov , Alexander Potapenko , Dmitry Vyukov , Vlastimil Babka , kasan-dev@googlegroups.com, Evgenii Stepanov , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrey Konovalov Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 50444100008 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: ny3t1oxiw8seaontp7u6ndwimbfbyo33 X-HE-Tag: 1704360379-760148 X-HE-Meta: U2FsdGVkX19vFcyUX8z0C2St6IbHXJknUozLURriWT0SzkEb+RYT5uwZ6G4xld7vAB5oiHf+c4MFQecpo9UC7P0sdYjW9aB2zCe4i9+2OdWHXnmPWKE0tvNv+s8niK8BSufrpsyCNfW74UOxxupi7UkzpQR7SDyKmEPIpgthvecNctsNN5wr6MFG/2r47OjywsxaFPxswYrwe1j7hnbc8O319RisOm0UyAC/GJZQ2VjWCQXXu2Iy6izsQkR8vR0vKsfW5twVMQJoa/dW0xppHphILg2+R62qUI1TzvTGtAnOKwoippbB1b1oYffVxcuEPryk6/pkO6i1YnbRZvLu5au+PabWBSyM4MJ0ja9+zmgzF4ztIb0YRNkJg1EEFxkBzSTrztIZ6jlRJNJy3BgPSgWkW6BhmLY+cmG/JNZEe8WpEdwd59t0vpPQ+zdqhLcU/3xD/WZQxK3F8Tc1cYHvU7LCEk8WFSZ80YnMJGEUkDQXL+AkQ2Xm4HC+UMom3JVpLqL+k3H3MzFVaB7JBFnj/yKQ5766hPWS/BKMKuHMiDbPIzkkvfCM/ZHMrBEHCOGxfMECrA+bGcPjlD+c/OresRrDMO4CHYbp0EF/do473Iu4Vxcr7dnhZSogni5lu1A/7EzE80IuzqjMpTiEox4EeXz/7zmtH+EQMicbHngyDmHOFsWxMfAbj8DXsCNiq14vSPR9U8JRNRyx0OGML8MWnAI4lw7wamUR0UbzypZKVRZIWOOAIaTPygsrUS4asSiUHLp8wvtUTlauRlLyW+F3AX6JAZnWBw0xVq9CCBSNbLVt1UZdUsQt5aAHa4JoIDIHVnD2joP6KTsc3LFTVQF9yIMNi1nZYXUwKJ+EcwSm7dYiwR9UYRHb46aCu3Plja5cb5BGN3hEdNOjm/b714eiH2Fko61iFbblt0toYnhnCwEvgJ61Xj1TJVZ+PuYWSQYpT3JtBFBQavybU4B61SV e70NPzRI 7YUrgI6zfmRlwCoBVvzvi1QnD9AApnuuhait82UJgjslrzN/KMJTJBkKNA6iEU08miVcnKaRCfnytQIprMDpGAO3zEqWBxSYOP2T2ZKDNLWO5xgZLhqDwRXtIIMFd0psAV8OnVWzFNoVHe95Nx/ioQl3d73QPUzInUPll2Ea6DJDQ3kk+IhpccBkY/u2mfifPGDkQs7bs5u6T0XhEfZU08SqzwvrZTxxseiwz3xnC4c3MRd0BAzKMeFQd8GFTp500O0uoovfv5fdsJqJ6L3m2oFFRFl2P2OXsL4bMOEC9GXBMQD26DvhRXqvEpw== 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 Thu, 4 Jan 2024 at 09:52, Oscar Salvador wrote: > > On Mon, Nov 20, 2023 at 06:47:15PM +0100, andrey.konovalov@linux.dev wrote: > > From: Andrey Konovalov > > > > Add stack_depot_put, a function that decrements the reference counter > > on a stack record and removes it from the stack depot once the counter > > reaches 0. > > > > Internally, when removing a stack record, the function unlinks it from > > the hash table bucket and returns to the freelist. > > > > With this change, the users of stack depot can call stack_depot_put > > when keeping a stack trace in the stack depot is not needed anymore. > > This allows avoiding polluting the stack depot with irrelevant stack > > traces and thus have more space to store the relevant ones before the > > stack depot reaches its capacity. > > > > Signed-off-by: Andrey Konovalov > > I yet have to review the final bits of this series, but I'd like to > comment on something below > > > > +void stack_depot_put(depot_stack_handle_t handle) > > +{ > > + struct stack_record *stack; > > + unsigned long flags; > > + > > + if (!handle || stack_depot_disabled) > > + return; > > + > > + write_lock_irqsave(&pool_rwlock, flags); > > + > > + stack = depot_fetch_stack(handle); > > + if (WARN_ON(!stack)) > > + goto out; > > + > > + if (refcount_dec_and_test(&stack->count)) { > > + /* Unlink stack from the hash table. */ > > + list_del(&stack->list); > > + > > + /* Free stack. */ > > + depot_free_stack(stack); > > It would be great if stack_depot_put would also accept a boolean, > which would determine whether we want to erase the stack or not. I think a boolean makes the interface more confusing for everyone else. At that point stack_depot_put merely decrements the refcount and becomes a wrapper around refcount_dec, right? I think you want to expose the stack_record struct anyway for your series, so why not simply avoid calling stack_depot_put and decrement the refcount with your own helper (there needs to be a new stackdepot function to return a stack_record under the pool_rwlock held as reader). Also, you need to ensure noone else calls stack_depot_put on the stack traces you want to keep. If there is a risk someone else may call stack_depot_put on them, it obviously won't work (I think the only option then is to introduce a way to pin stacks). > For the feature I'm working on page_ower [1], I need to keep track > of how many times we allocated/freed from a certain path, which may > expose a potential leak, and I was using the refcount to do that, > but I don't want the record to be erased, because this new > functionality won't be exclusive with the existing one. > > e.g: you can check /sys/kernel/debug/page_owner AND > /sys/kernel/debug/page_owner_stacks > > So, while the new functionaliy won't care if a record has been erased, > the old one will, so information will be lost. > > [1] https://patchwork.kernel.org/project/linux-mm/cover/20231120084300.4368-1-osalvador@suse.de/ > > > > -- > Oscar Salvador > SUSE Labs