linux-kernel - Re: [PATCH v2 2/3] mm, page_owner: Add page_owner_stacks file to print out only stacks and their counter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 8 Sep 2022 07:31:29 +0200
From:   Marco Elver <elver@...gle.com>
To:     Oscar Salvador <osalvador@...e.de>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Michal Hocko <mhocko@...e.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Eric Dumazet <edumazet@...gle.com>,
        Waiman Long <longman@...hat.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Andrey Konovalov <andreyknvl@...il.com>,
        Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH v2 2/3] mm, page_owner: Add page_owner_stacks file to
 print out only stacks and their counter

On Thu, 8 Sept 2022 at 05:32, Oscar Salvador <osalvador@...e.de> wrote:
>
> On Wed, Sep 07, 2022 at 09:14:35AM +0200, Marco Elver wrote:
> > Why are you casting a stack_record** to a stack_record*? stack_table
> > is already appropriately typed, and there should be no need to cast
> > things around.
> >
> > 'stacks' is supposed to be the bucket? In which case you need to
> > dereference it to get the first entry in the bucket: bucket =
> > stack_table[table_i];
> >
> > stack_i cannot be used to index into the bucket, because the elements
> > in it are a linked list and not necessarily adjacent in memory. You
> > have to traverse the linked list stack_i elements to get to the start:
>
> Yea, I figured that much after thinking about more, but I was overly
> eager.
>
> >   for (int i = 0; stack && i < stack_i; stack = stack->next, ++i);
>
> But this seems suboptimal.
> With this code, we have to walk the list till we find the right index
> every time we enter the function, while the actual code of v2
> or even the patch from v1 [1], we do not really need to do that
> because we already have the pointer to the stack.
>
> So I much rather prefer that, than having to traverse the stacks
> till the find the right one.

I would not prematurely optimize this. It's a hash map, and the only
problem is if there are tons of collisions. Also, this function isn't
performance critical, it's only used for printing, which itself is
slow.

I suggest you collect some stats how many entries each bucket has on
average. If the average is <10, I'd go with the cleaner interface.