Re: [PATCH v2 10/32] perf/x86/intel/cqm: introduce (I)state and limbo prmids

From: Thomas Gleixner
Date: Wed May 18 2016 - 16:38:09 EST


On Wed, 11 May 2016, David Carrillo-Cisneros wrote:
> static inline bool __pmonr__in_astate(struct pmonr *pmonr)
> {
> lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
> - return pmonr->prmid;
> + return pmonr->prmid && !pmonr->ancestor_pmonr;
> }
>
> static inline bool __pmonr__in_ustate(struct pmonr *pmonr)
> {
> lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
> - return !pmonr->prmid;
> + return !pmonr->prmid && !pmonr->ancestor_pmonr;
> +}
> +
> +static inline bool __pmonr__in_istate(struct pmonr *pmonr)
> +{
> + lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
> + return pmonr->ancestor_pmonr;
> +}
> +
> +static inline bool __pmonr__in_ilstate(struct pmonr *pmonr)
> +{
> + lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
> + return __pmonr__in_istate(pmonr) && pmonr->limbo_prmid;
> +}

So this does lockdep asserts several times in a row. And looking at the call
sites you do that a dozen times in the same function on the same pmonr.

Throwing enough asserts into the code makes it better, right?

> +static inline bool __pmonr__in_instate(struct pmonr *pmonr)
> +{
> + lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
> + return __pmonr__in_istate(pmonr) && !__pmonr__in_ilstate(pmonr);
> }

This state tracking sucks. It's completely non obvious which combinations of
members are denoting a certain state.

What's wrong with having:

pmonr->state

and a enum

enum pmonr_state {
PMONR_UNUSED,
PMONR_ACTIVE,
PMONR_LIMBO,
PMONR_INHERITED,
};

That would make all this horror readable and understandable. I bet you can't
remember the meaning of all this state stuff 3 month from now. That's going to
be the hell of a ride to track down a problem in this code.

> static inline bool monr__is_root(struct monr *monr)
> @@ -191,9 +209,12 @@ static int pkg_data_init_cpu(int cpu)
>
> INIT_LIST_HEAD(&pkg_data->free_prmids_pool);
> INIT_LIST_HEAD(&pkg_data->active_prmids_pool);
> + INIT_LIST_HEAD(&pkg_data->pmonr_limbo_prmids_pool);
> INIT_LIST_HEAD(&pkg_data->nopmonr_limbo_prmids_pool);
>
> INIT_LIST_HEAD(&pkg_data->astate_pmonrs_lru);
> + INIT_LIST_HEAD(&pkg_data->istate_pmonrs_lru);
> + INIT_LIST_HEAD(&pkg_data->ilstate_pmonrs_lru);
>
> mutex_init(&pkg_data->pkg_data_mutex);
> raw_spin_lock_init(&pkg_data->pkg_data_lock);
> @@ -242,7 +263,15 @@ static struct pmonr *pmonr_alloc(int cpu)
> if (!pmonr)
> return ERR_PTR(-ENOMEM);
>
> + pmonr->ancestor_pmonr = NULL;
> +
> + /*
> + * Since (A)state and (I)state have union in members,
> + * initialize one of them only.

What one of them? list heads or some other random stuff?

> + */
> + INIT_LIST_HEAD(&pmonr->pmonr_deps_head);
> pmonr->prmid = NULL;
> + INIT_LIST_HEAD(&pmonr->limbo_rotation_entry);
>
> pmonr->monr = NULL;
> INIT_LIST_HEAD(&pmonr->rotation_entry);
> @@ -308,6 +337,44 @@ __pmonr__finish_to_astate(struct pmonr *pmonr, struct prmid *prmid)
> atomic64_set(&pmonr->prmid_summary_atomic, summary.value);
> }
>
> +/*
> + * Transition to (A)state from (IN)state, given a valid prmid.
> + * Cannot fail. Updates ancestor dependants to use this pmonr as new ancestor.
> + */
> +static inline void
> +__pmonr__instate_to_astate(struct pmonr *pmonr, struct prmid *prmid)

And this function is used where?

> +{
> + struct pmonr *pos, *tmp, *ancestor;
> + union prmid_summary old_summary, summary;
> +
> + lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
> +
> + /* If in (I) state, cannot have limbo_prmid, otherwise prmid
> + * in function's argument is superfluous.
> + */
> + WARN_ON_ONCE(pmonr->limbo_prmid);

And what happens if that WARN_ON triggers? Can the code below be executed
without damaging anything? I'm just asking because so far your WARN_ONs have
been superfluous in most of the places.

> + /* Do not depend on ancestor_pmonr anymore. Make it (A)state. */
> + ancestor = pmonr->ancestor_pmonr;
> + list_del_init(&pmonr->pmonr_deps_entry);
> + pmonr->ancestor_pmonr = NULL;
> + __pmonr__finish_to_astate(pmonr, prmid);
> +
> + /* Update ex ancestor's dependants that are pmonr descendants. */
> + list_for_each_entry_safe(pos, tmp, &ancestor->pmonr_deps_head,
> + pmonr_deps_entry) {
> + if (!__monr_hrchy_is_ancestor(monr_hrchy_root,
> + pmonr->monr, pos->monr))
> + continue;
> + list_move_tail(&pos->pmonr_deps_entry, &pmonr->pmonr_deps_head);
> + pos->ancestor_pmonr = pmonr;
> + old_summary.value = atomic64_read(&pos->prmid_summary_atomic);
> + summary.sched_rmid = prmid->rmid;
> + summary.read_rmid = old_summary.read_rmid;
> + atomic64_set(&pos->prmid_summary_atomic, summary.value);
> + }
> +}
> +
> static inline void
> __pmonr__ustate_to_astate(struct pmonr *pmonr, struct prmid *prmid)
> {
> @@ -315,9 +382,59 @@ __pmonr__ustate_to_astate(struct pmonr *pmonr, struct prmid *prmid)
> __pmonr__finish_to_astate(pmonr, prmid);
> }
>
> +/*
> + * Find lowest active ancestor.
> + * Always successful since monr_hrchy_root is always in (A)state.
> + */
> +static struct monr *
> +__monr_hrchy__find_laa(struct monr *monr, u16 pkg_id)

Just waiting for the next function to be named __monr_hrchy__find_loo()

> +{
> + lockdep_assert_held(&cqm_pkgs_data[pkg_id]->pkg_data_lock);
> +
> + while ((monr = monr->parent)) {
> + if (__pmonr__in_astate(monr->pmonrs[pkg_id]))
> + return monr;
> + }
> + /* Should have hitted monr_hrchy_root */
> + WARN_ON_ONCE(true);

So you have a WARN_ON here and you have one at the call site checking the
return value.

> + return NULL;
> +}
> +
> +/*
> + * __pmnor__move_dependants: Move dependants from one ancestor to another.
> + * @old: Old ancestor.
> + * @new: New ancestor.
> + *
> + * To be called on valid pmonrs. @new must be ancestor of @old.
> + */
> +static inline void
> +__pmonr__move_dependants(struct pmonr *old, struct pmonr *new)
> +{
> + struct pmonr *dep;
> + union prmid_summary old_summary, summary;
> +
> + WARN_ON_ONCE(old->pkg_id != new->pkg_id);

Great, another warning which will lead to explosions later on simply because
you access @new unlocked.

> + lockdep_assert_held(&__pkg_data(old, pkg_data_lock));
> +
> + /* Update this pmonr dependencies to use new ancestor. */
> + list_for_each_entry(dep, &old->pmonr_deps_head, pmonr_deps_entry) {
> + /* Set next summary for dependent pmonrs. */
> + dep->ancestor_pmonr = new;
> +
> + old_summary.value = atomic64_read(&dep->prmid_summary_atomic);
> + summary.sched_rmid = new->prmid->rmid;
> + summary.read_rmid = old_summary.read_rmid;
> + atomic64_set(&dep->prmid_summary_atomic, summary.value);
> + }
> + list_splice_tail_init(&old->pmonr_deps_head,
> + &new->pmonr_deps_head);
> +}
> +
> static inline void
> __pmonr__to_ustate(struct pmonr *pmonr)
> {
> + struct pmonr *ancestor;
> + u16 pkg_id = pmonr->pkg_id;
> union prmid_summary summary;
>
> lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
> @@ -331,9 +448,27 @@ __pmonr__to_ustate(struct pmonr *pmonr)
> if (__pmonr__in_astate(pmonr)) {
> WARN_ON_ONCE(!pmonr->prmid);
>
> + ancestor = __monr_hrchy__find_laa(
> + pmonr->monr, pkg_id)->pmonrs[pkg_id];

Using intermediat variables instead of these line breaks would make reading
this way simpler.

> + WARN_ON_ONCE(!ancestor);

Brilliant error handling. But at least consistently useless as the one 5 lines
above.

> + __pmonr__move_dependants(pmonr, ancestor);
> list_move_tail(&pmonr->prmid->pool_entry,
> &__pkg_data(pmonr, nopmonr_limbo_prmids_pool));
> pmonr->prmid = NULL;
> + } else if (__pmonr__in_istate(pmonr)) {
> + list_del_init(&pmonr->pmonr_deps_entry);
> + /* limbo_prmid is already in limbo pool */
> + if (__pmonr__in_ilstate(pmonr)) {
> + WARN_ON(!pmonr->limbo_prmid);

Sigh.

> + list_move_tail(
> + &pmonr->limbo_prmid->pool_entry,
> + &__pkg_data(pmonr, nopmonr_limbo_prmids_pool));

I really appreciate all the useful comments in that code. This shuffles stuff
around from one pool to the next w/o any explanation whatsoever.

And just for the record. I just had to look up __pkg_data() once again, which
is annoying enough.

You do:

cqm_pkgs_data[pmonr->pkg_id]->xxxx

over and over. Instead of that you can simply store the pointer to the pkg
data in pmonr and do:

pmonr->pdata->xxxx

Hmm?

> +
> + pmonr->limbo_prmid = NULL;
> + list_del_init(&pmonr->limbo_rotation_entry);
> + } else {
> + }
> + pmonr->ancestor_pmonr = NULL;
> } else {
> WARN_ON_ONCE(true);
> return;
> @@ -348,6 +483,62 @@ __pmonr__to_ustate(struct pmonr *pmonr)
> WARN_ON_ONCE(!__pmonr__in_ustate(pmonr));
> }
>
> +static inline void __pmonr__set_istate_summary(struct pmonr *pmonr)
> +{
> + union prmid_summary summary;
> +
> + summary.sched_rmid = pmonr->ancestor_pmonr->prmid->rmid;
> + summary.read_rmid =
> + pmonr->limbo_prmid ? pmonr->limbo_prmid->rmid : INVALID_RMID;
> + atomic64_set(
> + &pmonr->prmid_summary_atomic, summary.value);

Your name choices are interesting. You either chose cryptic names which cannot
be decoded or pointless long descriptive names which contain redundant
information.

pmonr->prmid_summary

Should be enough, right? The compiler will tell you if you access that
directly.

> +/*
> + * Transition to (I)state from no (I)state..
> + * Finds a valid ancestor transversing monr_hrchy. Cannot fail.
> + */
> +static inline void
> +__pmonr__to_istate(struct pmonr *pmonr)
> +{
> + struct pmonr *ancestor;
> + u16 pkg_id = pmonr->pkg_id;
> +
> + lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
> +
> + if (!(__pmonr__in_ustate(pmonr) || __pmonr__in_astate(pmonr))) {
> + /* Invalid initial state. */
> + WARN_ON_ONCE(true);

According to the comment above it cannot fail. What's this?

> + return;
> + }
> +
> + ancestor = __monr_hrchy__find_laa(pmonr->monr, pkg_id)->pmonrs[pkg_id];
> + WARN_ON_ONCE(!ancestor);

Lalalalala.

> +/* A pmonr in (I)state (either (IN)state or (IL)state. */
> +inline bool prmid_summary__is_istate(union prmid_summary summ)
> +{
> + return summ.sched_rmid != INVALID_RMID &&
> + summ.sched_rmid != summ.read_rmid;
> +}

Yet more undecodable state tracking.

> struct pmonr {
>
> - struct prmid *prmid;
> + /* If set, pmonr is in (I)state. */
> + struct pmonr *ancestor_pmonr;
> +
> + union{
> + struct { /* (A)state variables. */
> + struct list_head pmonr_deps_head;
> + struct prmid *prmid;
> + };
> + struct { /* (I)state variables. */
> + struct list_head pmonr_deps_entry;
> + struct prmid *limbo_prmid;
> + struct list_head limbo_rotation_entry;

What's the value of this union? I can't see one, really. Please explain.

Thanks,

tglx