On Wed, May 4, 2022 at 5:26 AM Michal Koutný <mkoutny@xxxxxxxx> wrote: > > Hello. > > On Mon, May 02, 2022 at 11:19:36PM +0000, "T.J. Mercier" <tjmercier@xxxxxxxxxx> wrote: > > This patch adds APIs to: > > -allow a device to register for memory accounting using the GPU cgroup > > controller. > > -charge and uncharge allocated memory to a cgroup. > > Is this API for separately built consumers? > The respective functions should be exported (EXPORT_SYMBOL_GPL) if I > haven't missed anything. > As the only users are dmabuf heaps and dmabuf, and those cannot be built as modules I did not export the symbols here. However these definitely would need to be exported to support use by modules, and I have had to do that in one of my device test trees for this change. Should I export these now for this series? > > +#ifdef CONFIG_CGROUP_GPU > > + /* The GPU cgroup controller data structure */ > > +struct gpucg { > > + struct cgroup_subsys_state css; > > + > > + /* list of all resource pools that belong to this cgroup */ > > + struct list_head rpools; > > +}; > > + > > +/* A named entity representing bucket of tracked memory. */ > > +struct gpucg_bucket { > > + /* list of various resource pools in various cgroups that the bucket is part of */ > > + struct list_head rpools; > > + > > + /* list of all buckets registered for GPU cgroup accounting */ > > + struct list_head bucket_node; > > + > > + /* string to be used as identifier for accounting and limit setting */ > > + const char *name; > > +}; > > Do these struct have to be defined "publicly"? > I.e. the driver code could just work with gpucg and gpucg_bucket > pointers. > > > +int gpucg_register_bucket(struct gpucg_bucket *bucket, const char *name) > > ...and the registration function would return a pointer to newly > (internally) allocated gpucg_bucket. > No, except maybe the gpucg_bucket name which I can add an accessor function for. Won't this mean depending on LTO for potential inlining of the functions currently implemented in the header? I'm happy to make this change, but I wonder why some parts of the kernel take this approach and others do not. > Regards, > Michal