Re: [PATCH 4.14 108/159] kvm, mm: account kvm related kmem slabs to kmemcg

[Michal Hocko]

On Fri 22-12-17 18:07:23, Sasha Levin wrote:
> On Fri, Dec 22, 2017 at 06:56:16PM +0100, Michal Hocko wrote:
> >On Fri 22-12-17 17:40:10, Sasha Levin wrote:
> >> On Fri, Dec 22, 2017 at 02:06:07PM +0100, Michal Hocko wrote:
> >> >On Fri 22-12-17 13:41:22, Greg KH wrote:
> >> >> On Fri, Dec 22, 2017 at 10:34:07AM +0100, Michal Hocko wrote:
> >> >> > On Fri 22-12-17 09:46:33, Greg KH wrote:
> >> >> > > 4.14-stable review patch.  If anyone has any objections, please let me know.
> >> >> > >
> >> >> > > ------------------
> >> >> > >
> >> >> > > From: Shakeel Butt <shakeelb@xxxxxxxxxx>
> >> >> > >
> >> >> > >
> >> >> > > [ Upstream commit 46bea48ac241fe0b413805952dda74dd0c09ba8b ]
> >> >> > >
> >> >> > > The kvm slabs can consume a significant amount of system memory
> >> >> > > and indeed in our production environment we have observed that
> >> >> > > a lot of machines are spending significant amount of memory that
> >> >> > > can not be left as system memory overhead. Also the allocations
> >> >> > > from these slabs can be triggered directly by user space applications
> >> >> > > which has access to kvm and thus a buggy application can leak
> >> >> > > such memory. So, these caches should be accounted to kmemcg.
> >> >> > >
> >> >> > > Signed-off-by: Shakeel Butt <shakeelb@xxxxxxxxxx>
> >> >> > > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> >> >> > > Signed-off-by: Sasha Levin <alexander.levin@xxxxxxxxxxx>
> >> >> > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >> >> >
> >> >> > The patch is not marked for stable, neither it fixes an existing bug.
> >> >> > It is a nice to have thing for sure but I am wondering how this got
> >> >> > through stable-filter.
> >> >>
> >> >> Sasha picked it out, and it seemed like a sane thing to backport.  If
> >> >> you think it's not worthy, I'll gladly drop it, but it seemed like such
> >> >> a simple bugfix to include.
> >> >
> >> >It is not that I would have some specific concerns about this particular
> >> >patch. It is more of a worry about the overal process. I thought that
> >> >_any_ patch backported to the stable tree would require a specific bug
> >> >to be fixed or in exceptional cases a performance issue. I have
> >> >experienced this pushback myself when trying to push "no real bug report
> >> >but better to have this plugged" patches.
> >> >
> >> >So something has apparently changed in the process, I just haven't
> >> >noticed it. I am worried this might lead to more regression in future.
> >> >Not that my worry counts all that much as I am not a stable kernel user
> >> >though. So this is just my 2c worth of worry.
> >>
> >> The way I see it is that stable commits are supposed to fix a bug that
> >> a user can hit/exploit, it doesn't have to have an actual user
> >> complaining about it.
> >>
> >> For this particular commit, the way I read it is that a user can avoid
> >> his kmemcg limits (maybe maliciously), which would qualify as an
> >> actual bug we want to get fixed.
> >
> >How are you going to judge all the possible relations to other
> >subsystems? I mean there is a good reason maintainers mark patches for
> >stable trees. How do you want to competently decide this for them? Can
> >you do that for all subsystems?
> >
> >I do not want to underestimate your judgment or misinterpret your
> >process here but I _believe_ that picking patches based on the changelog
> >without a deep understanding of the subsystem is really risky. We do
> >not really have to go a long way to see that. Just look at other patch
> >in this very thread [1]. But maybe our our understanding of the stable
> >trees are different.
> 
> I don't try and override maintainers, I mostly try to get fixes out
> of subsystems where maintainers/authors partially (or just don't)
> mark their commits for stable.

Well, I have see quite some MM patches and I believe we are quite good
at marking patches for stable trees... I also think we we (as the whole
kernel) are much better are using Fixes tag (although it is over used
sometimes).

Moreover it makes more sense to push on those maintainers than try to
substitude them without being so closely familiar with the subsystem. If
missing backports result in bug reports then this just increase the
pressure on those maintainers /me think.

> These patches also go through a much longer review process than
> commits that are marked for stable (there are at least 3 emails issued
> for each such commit, and at least 1 week (usually much more) is
> given for reviews).

Does any of the maintainers read those emails? How many acks/reviewes do
you get for those patches for the stable tree? To be honest I tend to
ignore those patchbombs most of the time because it is simply impossible
to handle them for me. I try to help backporting obvious fixes but
reviewing seemingly randomly selected patch which applies and changelog
looks reasonaly is simply out of my time budget. Not to mention that
this is not just about the patch itself but also the tree it is applied
to and other patches that are in the same pile.

-- 
Michal Hocko
SUSE Labs