Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1710767 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] x86/intel_rdt/cqm: Improve limbo list processing Thomas Gleixner <tglx@linutronix.de> - 2017-08-14 11:50 +0200
    Re: [PATCH 3/3] x86/intel_rdt/cqm: Improve limbo list processing Shivappa Vikas <vikas.shivappa@intel.com> - 2017-08-14 21:20 +0200
      Re: [PATCH 3/3] x86/intel_rdt/cqm: Improve limbo list processing Shivappa Vikas <vikas.shivappa@intel.com> - 2017-08-14 23:20 +0200

#1710767 — 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

[toc] | [next] | [standalone]

#1711367

On Mon, 14 Aug 2017, Thomas Gleixner wrote:

> 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 we logically offline all cpus in a package and bring it back, the worker 
needs to be scheduled on the package if there were busy RMIDs on this package. 
Otherwise that RMID never gets freed as its rmid->busy stays 1..

I needed to scan the limbo list and set the bits for all limbo RMIDs after the 
alloc and before doing the 'has_busy_rmid' check. Will fix

>
>>  	}
>>  	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?

Will fix the line break. The has_busy_rmid makes sure the worker was indeed 
scheduled - that way we cancel the worker which was actually scheduled..

>
>> +			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.

Ok will fix. We can flush(setup and run it immediately) the work here 
on the new cpu.

>
>> +		}
>> +		if (is_llc_occupancy_enabled() && cpu == d->mbm_work_cpu &&
>
> That want's to be d->cbm_work_cpu, right?

Correct - thanks for pointing , will fix.

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

For cqm 1s is not a hard requirement, but can flush the work like mbm to keep it 
uniform..

Thanks,
Vikas

>
> Thanks,
>
> 	tglx
>

[toc] | [prev] | [next] | [standalone]

#1711465

On Mon, 14 Aug 2017, Shivappa Vikas wrote:

>
>
> On Mon, 14 Aug 2017, Thomas Gleixner wrote:
>
>> 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 we logically offline all cpus in a package and bring it back, the worker 
> needs to be scheduled on the package if there were busy RMIDs on this 
> package. Otherwise that RMID never gets freed as its rmid->busy stays 1..
>
> I needed to scan the limbo list and set the bits for all limbo RMIDs after 
> the alloc and before doing the 'has_busy_rmid' check. Will fix

Tony pointed out that there is no guarentee that a domain will come back up once 
its down, so the above issue of rmid->busy staying at > 0 can still happen. 
So I will delete this -
 		if (has_busy_rmid(r, d))
 			cqm_setup_limbo_handler(d);

and add this when a domain is powered down -
 	for each rmid in d->rmid_busy_llc
 		if (--entry->busy)
 			free_rmid(rmid);

We have no way to know if the L3 was indeed flushed (or package was powered 
off). This may lead to incorrect counts in rare scenarios but can document the 
same.

Thanks,
vikas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web