Messages in this thread | | | From | David Carrillo-Cisneros <> | Date | Tue, 24 May 2016 17:52:03 -0700 | Subject | Re: [PATCH v2 10/32] perf/x86/intel/cqm: introduce (I)state and limbo prmids |
| |
>> +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
| |