Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
240 changes: 240 additions & 0 deletions devlib/module/irq.py
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))
Comment on lines +24 to +25
Copy link
Contributor

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.


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:
Copy link
Contributor

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['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])
Copy link
Contributor

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:

  1. split() is called twice
  2. 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] == '':
Copy link
Contributor

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]/

clean_dict[alist] = []
continue
Copy link
Contributor

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

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):
Copy link
Contributor

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.

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 verify=True default, this is basically impossible to change for the user, unless they know about it and use a terrible way of invoking it such as x.__class__.smp_affinity(x, affinity, verify=False).

So in this situation, I'd definitely opt for a simple set_smp_affinity() function, e.g. like it was done for cpufreq's set_frequency()

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):
Copy link
Contributor

Choose a reason for hiding this comment

The 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 collections.abc.Mapping instead, but if it's not its main purpose of existence, it's probably not a good idea.

return self.irq_info.copy()

@asyn.asyncf
async def update_affinities(self):
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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):
Copy link
Contributor

Choose a reason for hiding this comment

The 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 __init__ implementation

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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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():
Copy link
Contributor

Choose a reason for hiding this comment

The 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:

sysfs_dict.update({
    irq: {**sysfs_dict[irq], **procfs_data}
    for irq, procfs_data in procfs_dict.items()
})

if irq in procfs_dict.keys():
sysfs_dict[irq] = {**data, **procfs_dict[irq]}
Comment on lines +161 to +162
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad idea. From my other comment:

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.

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 :)

        temp_dict = self._scrape_data(self.target, self.irq_sysfs_root, self.irq_procfs_root)
        for irq, data in temp_dict.items():

The None-returning path will trigger a beautiful AttributeError: 'NoneType' object has no attribute 'items'. The only situation in which this can genuinely work fine are:

  1. very locally where all the involved code fits on a single screen and if actually simplifies the code (e.g. you produce data by mapping a None-returning closure to some list, then you filter-out the None part)
  2. not in Python, unless you use mypy and properly annotate. E.g. in Rust Option is fine as it's type-checked, so the client side is forced to handle None if they want the Some() value.



def get_all_irqs(self):
"""Returns list of all interrupt IDs (list of integers)."""
return list(self.irqs.keys())
Copy link
Contributor

Choose a reason for hiding this comment

The 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 sorted(self.irq.keys()) if the cost is small. This will prevent hard-to-reproduce bugs in client code, and make things like diffing logs much easier.


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]
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

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' : [],
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

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.


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]:
Copy link
Contributor

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

intid, data = line.split(':', 1)
data = data.split()

Copy link
Contributor

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.

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]:
Copy link
Contributor

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.

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}