Re: [RFC PATCH 1/3] misc_cgroup: introduce misc.events and misc_events.local

[Michal Koutný]

Hello Chunguang.

The new version looks like a good step generally. 

My main remark is that I wouldn't make a distinct v1 and v2 interface,
it's a new controller so I think the v2 could be exposed in both cases
(or in other words, don't create new v1-specific features).

On Wed, Sep 08, 2021 at 01:24:34PM +0800, brookxu <brookxu.cn@xxxxxxxxx> wrote:
> Introduce misc.events and misc.events.local to make it easier for
> us to understand the pressure of resources. The main idea comes
> from mem_cgroup.

BTW what are the events you really are interesed in? (See also the
proposal in my reply to 1/3.)

> @@ -171,6 +171,16 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
>  	return 0;
>  
>  err_charge:
> +	if (cgroup_subsys_on_dfl(misc_cgrp_subsys)) {
> +		atomic_long_inc(&i->events_local[type]);
> +		cgroup_file_notify(&i->events_local_file);
> +
> +		for (k = i; k; k = parent_misc(k)) {
> +			atomic_long_inc(&k->events[type]);
> +			cgroup_file_notify(&k->events_file);
> +		}
> +	}
> +

No reason to wrap this for the default hierarchy only.

> +static int misc_events_show(struct seq_file *sf, void *v)
> +{
> +	struct misc_cg *cg = css_misc(seq_css(sf));
> +	unsigned long count, i;
> +
> +	for (i = 0; i < MISC_CG_RES_TYPES; i++) {
> +		count = atomic_long_read(&cg->events[i]);
> +		if (READ_ONCE(misc_res_capacity[i]) || count)
> +			seq_printf(sf, "%s %lu\n", misc_res_name[i], count);

More future-proof key would be
			seq_printf(sf, "%s.max %lu\n", misc_res_name[i], count);
or
			seq_printf(sf, "max.%s %lu\n", misc_res_name[i], count);

(Which one is a judgement call but I'd include the "name" of event type too.)

> +static int misc_events_local_show(struct seq_file *sf, void *v)
> +{
> +	struct misc_cg *cg = css_misc(seq_css(sf));
> +	unsigned long count, i;
> +
> +	for (i = 0; i < MISC_CG_RES_TYPES; i++) {
> +		count = atomic_long_read(&cg->events_local[i]);
> +		if (READ_ONCE(misc_res_capacity[i]) || count)
> +			seq_printf(sf, "%s %lu\n", misc_res_name[i], count);

Ditto.

Thanks,
Michal