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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Aug 2017 11:45:45 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Vikas Shivappa <vikas.shivappa@...ux.intel.com>
cc:     vikas.shivappa@...el.com, x86@...nel.org,
        linux-kernel@...r.kernel.org, hpa@...or.com, peterz@...radead.org,
        ravi.v.shankar@...el.com, tony.luck@...el.com,
        fenghua.yu@...el.com, eranian@...gle.com, davidcc@...gle.com,
        ak@...ux.intel.com, sai.praneeth.prakhya@...el.com
Subject: Re: [PATCH 3/3] x86/intel_rdt/cqm: Improve limbo list processing

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