Re: [PATCH v13 07/32] x86,fs/resctrl: Use struct rdt_domain_hdr when reading counters

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 10/29/25 9:20 AM, Tony Luck wrote:
> @@ -497,8 +497,18 @@ static int __l3_mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
>  static int __mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
>  {
>  	switch (rr->r->rid) {
> -	case RDT_RESOURCE_L3:
> -		return __l3_mon_event_count(rdtgrp, rr);
> +	case RDT_RESOURCE_L3: {
> +		struct rdt_mon_domain *d = NULL;
> +
> +		if (rr->hdr) {
> +			if (!domain_header_is_valid(rr->hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3)) {
> +				rr->err = -EIO;
> +				return -EINVAL;
> +			}
> +			d = container_of(rr->hdr, struct rdt_mon_domain, hdr);
> +		}
> +		return __l3_mon_event_count(rdtgrp, rr, d);
> +	}

I tried running this series through a static checker and it flagged a few issues related to
this flow. The issues appear to be false positives but it demonstrates that this code is
becoming very hard to understand. Consider, for example, how __l3_mon_event_count() is
structured while thinking about d==NULL:


	__l3_mon_event_count()
	{
		...
		if (rr->is_mbm_cntr) {
			/* dereferences d */
		}

		if (rr->first) {
			/* dereferences d */
			return 0;
		}

		if (d) {
			/* dereferences d */
			return 0;
		}

		/* sum code */
	}

I believe it will be difficult for somebody to trace that rr->is_mbm_cntr and rr->first cannot
be true of d==NULL (the static checker issues supports this). The "if (d)" test that follows
these checks just adds to difficulty by implying that d could indeed be NULL before then.

I see two options to address this. I tried both and the static checker was ok with either. I find the
second option easier to understand than the first, but I share both for context:
option 1:

To make it obvious when the domain can be NULL:

	__l3_mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
	{
		...
		if (rr->hdr) {
			if (!domain_header_is_valid(rr->hdr, RESCTRL_MON_DOMAIN, RDT_RESOURCE_L3)) {
				rr->err = -EIO;
				return -EINVAL;
			}
			d = container_of(rr->hdr, struct rdt_mon_domain, hdr);

			if (rr->is_mbm_cntr) {
				/* dereferences d */
			}

			if (rr->first) {
				/* dereferences d */
				return 0;
			}

			/* dereferences d */
			return 0;
		}
		
		/* sum code */
	}
	
While easier to understand the above does not make the code easier to read. The function is already quite long
and this adds an additional indentation level. This does not seem necessary since the rr->hdr!=NULL
scenario really just looks like a "function within a function" since it does a "return".
	
This brings to:
option 2:
Split __l3_mon_event_count() into, for example, __l3_mon_event_count() that handles the rr->hdr!=NULL
flow and __l3_mon_event_count_sum() that handles the rr->hdr==NULL flow.
This can be called from __mon_event_count():

	if (rr->hdr)
		return __l3_mon_event_count(rdtgrp, rr);
	else
		return __l3_mon_event_count_sum(rdtgrp, rr);

Option 2 looks like the better option to me. What do you think?

Reinette