Re: [PATCH v1 03/20] x86/resctrl: Add domain online callback for resctrl work

[Reinette Chatre]

Hi James,

On 7/29/2021 3:35 PM, James Morse wrote:
> Because domains are exposed to user-space via resctrl, the filesystem
> must update its state when cpu hotplug callbacks are triggered.

Could you please replace "cpu" with "CPU" throughout the series in 
changelogs and comments?

> Some of this work is common to any architecture that would support
> resctrl, but the work is tied up with the architecture code to
> allocate the memory.

(I read the above as problem statement.)

> Move domain_setup_mon_state(), the monitor subdir creation call and the
> mbm/limbo workers into a new resctrl_online_domain() call.

(I read the above as what the code does.)

Could you please add description on what the patch does to address the 
problem you describe?

...

> +}
> +
> +int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
> +{
> +	int err;
> +
> +	lockdep_assert_held(&rdtgroup_mutex); // the arch code took this for us
> +

Please do not use trailing comments.

> +	if (!r->mon_capable)
> +		return 0;
> +
> +	err = domain_setup_mon_state(r, d);
> +	if (err)
> +		return err;
> +
> +	if (is_mbm_enabled()) {
> +		INIT_DELAYED_WORK(&d->mbm_over, mbm_handle_overflow);
> +		mbm_setup_overflow_handler(d, MBM_OVERFLOW_INTERVAL);
> +	}
> +
> +	if (is_llc_occupancy_enabled())
> +		INIT_DELAYED_WORK(&d->cqm_limbo, cqm_handle_limbo);
> +

You also seem to address an issue where this work was not properly 
cleaned up on the error paths of the replaced domain_setup_mon_state(). 
Thank you.

> +	/* If resctrl is mounted, add per domain monitor data directories. */
> +	if (static_branch_unlikely(&rdt_enable_key))

Should this be rdt_mon_enable_key instead?

Reinette