-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,240 @@ | ||
# Copyright 2024 ARM Limited | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import logging | ||
import devlib.utils.asyn as asyn | ||
from devlib.module import Module | ||
from devlib.utils.misc import ranges_to_list | ||
|
||
class Irq(object): | ||
def __init__(self, target, intid, data_dict, sysfs_root, procfs_root): | ||
self.target = target | ||
self.intid = intid | ||
self.sysfs_path = self.target.path.join(sysfs_root, str(intid)) | ||
self.procfs_path = self.target.path.join(procfs_root, str(intid)) | ||
|
||
self.irq_info = self._fix_data_dict(data_dict.copy()) | ||
|
||
def _fix_data_dict(self, data_dict): | ||
clean_dict = data_dict.copy() | ||
|
||
self._fix_sysfs_data(clean_dict) | ||
self._fix_procfs_data(clean_dict) | ||
|
||
return clean_dict | ||
|
||
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 commentThe 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,
|
||
clean_dict['hwirq'] = -1 | ||
else: | ||
clean_dict['hwirq'] = int(clean_dict['hwirq']) | ||
|
||
if 'per_cpu_count' in clean_dict: | ||
del clean_dict['per_cpu_count'] | ||
|
||
if 'name' not in clean_dict: | ||
clean_dict['name'] = '' | ||
|
||
if 'actions' not in clean_dict: | ||
clean_dict['actions'] = '' | ||
else: | ||
alist = clean_dict['actions'].split(',') | ||
if alist[0] == '(null)': | ||
alist = [] | ||
clean_dict['actions'] = alist | ||
|
||
def _fix_procfs_data(self, clean_dict): | ||
|
||
if 'spurious' not in clean_dict: | ||
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 commentThe 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:
However this still has some issues:
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 commentThe reason will be displayed to describe this comment to others. Learn more. s/clean_dict[alist] == '':/not clean_dict[alist]/ |
||
clean_dict[alist] = [] | ||
continue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
clean_dict[alist] = ranges_to_list(clean_dict[alist]) | ||
|
||
@property | ||
def actions(self): | ||
return self.irq_info['actions'] | ||
|
||
@property | ||
def chip_name(self): | ||
return self.irq_info['chip_name'] | ||
|
||
@property | ||
def hwirq(self): | ||
return self.irq_info['hwirq'] | ||
|
||
@property | ||
def name(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was set with:
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 |
||
return None if self.irq_info['name'] == '' else self.irq_info['name'] | ||
|
||
@property | ||
def type(self): | ||
return self.irq_info['type'] | ||
|
||
@property | ||
def wakeup(self): | ||
return self.irq_info['wakeup'] | ||
|
||
@property | ||
def smp_affinity(self): | ||
if 'smp_affinity' in self.irq_info.keys(): | ||
return self.irq_info['smp_affinity'] | ||
return -1 | ||
Comment on lines
+100
to
+102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. antipattern, use exception handling for this sort of thing, and in this specific circumstance dict.get() |
||
|
||
@smp_affinity.setter | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want to create a setter ? That will solidify in the API that we make the object mutable somehow, and making this via a property also means you forgo all future extension requiring parameters to be passed to the setter. Even if you have the So in this situation, I'd definitely opt for a simple |
||
def smp_affinity(self, affinity, verify=True): | ||
aff = str(affinity) | ||
aff_path = self.target.path.join(self.procfs_path, 'smp_affinity') | ||
self.target.write_value(aff_path, aff, verify=verify) | ||
|
||
self.update_affinities() | ||
|
||
@property | ||
def effective_affinity(self): | ||
if 'effective_affinity' in self.irq_info.keys(): | ||
return self.irq_info['effective_affinity'] | ||
return -1 | ||
|
||
def to_dict(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a real point to that considered irq_info is a public attribute ? If that whole class is fundamentally dict-like, maybe it should implement |
||
return self.irq_info.copy() | ||
|
||
@asyn.asyncf | ||
async def update_affinities(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd really avoid exposing an API that solidifies mutability, especially considered it's only used as part of the implementation of another function. It's fine to keep the code as-is, but that should be a private method so we can change it at any point. |
||
"""Read affinity masks from target.""" | ||
proc_data = await self.target.read_tree_values.asyn(self.procfs_path, depth=2, check_exit_code=False) | ||
self._fix_procfs_data(proc_data) | ||
|
||
for aff in ['smp_affinity', 'effective_affinity', 'smp_affinity_list', 'effective_affinity_list']: | ||
self.irq_info[aff] = proc_data[aff] | ||
|
||
class IrqModule(Module): | ||
name = 'irq' | ||
irq_sysfs_root = '/sys/kernel/irq/' | ||
irq_procfs_root = '/proc/irq/' | ||
Comment on lines
+132
to
+133
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these are constants and as such should be in upper case |
||
|
||
@staticmethod | ||
def probe(target): | ||
if target.file_exists(IrqModule.irq_sysfs_root): | ||
if target.file_exists(IrqModule.irq_procfs_root): | ||
return True | ||
|
||
def __init__(self, target): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this must call super().init() with the appropriate parameters and remove the redundant part in this |
||
self.logger = logging.getLogger(self.name) | ||
self.logger.debug(f'Initialized {self.name} module') | ||
|
||
self.target = target | ||
self.irqs = {} | ||
|
||
temp_dict = self._scrape_data(self.target, self.irq_sysfs_root, self.irq_procfs_root) | ||
for irq, data in temp_dict.items(): | ||
intid = int(irq) | ||
self.irqs[intid] = Irq(self.target, intid, data, self.irq_sysfs_root, self.irq_procfs_root) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use keyword arguments when building Irq, the risk of swapping parameters is pretty high here as lots of them have the same type. |
||
|
||
@asyn.asyncf | ||
@staticmethod | ||
async def _scrape_data(cls, target, sysfs_path=None, procfs_path=None): | ||
if sysfs_path and procfs_path: | ||
sysfs_dict = await target.read_tree_values.asyn(sysfs_path, depth=2, check_exit_code=False) | ||
procfs_dict = await target.read_tree_values.asyn(procfs_path, depth=2, check_exit_code=False) | ||
|
||
for irq, data in sysfs_dict.items(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mutation of a dict while iterating over it is a bad idea and can result in exceptions in some circumstances. In the current situation, I'd just rebuild a new dict and use it to update the existing one:
|
||
if irq in procfs_dict.keys(): | ||
sysfs_dict[irq] = {**data, **procfs_dict[irq]} | ||
Comment on lines
+161
to
+162
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. antipattern, use procfs_dict.items() |
||
return sysfs_dict | ||
|
||
if sysfs_path: | ||
sysfs_dict = await target.read_tree_values.asyn(sysfs_path, depth=2, check_exit_code=False) | ||
return sysfs_dict | ||
if procfs_path: | ||
procfs_dict = await target.read_tree_values.asyn(procfs_path, depth=1, check_exit_code=False) | ||
return procfs_dict | ||
|
||
return None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bad idea. From my other comment:
So all I had to do was to find where the issue was. This method is used in a single place currently, and as expected you tripped yourself :)
The
|
||
|
||
|
||
def get_all_irqs(self): | ||
"""Returns list of all interrupt IDs (list of integers).""" | ||
return list(self.irqs.keys()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since Python 3.7 (actually CPython 3.6), dicts preserve insertion order. However, this can still vary and generally speaking, it's a better idea to return |
||
|
||
def get_all_wakeup_irqs(self): | ||
"""Returns list of all wakeup-enabled interrupt IDs (list of integers).""" | ||
return [irq.intid for intid, irq in self.irqs.items() if irq.wakeup == 1] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto, could use sorted() |
||
|
||
@asyn.asyncf | ||
async def get_raw_stats(self): | ||
"""Return raw interrupt stats from procfs on target.""" | ||
raw_stats = await self.target.read_value.asyn('/proc/interrupts') | ||
return raw_stats | ||
|
||
@asyn.asyncf | ||
async def get_stats_dict(self): | ||
"""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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. closing bracket should be indented to match the current level, so one less level
Comment on lines
+196
to
+202
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. |
||
|
||
d_ipi = { | ||
'id' : [], | ||
'purpose' : [], | ||
} | ||
|
||
for cpu in range(0, nr_cpus): | ||
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 commentThe 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
|
||
intid, data = line.split(':', 1) | ||
data = data.split() | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
if 'IPI' in intid: | ||
d_ipi['id'].append(int(intid[3:])) | ||
|
||
for cpu in range(0, nr_cpus): | ||
d_ipi[f'cpu{cpu}'].append(int(data[cpu])) | ||
|
||
d_ipi['purpose'].append(' '.join(data[nr_cpus:])) | ||
else: | ||
d_irq['intid'].append(int(intid)) | ||
d_irq['chip_name'].append(data[nr_cpus]) | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. This does not seem particularly future-proof
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. |
||
d_irq['hwirq'].append(None) | ||
d_irq['type'].append(data[nr_cpus+1]) | ||
d_irq['actions'].append(data[nr_cpus+2:]) | ||
else: | ||
d_irq['hwirq'].append(int(data[nr_cpus+1])) | ||
d_irq['type'].append(data[nr_cpus+2]) | ||
d_irq['actions'].append(data[nr_cpus+3:]) | ||
|
||
return {'irq' : d_irq, 'ipi' : d_ipi} |
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.
At this point in time, pathlib.Path can finallly be used for pretty much anything in the standard library and is generally much nicer to manipulate than strings. @marcbonnici how would you feel if new APIs start exposing pathlib.Path objects ? In time, we should be able to end up with most of the code using pathlib.Path, and in the interim, user code can trivially call str() on it to get back a string if they want to stay in str-land when using more recent APIs.