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

[Ingo Molnar]

* Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Thu, Nov 10, 2016 at 06:22:17PM -0800, David Carrillo-Cisneros wrote:
> > A possible alternative to nested spinlocks is to use a single rw_lock to
> > protect changes to the monr hierarchy.
> > 
> > This works because, when manipulating pmonrs, the monr hierarchy (monr's
> > parent and children) is read but never changed (reader path).
> > Changes to the monr hierarchy do change pmonrs (writer path).
> > 
> > It would be implemented like:
> > 
> >   // system-wide lock for monr hierarchy
> >   rwlock_t monr_hrchy_rwlock = __RW_LOCK_UNLOCKED(monr_hrchy_rwlock);
> > 
> > 
> > Reader Path: Access pmonrs in same pkgd. Used in pmonr state transitions
> > and when reading cgroups (read subtree of pmonrs in same package):
> > 
> > v3 (old):
> > 
> >   raw_spin_lock(&pkgd->lock);
> >   // read monr data, read/write pmonr data
> >   raw_spin_unlock(&pkgd->lock);
> > 
> > next version:
> > 
> >   read_lock(&monr_hrchy_rwlock);
> >   raw_spin_lock(&pkgd->lock);
> >   // read monr data, read/write pmonr data
> >   raw_spin_unlock(&pkgd->lock);
> >   read_unlock(&monr_hrchy_rwlock);
> > 
> > 
> > Writer Path: Modifies monr hierarchy (add/remove monr in
> > creation/destruction of events and start/stop monitoring
> > of cgroups):
> > 
> > v3 (old):
> > 
> >   monr_hrchy_acquire_locks(...); // acquires all pkg->lock
> >   // read/write monr data, potentially read/write pmonr data
> >   monr_hrchy_release_locks(...); // release all pkg->lock
> > 
> > v4:
> > 
> >   write_lock(&monr_hrchy_rwlock);
> >   // read/write monr data, potentially read/write pmonr data
> >   write_unlock(&monr_hrchy_rwlock);
> > 
> > 
> > The write-path should occur infrequently. The reader-path
> > is slower due to the additional read_lock(&monr_hrchy_rwlock).
> > The number of locks to acquire remains constant on the number of
> > packages in both paths. There would be no need to ever nest pkgd->lock's.
> > 
> > For v3, I discarded this idea because, due to the additional
> > read_lock(&monr_hrchy_rwlock), the pmonr state transitions that may
> > occur in a context switch would be slower than in the current version.
> > But it may be ultimately better considering the scalability issues
> > that you mention.
> 
> 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?
> 
> Also, "monr" is a horribly 'word'.

And 'hrchy' is a hrbbl wrd as wll!

Thanks,

	Ingo