Re: [PATCH v3 05/46] perf/x86/intel/cmt: add per-package locks

[David Carrillo-Cisneros]

On Fri, Nov 11, 2016 at 1:41 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> On Fri, 11 Nov 2016, Peter Zijlstra wrote:
>> Well, its very hard to suggest alternatives, because there simply isn't
>> anything of content here. This patch just adds locks, and the next few
>> patches don't describe much either.
>>
>> Why can't this be RCU?
>
> AFAICT from looking at later patches the context switch path can do RMID
> borrowing/stealing whatever things which need protection of the package
> data. Other pathes (setup/rotation/ ...) take all the package locks to
> prevent concurrent RMID operations.

That's right. When a pmonr makes a state transition during context
switch, it needs to modify itself and potentially other pmonrs
because:
  - a pmonr in DEP_IDLE or DEP_DIRTY state will update its sched_rmid
if its lowest ancestor in Active state changes. This pmonrs "borrow"
the rmid.
  - a pmonr entering Active state must collect references to all
pmonrs that "borrow" rmid from it (all pmonrs that borrow rmids from
it).

All this transitions only affect data in pmonrs of a given package and
for that, they are protected by pkgd->lock. A rcu here is complicated
because many pmonrs must be changed atomically.

During pmonr state transition, monrs are not changed, only read,
because they embed the hierarchical relationship that pmonrs use to
find ancestors and descendants. For this reason, v3 of the series
requires to acquire pkgd->lock in all packages prior to any change to
the monrs hierarchical relationship.

The alternative proposes to protect pmonrs changes (that read the monr
hierarchy) with read_lock(&monr_hrchy_rwlock)  and pkgd->lock  . And
changes to the monr hierarchy with write_lock(&monr_hrchy_rwlock).

> I still have not figured out why all of this is necessary and unfortunately
> there is no real coherent epxlanation of the overall design. The cover
> letter is not really helpful either.

Note taken. I'll work on that.

>
> Thanks,
>
>         tglx