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

Groups > linux.kernel > #1682818 > unrolled thread

Re: [PATCH 21/21] x86/intel_rdt/mbm: Handle counter overflow

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 21/21] x86/intel_rdt/mbm: Handle counter overflow Shivappa Vikas <vikas.shivappa@intel.com> - 2017-07-07 02:00 +0200
    Re: [PATCH 21/21] x86/intel_rdt/mbm: Handle counter overflow Thomas Gleixner <tglx@linutronix.de> - 2017-07-07 09:00 +0200
      Re: [PATCH 21/21] x86/intel_rdt/mbm: Handle counter overflow "Luck, Tony" <tony.luck@intel.com> - 2017-07-10 20:00 +0200
        Re: [PATCH 21/21] x86/intel_rdt/mbm: Handle counter overflow Thomas Gleixner <tglx@linutronix.de> - 2017-07-11 17:30 +0200

#1682818 — Re: [PATCH 21/21] x86/intel_rdt/mbm: Handle counter overflow

On Sun, 2 Jul 2017, Thomas Gleixner wrote:

> On Mon, 26 Jun 2017, Vikas Shivappa wrote:
>> +static void mbm_update(struct rdt_domain *d, int rmid)
>> +{
>> +	struct rmid_read rr;
>> +
>> +	rr.first = false;
>> +	rr.d = d;
>> +
>> +	if (is_mbm_total_enabled()) {
>> +		rr.evtid = QOS_L3_MBM_TOTAL_EVENT_ID;
>> +		__mon_event_count(rmid, &rr);
>
> This is broken as it is not protected against a concurrent read from user
> space which comes in via a smp function call.

The read from user also has the rdtgroup_mutex.

Thanks,
Vikas

>
> This means both the internal state and __rmid_read() are unprotected.
>
> I'm not sure whether it's enough to disable interrupts around
> __mon_event_count(), but that's the minimal protection required. It's
> definitely good enough for __rmid_read(), but it might not be sufficient
> for protecting domain->mbm_[local|total]. I leave the exercise of figuring
> that out to you.
>
> Thanks,
>
> 	tglx
>

[toc] | [next] | [standalone]

#1682966

On Thu, 6 Jul 2017, Shivappa Vikas wrote:
> On Sun, 2 Jul 2017, Thomas Gleixner wrote:
> > On Mon, 26 Jun 2017, Vikas Shivappa wrote:
> > > +static void mbm_update(struct rdt_domain *d, int rmid)
> > > +{
> > > +	struct rmid_read rr;
> > > +
> > > +	rr.first = false;
> > > +	rr.d = d;
> > > +
> > > +	if (is_mbm_total_enabled()) {
> > > +		rr.evtid = QOS_L3_MBM_TOTAL_EVENT_ID;
> > > +		__mon_event_count(rmid, &rr);
> > 
> > This is broken as it is not protected against a concurrent read from user
> > space which comes in via a smp function call.
> 
> The read from user also has the rdtgroup_mutex.

Which is again, completely non obvious and undocumented in the code.

Aside of that, are you really serious about serializing the world and
everything on a single global mutex?

Thanks,

	tglx

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

#1684548

On Fri, Jul 07, 2017 at 08:50:40AM +0200, Thomas Gleixner wrote:
> Aside of that, are you really serious about serializing the world and
> everything on a single global mutex?

It would be nice to not do that, but there are challenges. At
any instant someone else might run:

# rmdir /sys/fs/resctrl/{some_control_group}

and blow away the control group and all the monitor groups under
it.

Someone else might do:

# echo 0 > /sys/devices/system/cpu/cpu{N}/online

where "N" is the last online cpu in a domain, which will
blow away an rdt_domain structure and ask kernfs to remove
some monitor files from every monitor directory.


If we change how we handle rdt_domains to

1) Not delete them when last CPU goes away (and re-use them
   if they come back)
2) Have a safe way to search rdt_resource.domains for a domain
   that we know is there even though another may be in the middle
   of being added

Then we could probably make:

$ cat /sys/fs/restrl/ ... /llc_occupancy

etc. not need to grab the mutex.  We'd still need something
to protect against a cross processor interrupt geting in the
middle of the access to IA32_QM_EVTSEL/IA32_QM_CTR and for
MBM counters to serialize access to mbm_state ... but it would
be a lot finer granularity.

-Tony

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

#1685141

On Mon, 10 Jul 2017, Luck, Tony wrote:
> On Fri, Jul 07, 2017 at 08:50:40AM +0200, Thomas Gleixner wrote:
> > Aside of that, are you really serious about serializing the world and
> > everything on a single global mutex?
> 
> It would be nice to not do that, but there are challenges. At
> any instant someone else might run:
> 
> # rmdir /sys/fs/resctrl/{some_control_group}
> 
> and blow away the control group and all the monitor groups under
> it.
> 
> Someone else might do:
> 
> # echo 0 > /sys/devices/system/cpu/cpu{N}/online
> 
> where "N" is the last online cpu in a domain, which will
> blow away an rdt_domain structure and ask kernfs to remove
> some monitor files from every monitor directory.
> 
> 
> If we change how we handle rdt_domains to
> 
> 1) Not delete them when last CPU goes away (and re-use them
>    if they come back)
> 2) Have a safe way to search rdt_resource.domains for a domain
>    that we know is there even though another may be in the middle
>    of being added
> 
> Then we could probably make:
> 
> $ cat /sys/fs/restrl/ ... /llc_occupancy
> 
> etc. not need to grab the mutex.  We'd still need something
> to protect against a cross processor interrupt geting in the
> middle of the access to IA32_QM_EVTSEL/IA32_QM_CTR and for
> MBM counters to serialize access to mbm_state ... but it would
> be a lot finer granularity.

Thanks for the explanation. Yes, that would be nice, but we can start off
with the global mutex and think about the scalability issue after we got
the functionality itself under control.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web