Message-ID: <a8ce5c48-3efc-5ea3-6f5c-53b9e33f65c7@virtuozzo.com>
Date:   Thu, 6 Oct 2022 10:34:52 +0300
From:   Alexander Atanasov <alexander.atanasov@...tuozzo.com>
To:     Nadav Amit <nadav.amit@...il.com>
Cc:     "Michael S. Tsirkin" <mst@...hat.com>,
        David Hildenbrand <david@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>, kernel@...nvz.org,
        Linux Virtualization <virtualization@...ts.linux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>, linux-mm@...ck.org
Subject: Re: [PATCH v4 2/7] Enable balloon drivers to report inflated memory

Hello,


On 5.10.22 20:25, Nadav Amit wrote:
> On Oct 5, 2022, at 2:01 AM, Alexander Atanasov <alexander.atanasov@...tuozzo.com> wrote:
> 
>> Add counters to be updated by the balloon drivers.
>> Create balloon notifier to propagate changes.
> 
> I missed the other patches before (including this one). Sorry, but next
> time, please cc me.

You are CCed in the cover letter since the version. I will add CC to you
in the individual patches if you want so.

> 
> I was looking through the series and I did not see actual users of the
> notifier. Usually, it is not great to build an API without users.


You are right. I hope to get some feedback/interest from potential users 
that i mentioned in the cover letter. I will probably split the notifier
in separate series. To make it usefull it will require more changes.
See bellow more about them.


> [snip]
> 
>> +
>> +static int balloon_notify(unsigned long val)
>> +{
>> +	return srcu_notifier_call_chain(&balloon_chain, val, NULL);
> 
> Since you know the inflated_kb value here, why not to use it as an argument
> to the callback? I think casting to (void *) and back is best. But you can
> also provide pointer to the value. Doesn’t it sound better than having
> potentially different notifiers reading different values?

My current idea is to have a struct with current and previous value,
may be change in percents. The actual value does not matter to anyone
but the size of change does. When a user gets notified it can act upon
the change - if it is small it can ignore it , if it is above some 
threshold it can act - if it makes sense for some receiver  is can 
accumulate changes from several notification. Other option/addition is 
to have si_meminfo_current(..) and totalram_pages_current(..) that 
return values adjusted with the balloon values.

Going further - there are few places that calculate something based on 
available memory that do not have sysfs/proc interface for setting 
limits. Most of them work in percents so they can be converted to do 
calculations when they get notification.

The one that have interface for configuration but use memory values can 
be handled in two ways - convert to use percents of what is available or 
extend the notifier to notify userspace which in turn to do calculations 
and update configuration.

> Anyhow, without users (actual notifiers) it’s kind of hard to know how
> reasonable it all is. For instance, is it balloon_notify() supposed to
> prevent further balloon inflating/deflating until the notifier completes?

No, we must avoid that at any cost.

> Accordingly, are callers to balloon_notify() expected to relinquish locks
> before calling balloon_notify() to prevent deadlocks and high latency?

My goal is to avoid any possible impact on performance. Drivers are free 
to delay notifications if they get in the way. (I see that i need to 
move the notification after the semaphore in the vmw driver - i missed 
that - will fix in the next iterration.)
Deadlocks - depends on the users but a few to none will possibly have to 
deal with common locks.


-- 
Regards,
Alexander Atanasov

Powered by blists - more mailing lists