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
