[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220810051331-mutt-send-email-mst@kernel.org>
Date: Wed, 10 Aug 2022 05:16:30 -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 10:50:10AM +0300, Alexander Atanasov wrote:
> Hello,
>
> On 10.08.22 9:05, Michael S. Tsirkin wrote:
> > 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.
>
> If you don't see what's wrong with using both, i might as well add
> Cached and Buffers - next hypervisor might want to use them or any other by
> its discretion leaving the fun to figure it out to the userspace?
Assuming you document what these mean, sure.
> Single definitive value is much better and clear from user prespective and
> meminfo is exactly for the users.
Not really, the negative value trick is anything but clear.
> If a driver for some wierd reason needs to do both it is a whole new topic
> that i don't like to go into. Good news is that currently no such driver
> exists.
>
>
> >
> > >
> > > > 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.
>
> Of course it does not. In one of your comments to my other patches you said
> you do not like patches that add one line then remove it in the next patch.
> To avoid that i put an atomic - if at one point it is clear it is not
> required i would be happy to change it but it is more likely to be need than
> not. So i will probably have to document it instead.
>
> At this point the decision if it should be or should not be in the meminfo
> is more important - if general opinion is positive i will address the
> technical details.
Not up to me, you need ack from linux-mm guys for that.
> --
> Regards,
> Alexander Atanasov
Powered by blists - more mailing lists