Messages in this thread | | | Date | Wed, 25 May 2016 10:51:14 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH v2 10/32] perf/x86/intel/cqm: introduce (I)state and limbo prmids |
| |
On Tue, 24 May 2016, David Carrillo-Cisneros wrote: > >> +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
And exaclty that stuff drives me nuts. You update stuff here and there and then you infer the state from this.
> Redundantly storing the state in an extra variable opens the door to > bugs that updates pmonr::state inconsistently with the member above.
Well, your 'infer' state from three other variables is error prone as well and you can simply add a debug feature which makes sure that the variables are consistent.
validate_state(p) { switch (p->state) { case PMONR_UNUSED: WARN_ON(p->xxxx .....);
case PMONR_ACTIVE: WARN_ON(p->xxxx .....); } }
That's way better than relying on three variables which are updated here and there to reflect the proper state.
It's not only the 3 variables which are involved there. You also have lists and whatever which depend on this. So having a proper 'state' variable as the central anchor gives you the ability to verify the dependent contents of your other variables, lists etc.
Thanks,
tglx
| |