On Tue, Nov 07, 2023 at 05:26:45PM -0700, Tycho Andersen wrote:
> + if (!charge_current_fds(newf, count_open_files(new_fdt)))
> + return newf;
Are you sure that on configs that are not cgroup-infested compiler
will figure out that count_open_files() would have no side effects
and doesn't need to be evaluated?
Incidentally, since you are adding your charge/uncharge stuff on each
allocation/freeing, why not simply maintain an accurate counter, cgroup or
no cgroup? IDGI... Make it an inlined helper right there in fs/file.c,
doing increment/decrement and, conditional upon config, calling
the cgroup side of things. No need to look at fdt, etc. outside
of fs/file.c either - the counter can be picked right from the
files_struct...
> static void __put_unused_fd(struct files_struct *files, unsigned int fd)
> {
> struct fdtable *fdt = files_fdtable(files);
> + if (test_bit(fd, fdt->open_fds))
> + uncharge_current_fds(files, 1);
Umm... Just where do we call it without the bit in ->open_fds set?
Any such caller would be a serious bug; suppose you are trying to
call __put_unused_fd(files, N) while N is not in open_fds. Just before
your call another thread grabs a descriptor and picks N. Resulting
state won't be pretty, especially if right *after* your call the
third thread also asks for a descriptor - and also gets N.
Sure, you have an exclusion on ->file_lock, but AFAICS all callers
are under it and in all callers except for put_unused_fd() we
have just observed a non-NULL file reference in ->fd[N]; that
would *definitely* be a hard constraint violation if it ever
happened with N not in ->open_fds at that moment.
So the only possibility would be a broken caller of put_unused_fd(),
and any such would be a serious bug.
Details, please - have you ever observed that?
BTW, what about the locking hierarchy? In the current tree ->files_lock
nests inside of everything; what happens with your patches in place?
