Re: [PATCH 3/3] x86/intel_rdt/cqm: Improve limbo list processing

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 9 Aug 2017, Vikas Shivappa wrote:

> @@ -426,6 +426,9 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_domain *d)
>  					   GFP_KERNEL);
>  		if (!d->rmid_busy_llc)
>  			return -ENOMEM;
> +		INIT_DELAYED_WORK(&d->cqm_limbo, cqm_handle_limbo);
> +		if (has_busy_rmid(r, d))
> +			cqm_setup_limbo_handler(d);

This is beyond silly. d->rmid_busy_llc is allocated a few lines above. How
would a bit be set here?

>  	}
>  	if (is_mbm_total_enabled()) {
>  		tsize = sizeof(*d->mbm_total);
> @@ -536,11 +539,25 @@ static void domain_remove_cpu(int cpu, struct rdt_resource *r)
>  		list_del(&d->list);
>  		if (is_mbm_enabled())
>  			cancel_delayed_work(&d->mbm_over);
> +
> +		if (is_llc_occupancy_enabled() &&
> +		    has_busy_rmid(r, d))

What is that line break helping here and why can't you just unconditionally
cancel the work?

> +			cancel_delayed_work(&d->cqm_limbo);
> +
>  		kfree(d);
> -	} else if (r == &rdt_resources_all[RDT_RESOURCE_L3] &&
> -		   cpu == d->mbm_work_cpu && is_mbm_enabled()) {
> -		cancel_delayed_work(&d->mbm_over);
> -		mbm_setup_overflow_handler(d);
> +		return;
> +	}
> +
> +	if (r == &rdt_resources_all[RDT_RESOURCE_L3]) {
> +		if (is_mbm_enabled() && cpu == d->mbm_work_cpu) {
> +			cancel_delayed_work(&d->mbm_over);
> +			mbm_setup_overflow_handler(d);

I think this is the wrong approach. If the timer is about to fire you
essentially double the interval. So you better flush the work, which will
reschedule it if needed.

> +		}
> +		if (is_llc_occupancy_enabled() && cpu == d->mbm_work_cpu &&

That want's to be d->cbm_work_cpu, right?

> +		    has_busy_rmid(r, d)) {
> +			cancel_delayed_work(&d->cqm_limbo);
> +			cqm_setup_limbo_handler(d);

See above.

Thanks,

	tglx