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

[David Carrillo-Cisneros]

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

In the pmonr, the state can be inferred by the values of:
  - pmonr->ancestor_pmonr
  - pmonr->prmid
  - pmonr->limbo_prmid

Redundantly storing the state in an extra variable opens the door to
bugs that updates pmonr::state inconsistently with the member above.

The functions __pmonr__in_*state (to be renamed to pmonr_in_*state)
are the single point where the pmonr members that constitute a state
are checked.

I can do a better job documenting the states. Should we try that first?

Thanks,
    David