Linux Virtualization - Mar 2020 - [PATCH v3] virtio-balloon: Switch back to OOM handler for VIRTIO_BALLOON_F_DEFLATE_ON_OOM

On 10.03.20 20:05, David Rientjes wrote:
> On Tue, 10 Mar 2020, David Hildenbrand wrote:
> 
>> Commit 71994620bb25 ("virtio_balloon: replace oom notifier with
shrinker")
>> changed the behavior when deflation happens automatically. Instead of
>> deflating when called by the OOM handler, the shrinker is used.
>>
>> However, the balloon is not simply some other slab cache that should be
>> shrunk when under memory pressure. The shrinker does not have a concept
of
>> priorities yet, so this behavior cannot be configured. Eventually once
>> that is in place, we might want to switch back after doing proper
>> testing.
>>
>> There was a report that this results in undesired side effects when
>> inflating the balloon to shrink the page cache. [1]
>> 	"When inflating the balloon against page cache (i.e. no free
memory
>> 	 remains) vmscan.c will both shrink page cache, but also invoke the
>> 	 shrinkers -- including the balloon's shrinker. So the balloon
>> 	 driver allocates memory which requires reclaim, vmscan gets this
>> 	 memory by shrinking the balloon, and then the driver adds the
>> 	 memory back to the balloon. Basically a busy no-op."
>>
>> The name "deflate on OOM" makes it pretty clear when
deflation should
>> happen - after other approaches to reclaim memory failed, not while
>> reclaiming. This allows to minimize the footprint of a guest - memory
>> will only be taken out of the balloon when really needed.
>>
>> Keep using the shrinker for VIRTIO_BALLOON_F_FREE_PAGE_HINT, because
>> this has no such side effects. Always register the shrinker with
>> VIRTIO_BALLOON_F_FREE_PAGE_HINT now. We are always allowed to reuse
free
>> pages that are still to be processed by the guest. The hypervisor takes
>> care of identifying and resolving possible races between processing a
>> hinting request and the guest reusing a page.
>>
>> In contrast to pre commit 71994620bb25 ("virtio_balloon: replace
oom
>> notifier with shrinker"), don't add a moodule parameter to
configure the
>> number of pages to deflate on OOM. Can be re-added if really needed.
>> Also, pay attention that leak_balloon() returns the number of 4k pages
-
>> convert it properly in virtio_balloon_oom_notify().
>>
>> Testing done by Tyler for future reference:
>>   Test setup: VM with 16 CPU, 64GB RAM. Running Debian 10. We have a 42
>>   GB file full of random bytes that we continually cat to /dev/null.
>>   This fills the page cache as the file is read. Meanwhile we trigger
>>   the balloon to inflate, with a target size of 53 GB. This setup
causes
>>   the balloon inflation to pressure the page cache as the page cache is
>>   also trying to grow. Afterwards we shrink the balloon back to zero
(so
>>   total deflate = total inflate).
>>
>>   Without patch (kernel 4.19.0-5):
>>   Inflation never reaches the target until we stop the "cat file
>
>>   /dev/null" process. Total inflation time was 542 seconds. The
longest
>>   period that made no net forward progress was 315 seconds (see
attached
>>   graph).
>>   Result of "grep balloon /proc/vmstat" after the test:
>>   balloon_inflate 154828377
>>   balloon_deflate 154828377
>>
>>   With patch (kernel 5.6.0-rc4+):
>>   Total inflation duration was 63 seconds. No deflate-queue activity
>>   occurs when pressuring the page-cache.
>>   Result of "grep balloon /proc/vmstat" after the test:
>>   balloon_inflate 12968539
>>   balloon_deflate 12968539
>>
>>   Conclusion: This patch fixes the issue. In the test it reduced
>>   inflate/deflate activity by 12x, and reduced inflation time by 8.6x.
>>   But more importantly, if we hadn't killed the "grep balloon
>>   /proc/vmstat" process then, without the patch, the inflation
process
>>   would never reach the target.
>>
>> [1] https://www.spinics.net/lists/linux-virtualization/msg40863.html
>>
>> Reported-by: Tyler Sanderson <tysand at google.com>
>> Tested-by: Tyler Sanderson <tysand at google.com>
>> Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier with
shrinker")
>> Cc: Michael S. Tsirkin <mst at redhat.com>
>> Cc: Wei Wang <wei.w.wang at intel.com>
>> Cc: Alexander Duyck <alexander.h.duyck at linux.intel.com>
>> Cc: David Rientjes <rientjes at google.com>
>> Cc: Nadav Amit <namit at vmware.com>
>> Cc: Michal Hocko <mhocko at kernel.org>
>> Cc: Andrew Morton <akpm at linux-foundation.org>
>> Signed-off-by: David Hildenbrand <david at redhat.com>
> 
> Acked-by: David Rientjes <rientjes at google.com>
> 
Thanks!
> Should this have:
> 
> Cc: stable at vger.kernel.org # 4.19+
I guess as nothing will actually "crash" it's not worth stable.

-- 
Thanks,

David / dhildenb

On Tue, Mar 10, 2020 at 08:13:19PM +0100, David Hildenbrand
wrote:
> > Should this have:
> > 
> > Cc: stable at vger.kernel.org # 4.19+
> 
> I guess as nothing will actually "crash" it's not worth
stable.

No - it's a regression, it would be a stable candidate from that POV.
The issue is the patch size - stable rules say:

 - It cannot be bigger than 100 lines, with context.

This is about 200 lines with context.
In practice Fixes: is enough to make a bunch of downstreams pick it up.

-- 
MST

> Am 10.03.2020 um 22:25 schrieb Michael S. Tsirkin <mst at
redhat.com>:
> 
> ?On Tue, Mar 10, 2020 at 08:13:19PM +0100, David Hildenbrand wrote:
>>> Should this have:
>>> 
>>> Cc: stable at vger.kernel.org # 4.19+
>> 
>> I guess as nothing will actually "crash" it's not worth
stable.
> 
> 
> No - it's a regression, it would be a stable candidate from that POV.
AFAIK

? It must fix a problem that causes a build error (but not for things marked
CONFIG_BROKEN), an oops, a hang, data corruption, a real security issue, or some
?oh, that?s not good? issue. In short, something critical.?

If this regression is that critical is debatable. But it doesn?t matter as you
correctly say, it?s too big :)

On Tue, Mar 10, 2020 at 08:13:19PM +0100, David Hildenbrand
wrote:
> On 10.03.20 20:05, David Rientjes wrote:
> > On Tue, 10 Mar 2020, David Hildenbrand wrote:
> > 
> >> Commit 71994620bb25 ("virtio_balloon: replace oom notifier
with shrinker")
> >> changed the behavior when deflation happens automatically. Instead
of
> >> deflating when called by the OOM handler, the shrinker is used.
> >>
> >> However, the balloon is not simply some other slab cache that
should be
> >> shrunk when under memory pressure. The shrinker does not have a
concept of
> >> priorities yet, so this behavior cannot be configured. Eventually
once
> >> that is in place, we might want to switch back after doing proper
> >> testing.
> >>
> >> There was a report that this results in undesired side effects
when
> >> inflating the balloon to shrink the page cache. [1]
> >> 	"When inflating the balloon against page cache (i.e. no free
memory
> >> 	 remains) vmscan.c will both shrink page cache, but also invoke
the
> >> 	 shrinkers -- including the balloon's shrinker. So the
balloon
> >> 	 driver allocates memory which requires reclaim, vmscan gets this
> >> 	 memory by shrinking the balloon, and then the driver adds the
> >> 	 memory back to the balloon. Basically a busy no-op."
> >>
> >> The name "deflate on OOM" makes it pretty clear when
deflation should
> >> happen - after other approaches to reclaim memory failed, not
while
> >> reclaiming. This allows to minimize the footprint of a guest -
memory
> >> will only be taken out of the balloon when really needed.
> >>
> >> Keep using the shrinker for VIRTIO_BALLOON_F_FREE_PAGE_HINT,
because
> >> this has no such side effects. Always register the shrinker with
> >> VIRTIO_BALLOON_F_FREE_PAGE_HINT now. We are always allowed to
reuse free
> >> pages that are still to be processed by the guest. The hypervisor
takes
> >> care of identifying and resolving possible races between
processing a
> >> hinting request and the guest reusing a page.
> >>
> >> In contrast to pre commit 71994620bb25 ("virtio_balloon:
replace oom
> >> notifier with shrinker"), don't add a moodule parameter
to configure the
> >> number of pages to deflate on OOM. Can be re-added if really
needed.
> >> Also, pay attention that leak_balloon() returns the number of 4k
pages -
> >> convert it properly in virtio_balloon_oom_notify().
> >>
> >> Testing done by Tyler for future reference:
> >>   Test setup: VM with 16 CPU, 64GB RAM. Running Debian 10. We have
a 42
> >>   GB file full of random bytes that we continually cat to
/dev/null.
> >>   This fills the page cache as the file is read. Meanwhile we
trigger
> >>   the balloon to inflate, with a target size of 53 GB. This setup
causes
> >>   the balloon inflation to pressure the page cache as the page
cache is
> >>   also trying to grow. Afterwards we shrink the balloon back to
zero (so
> >>   total deflate = total inflate).
> >>
> >>   Without patch (kernel 4.19.0-5):
> >>   Inflation never reaches the target until we stop the "cat
file >
> >>   /dev/null" process. Total inflation time was 542 seconds.
The longest
> >>   period that made no net forward progress was 315 seconds (see
attached
> >>   graph).
> >>   Result of "grep balloon /proc/vmstat" after the test:
> >>   balloon_inflate 154828377
> >>   balloon_deflate 154828377
> >>
> >>   With patch (kernel 5.6.0-rc4+):
> >>   Total inflation duration was 63 seconds. No deflate-queue
activity
> >>   occurs when pressuring the page-cache.
> >>   Result of "grep balloon /proc/vmstat" after the test:
> >>   balloon_inflate 12968539
> >>   balloon_deflate 12968539
> >>
> >>   Conclusion: This patch fixes the issue. In the test it reduced
> >>   inflate/deflate activity by 12x, and reduced inflation time by
8.6x.
> >>   But more importantly, if we hadn't killed the "grep
balloon
> >>   /proc/vmstat" process then, without the patch, the
inflation process
> >>   would never reach the target.
> >>
> >> [1]
https://www.spinics.net/lists/linux-virtualization/msg40863.html
> >>
> >> Reported-by: Tyler Sanderson <tysand at google.com>
> >> Tested-by: Tyler Sanderson <tysand at google.com>
> >> Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier
with shrinker")
> >> Cc: Michael S. Tsirkin <mst at redhat.com>
> >> Cc: Wei Wang <wei.w.wang at intel.com>
> >> Cc: Alexander Duyck <alexander.h.duyck at linux.intel.com>
> >> Cc: David Rientjes <rientjes at google.com>
> >> Cc: Nadav Amit <namit at vmware.com>
> >> Cc: Michal Hocko <mhocko at kernel.org>
> >> Cc: Andrew Morton <akpm at linux-foundation.org>
> >> Signed-off-by: David Hildenbrand <david at redhat.com>
> > 
> > Acked-by: David Rientjes <rientjes at google.com>
> > 
> 
> Thanks!
David could you repost with corrected commit log
so Andrew can merge? To make sure it's not missed,
it's probably a good idea to have a cover letter
mentioning mm in the subject.
> > Should this have:
> > 
> > Cc: stable at vger.kernel.org # 4.19+
> 
> I guess as nothing will actually "crash" it's not worth
stable.
> 
> -- 
> Thanks,
> 
> David / dhildenb

On 11.03.20 12:12, Michael S. Tsirkin wrote:
> On Tue, Mar 10, 2020 at 08:13:19PM +0100, David Hildenbrand wrote:
>> On 10.03.20 20:05, David Rientjes wrote:
>>> On Tue, 10 Mar 2020, David Hildenbrand wrote:
>>>
>>>> Commit 71994620bb25 ("virtio_balloon: replace oom notifier
with shrinker")
>>>> changed the behavior when deflation happens automatically.
Instead of
>>>> deflating when called by the OOM handler, the shrinker is used.
>>>>
>>>> However, the balloon is not simply some other slab cache that
should be
>>>> shrunk when under memory pressure. The shrinker does not have a
concept of
>>>> priorities yet, so this behavior cannot be configured.
Eventually once
>>>> that is in place, we might want to switch back after doing
proper
>>>> testing.
>>>>
>>>> There was a report that this results in undesired side effects
when
>>>> inflating the balloon to shrink the page cache. [1]
>>>> 	"When inflating the balloon against page cache (i.e. no
free memory
>>>> 	 remains) vmscan.c will both shrink page cache, but also
invoke the
>>>> 	 shrinkers -- including the balloon's shrinker. So the
balloon
>>>> 	 driver allocates memory which requires reclaim, vmscan gets
this
>>>> 	 memory by shrinking the balloon, and then the driver adds the
>>>> 	 memory back to the balloon. Basically a busy no-op."
>>>>
>>>> The name "deflate on OOM" makes it pretty clear when
deflation should
>>>> happen - after other approaches to reclaim memory failed, not
while
>>>> reclaiming. This allows to minimize the footprint of a guest -
memory
>>>> will only be taken out of the balloon when really needed.
>>>>
>>>> Keep using the shrinker for VIRTIO_BALLOON_F_FREE_PAGE_HINT,
because
>>>> this has no such side effects. Always register the shrinker
with
>>>> VIRTIO_BALLOON_F_FREE_PAGE_HINT now. We are always allowed to
reuse free
>>>> pages that are still to be processed by the guest. The
hypervisor takes
>>>> care of identifying and resolving possible races between
processing a
>>>> hinting request and the guest reusing a page.
>>>>
>>>> In contrast to pre commit 71994620bb25 ("virtio_balloon:
replace oom
>>>> notifier with shrinker"), don't add a moodule
parameter to configure the
>>>> number of pages to deflate on OOM. Can be re-added if really
needed.
>>>> Also, pay attention that leak_balloon() returns the number of
4k pages -
>>>> convert it properly in virtio_balloon_oom_notify().
>>>>
>>>> Testing done by Tyler for future reference:
>>>>   Test setup: VM with 16 CPU, 64GB RAM. Running Debian 10. We
have a 42
>>>>   GB file full of random bytes that we continually cat to
/dev/null.
>>>>   This fills the page cache as the file is read. Meanwhile we
trigger
>>>>   the balloon to inflate, with a target size of 53 GB. This
setup causes
>>>>   the balloon inflation to pressure the page cache as the page
cache is
>>>>   also trying to grow. Afterwards we shrink the balloon back to
zero (so
>>>>   total deflate = total inflate).
>>>>
>>>>   Without patch (kernel 4.19.0-5):
>>>>   Inflation never reaches the target until we stop the
"cat file >
>>>>   /dev/null" process. Total inflation time was 542
seconds. The longest
>>>>   period that made no net forward progress was 315 seconds (see
attached
>>>>   graph).
>>>>   Result of "grep balloon /proc/vmstat" after the
test:
>>>>   balloon_inflate 154828377
>>>>   balloon_deflate 154828377
>>>>
>>>>   With patch (kernel 5.6.0-rc4+):
>>>>   Total inflation duration was 63 seconds. No deflate-queue
activity
>>>>   occurs when pressuring the page-cache.
>>>>   Result of "grep balloon /proc/vmstat" after the
test:
>>>>   balloon_inflate 12968539
>>>>   balloon_deflate 12968539
>>>>
>>>>   Conclusion: This patch fixes the issue. In the test it
reduced
>>>>   inflate/deflate activity by 12x, and reduced inflation time
by 8.6x.
>>>>   But more importantly, if we hadn't killed the "grep
balloon
>>>>   /proc/vmstat" process then, without the patch, the
inflation process
>>>>   would never reach the target.
>>>>
>>>> [1]
https://www.spinics.net/lists/linux-virtualization/msg40863.html
>>>>
>>>> Reported-by: Tyler Sanderson <tysand at google.com>
>>>> Tested-by: Tyler Sanderson <tysand at google.com>
>>>> Fixes: 71994620bb25 ("virtio_balloon: replace oom notifier
with shrinker")
>>>> Cc: Michael S. Tsirkin <mst at redhat.com>
>>>> Cc: Wei Wang <wei.w.wang at intel.com>
>>>> Cc: Alexander Duyck <alexander.h.duyck at
linux.intel.com>
>>>> Cc: David Rientjes <rientjes at google.com>
>>>> Cc: Nadav Amit <namit at vmware.com>
>>>> Cc: Michal Hocko <mhocko at kernel.org>
>>>> Cc: Andrew Morton <akpm at linux-foundation.org>
>>>> Signed-off-by: David Hildenbrand <david at redhat.com>
>>>
>>> Acked-by: David Rientjes <rientjes at google.com>
>>>
>>
>> Thanks!
> 
> David could you repost with corrected commit log
> so Andrew can merge? To make sure it's not missed,
> it's probably a good idea to have a cover letter
> mentioning mm in the subject.
Yes, on my list, busy with some other stuff. Will make sure Andrew will
see this mail :)


-- 
Thanks,

David / dhildenb