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 0F818C38145 for ; Tue, 6 Sep 2022 08:35:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5F9CB8026B; Tue, 6 Sep 2022 04:35:38 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5A89380224; Tue, 6 Sep 2022 04:35:38 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 449798026B; Tue, 6 Sep 2022 04:35:38 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 3163A80224 for ; Tue, 6 Sep 2022 04:35:38 -0400 (EDT) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id EEB4C80599 for ; Tue, 6 Sep 2022 08:35:37 +0000 (UTC) X-FDA: 79881001914.25.8074688 Received: from mail-yb1-f178.google.com (mail-yb1-f178.google.com [209.85.219.178]) by imf27.hostedemail.com (Postfix) with ESMTP id 7723B400A9 for ; Tue, 6 Sep 2022 08:35:37 +0000 (UTC) Received: by mail-yb1-f178.google.com with SMTP id y82so7591968yby.6 for ; Tue, 06 Sep 2022 01:35:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=YdNM7Ik2NWgFFzggAJTVBKjWtAKl429n8iXk5rBHK48=; b=cStlk6Dfg5rJdWkIE1JyNVlW48yWwGOmCCJyLxIcoW8xFNmnH4HpvqKYHbufn2BCv7 ZA+6/hNDAVW5/pitBUGS2jZ+ueCvhvHUlgYgBp6AQG+M2WzNreCFQmfc/4RUJzfvIrF5 MdYBZFgiQvDO3eou+ZVuT5sH9/gKrIGms+M8l+Oc8eu/4A1+0+iVrSL5D/V9dkKQQsOH TUtxGBytlVOqxzb0Jml3EJCM+rXLCvXufKhMz4cZOsubKT8vuJA6m2Z+XR2Zy2hryp8B xCmgAklrZg39/ia0/nRUsTRJMCJl5JXH8EYkbL3meOmvWrP4LNjjhF4NAxudb/lLFMAS 7gpw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=YdNM7Ik2NWgFFzggAJTVBKjWtAKl429n8iXk5rBHK48=; b=qLBJw0HsrXAJc1Gx2mgXLAjxOrXdFMPLaGGPmtQiq0AA8Sux1ImHMfOdpA/FL9rW8C swTAXAXm+tR4tzy69CeyrR9XXSiL7+c5lkESIWnzV2+lPe5OhGhIR+XUvtbhcWIIbJbU UEaE2gLgAom6BM+bP1UkW0nREY+3WO4X/J7od9rXKh2/Fcko+jAzk2nsT82QgNDTJoVa HYBPyI0ppqpOYc3LqjqECQ577vJvTsn8UfLEOC8sWzDaf5C9P2CIKWksU9hzg5pPCA4Y jkdT+ppDCS7TZGnYnhAxG5cT0uzRNsn/XESZ7UmE3W/LHjQG6TIrr5tJhKp1dj2ZMiwU +a+Q== X-Gm-Message-State: ACgBeo1pTHFu7dCIL9QUcaoiv+3F/2lKVSqNYIpdu1c0uPkirg1Mfipp FQFw5Z9+EPzk8bQnhZyYDWIyq8QTimmQ8iflcU/SyA== X-Google-Smtp-Source: AA6agR4nQtf5fsDxIb/7RKHfhElIpoTm60GSIpKdNdi+EgERf+B1HxnUyQZ3K/I2h7ChibvcPuzzXKWM+rdEDzVX2K8= X-Received: by 2002:a25:9f85:0:b0:693:614:cb2a with SMTP id u5-20020a259f85000000b006930614cb2amr34489719ybq.143.1662453336501; Tue, 06 Sep 2022 01:35:36 -0700 (PDT) MIME-Version: 1.0 References: <20220905031012.4450-1-osalvador@suse.de> <20220905031012.4450-3-osalvador@suse.de> In-Reply-To: From: Marco Elver Date: Tue, 6 Sep 2022 10:35:00 +0200 Message-ID: Subject: Re: [PATCH v2 2/3] mm, page_owner: Add page_owner_stacks file to print out only stacks and their counter To: Oscar Salvador Cc: Andrew Morton , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Michal Hocko , Vlastimil Babka , Eric Dumazet , Waiman Long , Suren Baghdasaryan , Andrey Konovalov , Alexander Potapenko Content-Type: text/plain; charset="UTF-8" ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1662453337; a=rsa-sha256; cv=none; b=QDBcGA0yfdmQ8c3XvQ2kU4RyPoLlZnZZElMx4rzsfS0tmzYLmWd7b5W/lqz0s5iBiOYdOG 0yxDTMt6hTjeyMZiCwBCW2mY3sYDEZ1DjvdExetUbybbSHFYXvoIQgvRLKlBc1TTDeo1RC rQyHAbVn5hTT5qq1kXJt8uzI9ukBAbE= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=cStlk6Df; spf=pass (imf27.hostedemail.com: domain of elver@google.com designates 209.85.219.178 as permitted sender) smtp.mailfrom=elver@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=1662453337; 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=YdNM7Ik2NWgFFzggAJTVBKjWtAKl429n8iXk5rBHK48=; b=vcgT1npKKJ/7A+47cRh+vzkptluKp4ke+qrhjlGwiD5Z/2S8bgjBgdcds3sW640GhFp5F7 Tli2ySgLOFIyvytQ2otbj2tPDy3Gz5/NmiKwjUMqAUtdzDuSj7+zEU1olb+E6cEiRlvTgC Qcu1/Ostsz2kJ01rxS7joVWM6Gx0brQ= X-Rspamd-Queue-Id: 7723B400A9 Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=cStlk6Df; spf=pass (imf27.hostedemail.com: domain of elver@google.com designates 209.85.219.178 as permitted sender) smtp.mailfrom=elver@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspam-User: X-Rspamd-Server: rspam12 X-Stat-Signature: 99mwz8kskbea19grnmajdh9r33yqfe4r X-HE-Tag: 1662453337-284743 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: On Tue, 6 Sept 2022 at 09:44, Oscar Salvador wrote: > > On Mon, Sep 05, 2022 at 02:57:50PM +0200, Marco Elver wrote: > > On Mon, Sep 05, 2022 at 05:10AM +0200, Oscar Salvador wrote: > > [...] > > > +int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos) > > > > Can you add kernel-doc comment what this does (and also update > > accordingly in 3/3 when you add 'threshold'). > > Yes, I guess a kernel-doc comment is due. > > > From what I see it prints *all* stacks that have a non-zero count. > > Correct? > > That's right. > > > If so, should this be called stack_depot_print_all_count() (having > > stack(s) in the name twice doesn't make it more obvious what it does)? > > Then in the follow-up patch you add the 'threshold' arg. > > I guess so. The only reason I went with the actual name is that for me > "stack_depot" was kinda the name of the module/library, and > so I wanted to make crystal clear what were we printing. > > But I'm ok with renaming it if it's already self-explanatory I think it's clear from the fact we're using the stack depot that any printing will print stacks. To mirror the existing 'stack_depot_print()', I'd go with 'stack_depot_print_all_count()'. > > > +{ > > > + int i = *pos, ret = 0; > > > + struct stack_record **stacks, *stack; > > > + static struct stack_record *last = NULL; > > > + unsigned long stack_table_entries = stack_hash_mask + 1; > > > + > > > + /* Continue from the last stack if we have one */ > > > + if (last) { > > > + stack = last->next; > > > > This is dead code? > > No, more below. > > > Either I'm missing something really obvious, but I was able to simplify > > the above function to just this (untested!): > > > > int stack_depot_print_stacks_threshold(char *buf, size_t size, loff_t *pos) > > { > > const unsigned long stack_table_entries = stack_hash_mask + 1; > > > > /* Iterate over all tables for valid stacks. */ > > for (; *pos < stack_table_entries; (*pos)++) { > > for (struct stack_record *stack = stack_table[*pos]; stack; stack = stack->next) { > > if (!stack->size || stack->size < 0 || stack->size > size || > > stack->handle.valid != 1 || refcount_read(&stack->count) < 1) > > continue; > > > > return stack_trace_snprint(buf, size, stack->entries, stack->size, 0) + > > scnprintf(buf + ret, size - ret, "stack count: %d\n\n", > > refcount_read(&stack->count)); > > } > > } > > > > return 0; > > Yes, this will not work. > > You have stack_table[] which is an array for struct stacks, and each struct > stack has a pointer to its next stack which walks from the beginning fo a specific > table till the end. e.g: > > stack_table[0] = {stack1, stack2, stack3, ...} (each linked by ->next) > stack_table[1] = {stack1, stack2, stack3, ...} (each linked by ->next) > .. > stack_table[stack_table_entries - 1] = {stack1, stack2, stack3, ...} (each linked by ->next) > > *pos holds the index of stack_table[], while "last" holds the last stack within > the table we were processing. > > So, when we find a valid stack to print, set "last" to that stack, and *pos to the index > of stack_table. > So, when we call stack_depot_print_stacks_threshold() again, we set "stack" to "last"->next, > and we are ready to keep looking with: > > for (; stack; stack = stack->next) { > ... > check if stack is valid > } > > Should not we find any more valid stacks in that stack_table, we need to check in > the next table, so we do:: > > i++; (note that i was set to *pos at the beginning of the function) > *pos = i; > last = NULL; > goto new_table > > and now are ready to do: > > new_table: > stacks = &stack_table[i]; > stack = (struct stack_record *)stacks; > > > Does this clarify it a little bit? > > About using static vs non-static. > In the v1, I was using a parameter which contained last_stack: > > https://patchwork.kernel.org/project/linux-mm/patch/20220901044249.4624-3-osalvador@suse.de/ > > Not sure if that's better? Thoughts? Moderately better, but still not great. Essentially you need 2 cursors, but with loff_t you only get 1. I think the loff_t parameter can be used to encode both cursors. In the kernel, loff_t is always 'long long', so it'll always be 64-bit. Let's assume that collisions in the hash table are rare, so the number of stacks per bucket are typically small. Then you can encode the index into the bucket in bits 0-31 and the bucket index in bits 32-63. STACK_HASH_ORDER_MAX is 20, so 32 bits is plenty to encode the index.