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
