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