lkml.org 
[lkml]   [2016]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2016-05-25 03:01    [W:0.141 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site