Re: [RFC 4/6] misc cgroup: introduce an fd counter

[Tycho Andersen <tycho@tycho.pizza>]

On Thu, Nov 09, 2023 at 10:53:17AM +0100, Christian Brauner wrote:
> > @@ -411,9 +453,22 @@ struct files_struct *dup_fd(struct files_struct *oldf, unsigned int max_fds, int
> >  
> >  	rcu_assign_pointer(newf->fdt, new_fdt);
> >  
> > -	return newf;
> > +	if (!charge_current_fds(newf, count_open_files(new_fdt)))
> > +		return newf;
> 
> 
> > @@ -542,6 +600,10 @@ static int alloc_fd(unsigned start, unsigned end, unsigned flags)
> >  	if (error)
> >  		goto repeat;
> >  
> > +	error = -EMFILE;
> > +	if (charge_current_fds(files, 1) < 0)
> > +		goto out;
> 
> Whoops, I had that message ready to fire but didn't send it.
> 
> This may have a noticeable performance impact as charge_current_fds()
> calls misc_cg_try_charge() which looks pretty expensive in this
> codepath.
> 
> We're constantly getting patches to tweak performance during file open
> and closing and adding a function that does require multiple atomics and
> spinlocks won't exactly improve this.

I don't see any spin locks in misc_cg_try_charge(), but it does walk
up the tree, resulting in multiple atomic writes.
If we didn't walk up the tree it would change the semantics, but
Netflix probably wouldn't delegate this, so at least for our purposes
having only one atomic would be sufficient. Is that more tenable?

> On top of that I really dislike that we're pulling cgroups into this
> code here at all.
> 
> Can you get a similar effect through a bpf program somehow that you
> don't even tie this to cgroups?

Possibly, I can look into it.

Tycho