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 58711C38145 for ; Tue, 6 Sep 2022 07:44:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D5EEA80260; Tue, 6 Sep 2022 03:44:02 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D0E4B80224; Tue, 6 Sep 2022 03:44:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BD67280260; Tue, 6 Sep 2022 03:44:02 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id AF63980224 for ; Tue, 6 Sep 2022 03:44:02 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 87762C0571 for ; Tue, 6 Sep 2022 07:44:02 +0000 (UTC) X-FDA: 79880871924.04.F35EE5B Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by imf30.hostedemail.com (Postfix) with ESMTP id 2D5A18008A for ; Tue, 6 Sep 2022 07:44:01 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 05F4B1F96B; Tue, 6 Sep 2022 07:44:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1662450241; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=anGEhIPunbdYprwHmKAn9l8RRjOyN25i2V5av6YR7Xk=; b=HpR3wikdZt2jGNZYpD7P8n188HtnxIQ6TRZGkLwPLcy4aRP9zMkGrVxuRFQklIc4Pgu1I7 iP4R8/tkwpr3JsW+zXqoXtpoZ1O2Ov7iQU+XN8b5yARAJWDVhqrDHm6PGjNzK/yVgBekl4 6qd/04ZlAoQFVXURP+hNcow+jTn/ZtY= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1662450241; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=anGEhIPunbdYprwHmKAn9l8RRjOyN25i2V5av6YR7Xk=; b=rXK6GYawgOTHs7c2VvOWVRStaOErHAKZFhNHdWE/Wol1AumJi+1yZlAyd887Q7Ifp5FF4O 882FLtEJ5mYkfmCQ== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 66CA113A7A; Tue, 6 Sep 2022 07:44:00 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id Jyd5FkD6FmOaPAAAMHmgww (envelope-from ); Tue, 06 Sep 2022 07:44:00 +0000 Date: Tue, 6 Sep 2022 09:43:58 +0200 From: Oscar Salvador To: Marco Elver 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 Subject: Re: [PATCH v2 2/3] mm, page_owner: Add page_owner_stacks file to print out only stacks and their counter Message-ID: References: <20220905031012.4450-1-osalvador@suse.de> <20220905031012.4450-3-osalvador@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1662450242; a=rsa-sha256; cv=none; b=XT3LOOb9ls0BA7yXxjYZQAGV95nrIqK7cKmg/EXX4Yg39KjwsNVI6zblwr6sb8F8+g/OUi BSR8o96yQB2637GkxmTKg6Vh325h+/MtYsIMvtWoxLYJ0VC2bycT/pVgRt7xDvSDKPw2OU /vkoTpeUo7ZXLlu/cB7FiKHTTW9/Tgw= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=suse.de header.s=susede2_rsa header.b=HpR3wikd; dkim=pass header.d=suse.de header.s=susede2_ed25519 header.b=rXK6GYaw; dmarc=pass (policy=none) header.from=suse.de; spf=pass (imf30.hostedemail.com: domain of osalvador@suse.de designates 195.135.220.29 as permitted sender) smtp.mailfrom=osalvador@suse.de ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1662450242; 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=anGEhIPunbdYprwHmKAn9l8RRjOyN25i2V5av6YR7Xk=; b=pXQlBAvjYOJkmnirGEFR+CtQPjSX4RYFzqMtC1SltWwMVkcfikpXqvXaaHPnamdu2vQiyv iEfp/FE8JqZXp53Q93KM4wIfJFamdLzqUxfjcCCGkA5JweJy9RV/svCe+tNzp0g+O53OMH oW+bHk3NmGWRHJknAijf2ak9hm05Fcc= X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: hhiana34nx5j5tw5dbeexq6qhs46hokr Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=suse.de header.s=susede2_rsa header.b=HpR3wikd; dkim=pass header.d=suse.de header.s=susede2_ed25519 header.b=rXK6GYaw; dmarc=pass (policy=none) header.from=suse.de; spf=pass (imf30.hostedemail.com: domain of osalvador@suse.de designates 195.135.220.29 as permitted sender) smtp.mailfrom=osalvador@suse.de X-Rspamd-Queue-Id: 2D5A18008A X-HE-Tag: 1662450241-341664 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 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 > > +{ > > + 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? -- Oscar Salvador SUSE Labs