On Sat, May 18, 2019 at 12:56:12AM +0000, Roman Gushchin wrote:
> On Fri, May 17, 2019 at 12:12:51PM +0200, Pavel Hrdina wrote:
> > On Thu, May 16, 2019 at 10:25:50AM -0700, Alexei Starovoitov wrote:
> > > On Thu, May 16, 2019 at 10:15 AM Roman Gushchin <guro@fb.com> wrote:
> > > >
> > > > On Thu, May 16, 2019 at 05:31:44PM +0200, Pavel Hrdina wrote:
> > > > > On Thu, May 16, 2019 at 03:22:33PM +0000, Roman Gushchin wrote:
> > > > > > On Thu, May 16, 2019 at 12:39:15PM +0200, Jiri Olsa wrote:
> > > > > > > hi,
> > > > > > > Pavel reported an issue with bpf programs (attached to cgroup)
> > > > > > > not being released at the time when the cgroup is removed and
> > > > > > > are still visible in 'bpftool prog' list afterwards.
> > > > > >
> > > > > > Hi Jiri!
> > > > > >
> > > > > > Can you, please, try the patch from
> > > > > > https://github.com/rgushchin/linux/commit/f77afa1952d81a1afa6c4872d342bf6721e148e2 ?
> > > > > >
> > > > > > It should solve the problem, and I'm about to post it upstream.
> > > > >
> > > > > Perfect, I'll give it a try with full libvirt setup as well.
> > > > >
> > > > > Can we have this somehow detectable from user-space so libvirt can
> > > > > decide when to use BPF or not?  I would like to avoid using BPF with
> > > > > libvirt if this issue is not fixed and we cannot simply workaround it
> > > > > as systemd automatically removes cgroups for us.
> > > >
> > > > Hm, I don't think there is a good way to detect it from userspace.
> > > > At least I have no good ideas. Alexei? Daniel?
> > > >
> > > > If you're interested in a particular stable version, we can probably
> > > > treat it as a "fix", and backport.
> > > 
> > > right.
> > > also user space workaround is trivial.
> > > Just detach before rmdir.
> > 
> > Well yes, it's trivial but not if you are using machined from systemd.
> > Once libvirt kills QEMU process systemd automatically removes the
> > cgroup so we don't have any chance to remove the BPF program.
> > 
> > Would it be too ugly to put something into
> > '/sys/kernel/cgroup/features'?
> 
> I thought about it, but it seems that /sys/kernel/cgroup/features is also
> relatively new. So if we're not going to backport it (I mean auto-detaching),
> than we can simple look at the kernel version, right?

If you think only about upstream then the version check is in most cases
good enough, but usually that's not the case and patches are backported
to downstream distributions as well.

Yes, that file was introduced in kernel 4.15 so there are some
limitations where the fix would be introspectable.

> If we're going to backport it, the question is which stable version we're
> looking at.
> 
> In general, I don't see any reasons why cgroup/features can't be used.

Perfect, in that case I would prefer if we could export it in
cgroup/features as it will be easier for user-space to figure out
whether it's safe to relay on proper cleanup behavior or not and
it will make downstream distributions life easier.

I'll try the patch today with libvirt setup.

Thanks,

Pavel