Hello.
(I hope I'm replying to the latest iteration and it has some validitiy
still. Sorry for my late reply. Few points caught my attention.)
On Tue, Oct 24, 2023 at 05:07:25PM +0100, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> @@ -15,10 +17,28 @@ struct drm_cgroup_state {
> struct cgroup_subsys_state css;
>
> struct list_head clients;
> +
> + unsigned int weight;
> +
> + unsigned int sum_children_weights;
> +
> + bool over;
> + bool over_budget;
> +
> + u64 per_s_budget_us;
Nit: sounds reversed (cf USEC_PER_SEC).
> +static int drmcg_period_ms = 2000;
> +module_param(drmcg_period_ms, int, 0644);
> +
cgroup subsystems as loadable modules were abandoded long time ago.
I'm not sure if this works as expected then.
The common way is __seutp(), see for instance __setup() in mm/memcontrol.c
> +static bool __start_scanning(unsigned int period_us)
...
> + css_for_each_descendant_post(node, &root->css) {
> + struct drm_cgroup_state *drmcs = css_to_drmcs(node);
> + struct drm_cgroup_state *parent;
> + u64 active;
> +
> + if (!css_tryget_online(node))
> + goto out;
> + if (!node->parent) {
> + css_put(node);
> + continue;
> + }
I think the parent check can go first witout put'ting (RCU is enough for
node).
> + if (!css_tryget_online(node->parent)) {
> + css_put(node);
> + goto out;
> + }
Online parent is implied onlined node. So this can be removed.
...
> +
> + css_put(node);
> + }
I wonder if the first passes could be implemented with rstat flushing
and then only invoke signalling based on the data. (As that's
best-effort).
> +
> + /*
> + * 4th pass - send out over/under budget notifications.
> + */
> + css_for_each_descendant_post(node, &root->css) {
> + struct drm_cgroup_state *drmcs = css_to_drmcs(node);
> +
> + if (!css_tryget_online(node))
> + goto out_retry;
> +
> + if (drmcs->over || drmcs->over_budget)
> + drmcs_signal_budget(drmcs,
> + drmcs->active_us,
> + drmcs->per_s_budget_us);
> + drmcs->over_budget = drmcs->over;
> +
> + css_put(node);
> + }
> static struct cgroup_subsys_state *
> @@ -82,6 +365,7 @@ drmcs_alloc(struct cgroup_subsys_state *parent_css)
>
> if (!parent_css) {
> drmcs = &root_drmcs.drmcs;
> + INIT_DELAYED_WORK(&root_drmcs.scan_work, scan_worker);
This reminds me discussion
https://lore.kernel.org/lkml/rlm36iypckvxol2edyr25jyo4imvlidtepbcjdaa2ouvwh3wjq@pqyuk3v2jesb/
I.e. worker needn't be initialized if controller is "invisible".
> +static void drmcs_attach(struct cgroup_taskset *tset)
> +{
> + struct drm_cgroup_state *old = old_drmcs;
> + struct cgroup_subsys_state *css;
> + struct drm_file *fpriv, *next;
> + struct drm_cgroup_state *new;
> + struct task_struct *task;
> + bool migrated = false;
> +
> + if (!old)
> + return;
> +
> + task = cgroup_taskset_first(tset, &css);
> + new = css_to_drmcs(task_css(task, drm_cgrp_id));
> + if (new == old)
> + return;
> +
> + mutex_lock(&drmcg_mutex);
> +
> + list_for_each_entry_safe(fpriv, next, &old->clients, clink) {
> + cgroup_taskset_for_each(task, css, tset) {
> + struct cgroup_subsys_state *old_css;
> +
> + if (task->flags & PF_KTHREAD)
> + continue;
I'm curious, is this because of particular kthreads? Or would it fail
somehow below? (Like people should not migrate kthreads normally, so
their expectation cannot be high.)
Michal
Attachment:
signature.asc
Description: PGP signature
