Re: [PATCH RFC v3 06/11] RISC-V: QoS: add resctrl setup and domain management

On Thu, Apr 30, 2026 at 04:20:09PM -0700, Reinette Chatre wrote:
> Hi Drew,
> 
> On 4/14/26 6:54 PM, Drew Fustini wrote:
> > +
> > +static int qos_resctrl_add_controller_domain(struct cbqri_controller *ctrl)
> > +{
> > +	struct rdt_ctrl_domain *domain;
> > +	struct cbqri_resctrl_res *cbqri_res = NULL;
> > +	struct rdt_resource *res = NULL;
> > +	struct list_head *pos = NULL;
> > +	int err;
> > +
> > +	domain = qos_new_domain(ctrl);
> > +	if (!domain)
> > +		return -ENOSPC;
> > +
> > +	switch (ctrl->type) {
> > +	case CBQRI_CONTROLLER_TYPE_CAPACITY:
> > +		cpumask_copy(&domain->hdr.cpu_mask, &ctrl->cache.cpu_mask);
> 
> Looking at patch #10 ctrl->cache.cpu_mask contains all CPUs associated with cache
> even if they are offline. This is not what resctrl expects. Instead the expectation is
> that a domain exists and is online (hence "resctrl_online_ctrl_domain()") if at least one CPU
> belonging to that domain is online and domain->hdr.cpu_mask lists all the *online* CPUs
> associated with that domain.
> This is why resctrl always takes the CPU hotplug lock when traversing the domain
> lists.
> 
> I thus expected this initialization to be split between an early initialization of
> resource capabilities and then domain initialization as part of the CPU online/offline
> handlers.

Good point. I'll rework this so that domain allocation moves to the cpu
online/offline handlers.

[..]
> > +	if (pos)
> > +		list_add_tail(&domain->hdr.list, pos);
> > +	else
> > +		list_add_tail(&domain->hdr.list, &res->ctrl_domains);
> 
> resctrl_find_domain() returns NULL if it cannot find an existing domain, in that
> case it initializes "pos" to support adding a new domain in a sorted list.
> Expectation is that domains are managed as part of CPU hotplug handlers. When
> a CPU comes online then handler can check if the domain it belongs to already exists,
> if it does then the CPU can just be added to that domain's cpu_mask, if it does
> not then a new domain is created and added in the the appropriate spot in the
> sorted list (based on domain ID) of domains. 

Okay, I'll move this from probe to qos_resctrl_online_cpu().

> > +int qos_resctrl_online_cpu(unsigned int cpu)
> > +{
> > +	resctrl_online_cpu(cpu);
> 
> This is where a domain is expected to be added when its first CPU comes online.
[..]
> > +int qos_resctrl_offline_cpu(unsigned int cpu)
> > +{
> > +	resctrl_offline_cpu(cpu);
> 
> This is where a domain is expected to be removed when its last CPU goes offline.

Okay, I'll update v4 to add and remove domains in these hooks.

Thanks,
Drew