Messages in this thread | | | From | Linus Torvalds <> | Date | Fri, 10 Dec 2021 09:53:41 -0800 | Subject | Re: [PATCH 2/6] cgroup: Allocate cgroup_file_ctx for kernfs_open_file->priv |
| |
On Thu, Dec 9, 2021 at 1:47 PM Tejun Heo <tj@kernel.org> wrote: > > of->priv is currently used by each interface file implementation to store > private information. This patch collects the current two private data usages > into struct cgroup_file_ctx which is allocated and freed by the common path. > This allows generic private data which applies to multiple files, which will > be used to in the following patch.
I'm not sure if it's worth it having that union just to make the struct be 8 bytes instead of 16 (and later 16 bytes instead of 24), when the real cost is that dynamic allocation overhead, and there's likely only one or two active actual allocations at a time.
IOW, I'm not convinced there's any real memory savings, and making it a union means that now you have to be very careful about who does what..
And yes, people historically had to be very careful _anyway_, because the "union" was kind of implicit in how there was just a shared 'void *priv' thing that was either that iterator pointer or that psi trigger pointer.
So in that sense this is a semantically minimal patch, but I think that practically speaking we'd be better off without that possible source of confusion, and just always have that cgroup proc file have a full structure.
In fact, I think it means we could just make the thing *contain* the iterator, instead of having a pointer to an iterator, and getting rid of the now redundant dynamic alloc/free of the iterator pointer.
Wouldn't that simplify things? And might there not be some cgroup pressure user that also wants to use the iterator interfaces? Maybe not, my point is more that once we have an explicit struct allocation for cgroup proc files, we might as well clarify and simplify the code..
Linus
| |