linux-kernel - Re: [RFC 6/8] cgroup/drm: Introduce weight based drm cgroup control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <lisfkz7wwhwwe3wkc76ufemlotaguuxslymtz6gxfreu62u65z@dsa3zpaxtjo3>
Date: Wed, 7 Feb 2024 16:28:12 +0100
From: Michal Koutný <mkoutny@...e.com>
To: Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
Cc: Intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org, 
	cgroups@...r.kernel.org, linux-kernel@...r.kernel.org, Tejun Heo <tj@...nel.org>, 
	Johannes Weiner <hannes@...xchg.org>, Zefan Li <lizefan.x@...edance.com>, 
	Dave Airlie <airlied@...hat.com>, Daniel Vetter <daniel.vetter@...ll.ch>, 
	Rob Clark <robdclark@...omium.org>, Stéphane Marchesin <marcheu@...omium.org>, 
	"T . J . Mercier" <tjmercier@...gle.com>, Kenny.Ho@....com, 
	Christian König <christian.koenig@....com>, Brian Welty <brian.welty@...el.com>, 
	Tvrtko Ursulin <tvrtko.ursulin@...el.com>
Subject: Re: [RFC 6/8] cgroup/drm: Introduce weight based drm cgroup control

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@...ux.intel.com> 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

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)