-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
devlib/module: Add irq module for stats and affinity manipulation #669
base: master
Are you sure you want to change the base?
Conversation
FEATURE Add module to collect irq configuration, stats, and affinities from target. Enables setting of affinity.
e89bb70
to
019b4ac
Compare
def _fix_sysfs_data(self, clean_dict): | ||
clean_dict['wakeup'] = 0 if clean_dict['wakeup'] == 'disabled' else 1 | ||
|
||
if 'hwirq' not in clean_dict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, the "try and see if it fails" is a better idea than "ask for permission". In this context,
try:
x = clean_dict['hwirq']
except KeyError:
clean_dict['hwirq'] = -1
else:
clean_dict['hwirq'] = int(x)
clean_dict['spurious'] = '' | ||
else: | ||
temp = clean_dict['spurious'].split('\n') | ||
clean_dict['spurious'] = dict([[i.split(' ')[0], i.split(' ')[1]] for i in temp]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could do without an unneeded list comprehension and just replace the dict(iterable) pattern by a dict comprehension:
{
i.split(' ')[0]: i.split(' ')[1]
for i in temp
}
However this still has some issues:
- split() is called twice
- what if there are more than 2 items in the split result ? Should this fail ? Should this only split on the first space encountered and treat the rest as a single item (
i.split(' ', 1)
) ?
For 1., this can be solved with the walrus operator, available from Python 3.8. This would require bumping the requirement of devlib (see setup.py) but 3.7 has already reached EOL in June 2023 so it's probably fine. Also even Ubuntu 20.04 has 3.8, so I don't think dropping support for 3.7 is going to affect anyone at this point.
|
||
for alist in ['smp_affinity_list', 'effective_affinity_list']: | ||
if alist in clean_dict: | ||
if clean_dict[alist] == '': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/clean_dict[alist] == '':/not clean_dict[alist]/
if alist in clean_dict: | ||
if clean_dict[alist] == '': | ||
clean_dict[alist] = [] | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd advise against using continue this way as this can really easily backfire down the line when someone else adds extra processing in the loop body. Especially in this case where else:
can be used, and not make the indentation deeper than it is currently
return self.irq_info['hwirq'] | ||
|
||
@property | ||
def name(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was set with:
if 'name' not in clean_dict:
clean_dict['name'] = ''
Both combined together form a nice anti pattern: if there is no name, there is no name, no need to invent one (empty string). Especially if the matching property turns "empty string" into None
. Either the whole system "invents" a None name upon init if there is no name already, or the name
key is not added. I'd personally strongly favor the not having that key if there is no value for it, as this will allow downstream code to rely on the fact that the type is str. If the downstream code desperately needs a value, they can trivially provide one with irq_info.get('name', <default>)
, or just irq_info.get('name')
if default they want is None. In my experience, dropping a None value where something else is expected always result in broken code that passes tests and one day explodes in the wild under a different use case.
Note that this would make the name
property naturally raise an exception if there is no name. You may want to re-raise an AttributeError instead of the KeyError. Otherwise, some mechanism around __getattr__
will not work as expected. You can build a message that explains why the exception is raised however, so that a user knows it's because of the lack of name rather than e.g. a typo on their end.
d_irq[f'cpu{cpu}'] = [] | ||
d_ipi[f'cpu{cpu}'] = [] | ||
|
||
for line in self.target.irq.get_raw_stats().splitlines()[1:-1]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is excluding first and last lines really what we want ? The first row seems hard to detect as a "to be ignored" format (e.g. no leading #
char), but the last row seems to be an actual interrupt to me:
PIW: 0 0 0 0 0 0 0 0 Posted-interrupt wakeup event
for line in self.target.irq.get_raw_stats().splitlines()[1:-1]: | ||
intid, data = line.split(':', 1) | ||
data = data.split() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experience, the code is more extendable and readable if structured to build one d_ipi
value at a time, rather than directing the "search" from the line iterations. This will avoid much of the stateful append business and make it easier to find out exactly what goes into each key/value without having to figure out the interaction with all the other values.
"""Returns dict of dicts of irq and IPI stats.""" | ||
raw_stats = await self.get_raw_stats.asyn() | ||
|
||
nr_cpus = self.target.number_of_cpus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bad idea to use a source of information X to drive traversal of some Y information. Any bug related to that will break this code in a subtle way (and there can be many, "number of cpus" is actually very ill-defined, and we had issues in the past in trace-cmd due to different interpretation of the idea in muscl vs glibc).
The information is already present in the data, extract it and use that. That will also be much easier to use than having to defensively assume the other source of info mismatches our current needs (which is required if you are going to use it).
d_irq = { | ||
'intid' : [], | ||
'chip_name' : [], | ||
'hwirq' : [], | ||
'type' : [], | ||
'actions' : [], | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIU this dict is basically a list of columns where all the values at index i
form a record for a given interrupt. If so, that's a fertile recipe for bugs, especially in the code that builds it, and especially with all this conditional append() business (any issue in there means everything gets shifted by one, and the issue will be 100% silent unless the user needs the last row and then you'll get some IndexErrors).
On top of that, the usability would be pretty poor: in Python, you can iterate over an iterable, so having a list of tuples is much more convenient than a tuple of list. This is unlike C but basically like every other language.
In this specific situation, a namedtuple could be appropriate and make usage easier. A better alternative would be to create our own class to hold the info of a row. This will make future maintenance easier, as namedtuple has a number of guarantees and methods that might be harder to implement in a compatibility shim.
for cpu in range(0, nr_cpus): | ||
d_irq[f'cpu{cpu}'].append(int(data[cpu])) | ||
|
||
if 'Level' in data[nr_cpus+1] or 'Edge' in data[nr_cpus+1]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not seem particularly future-proof 'Level' in data[nr_cpus+1]
. That sort of "column assignation" logic should be handled separately from parsing each rows, and should definitely be driven by row 0 content:
CPU0 CPU1 CPU2 CPU3 CPU4 CPU5 CPU6 CPU7
The code that parses the line should have access to the pieces of the line by name, rather than having to craft an index like that everywhere.
FEATURE
Add module to collect irq configuration, stats, and affinities from target. Enables setting of affinity.