linux-kernel - Re: [PATCH v1 1/2] Enable balloon drivers to report inflated memory

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]

Message-ID: <20220810020330-mutt-send-email-mst@kernel.org>
Date:   Wed, 10 Aug 2022 02:05:09 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Alexander Atanasov <alexander.atanasov@...tuozzo.com>
Cc:     Jonathan Corbet <corbet@....net>,
        Andrew Morton <akpm@...ux-foundation.org>, kernel@...nvz.org,
        David Hildenbrand <david@...hat.com>,
        Wei Liu <wei.liu@...nel.org>, Nadav Amit <namit@...are.com>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v1 1/2] Enable balloon drivers to report inflated memory

On Wed, Aug 10, 2022 at 08:54:52AM +0300, Alexander Atanasov wrote:
> On 9.08.22 13:32, Michael S. Tsirkin wrote:
> > On Tue, Aug 09, 2022 at 12:49:32PM +0300, Alexander Atanasov wrote:
> > > @@ -153,6 +156,14 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
> > >   		    global_zone_page_state(NR_FREE_CMA_PAGES));
> > >   #endif
> > > +#ifdef CONFIG_MEMORY_BALLOON
> > > +	inflated_kb = atomic_long_read(&mem_balloon_inflated_kb);
> > > +	if (inflated_kb >= 0)
> > > +		seq_printf(m,  "Inflated(total): %8ld kB\n", inflated_kb);
> > > +	else
> > > +		seq_printf(m,  "Inflated(free): %8ld kB\n", -inflated_kb);
> > > +#endif
> > > +
> > >   	hugetlb_report_meminfo(m);
> > >   	arch_report_meminfo(m);
> > 
> > 
> > This seems too baroque for my taste.
> > Why not just have two counters for the two pruposes?
> 
> I agree it is not good but it reflects the current situation.
> Dirvers account in only one way - either used or total - which i don't like.
> So to save space and to avoid the possibility that some driver starts to use
> both at the same time. I suggest to be only one value.

I don't see what would be wrong if some driver used both
at some point.

> 
> > And is there any value in having this atomic?
> > We want a consistent value but just READ_ONCE seems sufficient ...
> 
> I do not see this as only a value that is going to be displayed.
> I tried to be defensive here and to avoid premature optimization.
> One possible scenario is OOM killer(using the value) vs balloon deflate on
> oom will need it. But any other user of that value will likely need it
> atomic too. Drivers use spin_locks for calculations they might find a way to
> reduce the spin lock usage and use the atomic.
> While making it a long could only bring bugs without benefits.
> It is not on a fast path too so i prefer to be safe.

Well we do not normally spread atomics around just because we
can, it does not magically make the code safe.
If this needs atomics we need to document why.

> -- 
> Regards,
> Alexander Atanasov