module/cpufreq: Fix async use_governor()

use_governor() was trying to set concurrently both per-cpu and global tunables for
each governor, which lead to a write conflict.

Split the work into the per-governor global tunables and the per-cpu
tunables, and do all that in concurrently. Each task is therefore
responsible of a distinct set of files and all is well.

Also remove @memoized on async functions. It will be reintroduced in a
later commit when there is a safe alternative for async functions.

Author: douglas-raillard-arm

devlib/module/cpufreq.py

@@ -58,7 +58,6 @@ def __init__(self, target):
         super(CpufreqModule, self).__init__(target)
         self._governor_tunables = {}
 
-    @memoized
     @asyn.asyncf
     async def list_governors(self, cpu):
         """Returns a list of governors supported by the cpu."""
@@ -105,7 +104,7 @@ async def set_governor(self, cpu, governor, **kwargs):
             raise TargetStableError('Governor {} not supported for cpu {}'.format(governor, cpu))
         sysfile = '/sys/devices/system/cpu/{}/cpufreq/scaling_governor'.format(cpu)
         await self.target.write_value.asyn(sysfile, governor)
-        return await self.set_governor_tunables.asyn(cpu, governor, **kwargs)
+        await self.set_governor_tunables.asyn(cpu, governor, **kwargs)
 
     @asyn.asynccontextmanager
     async def use_governor(self, governor, cpus=None, **kwargs):
@@ -151,35 +150,74 @@ async def get_cpu_info(cpu):
         try:
             yield
         finally:
-            async def set_gov(cpu):
+            async def set_per_cpu_tunables(cpu):
                 domain, prev_gov, tunables, freq = cpus_infos[cpu]
-                await self.set_governor.asyn(cpu, prev_gov, **tunables)
+                # Per-cpu tunables are safe to set concurrently
+                await self.set_governor_tunables.asyn(cpu, prev_gov, per_cpu=True, **tunables)
                 # Special case for userspace, frequency is not seen as a tunable
                 if prev_gov == "userspace":
                     await self.set_frequency.asyn(cpu, freq)
 
+            per_cpu_tunables = self.target.async_manager.concurrently(
+                set_per_cpu_tunables(cpu)
+                for cpu in domains
+            )
+
+            # Non-per-cpu tunables have to be set one after the other, for each
+            # governor that we had to deal with.
+            global_tunables = {
+                prev_gov: (cpu, tunables)
+                for cpu, (domain, prev_gov, tunables, freq) in cpus_infos.items()
+            }
+
+            global_tunables = self.target.async_manager.concurrently(
+                self.set_governor_tunables.asyn(cpu, gov, per_cpu=False, **tunables)
+                for gov, (cpu, tunables) in global_tunables.items()
+            )
+
+            # Set the governor first
             await self.target.async_manager.concurrently(
-                set_gov(cpu)
+                self.set_governor.asyn(cpu, cpufs_infos[cpu][1])
                 for cpu in domains
             )
+            # And then set all the tunables concurrently. Each task has a
+            # specific and non-overlapping set of file to write.
+            await self.target.async_manager.concurrently(
+                (per_cpu_tunables, global_tunables)
+            )
 
     @asyn.asyncf
-    async def list_governor_tunables(self, cpu):
-        """Returns a list of tunables available for the governor on the specified CPU."""
+    async def _list_governor_tunables(self, cpu):
         if isinstance(cpu, int):
             cpu = 'cpu{}'.format(cpu)
         governor = await self.get_governor.asyn(cpu)
-        if governor not in self._governor_tunables:
-            try:
-                tunables_path = '/sys/devices/system/cpu/{}/cpufreq/{}'.format(cpu, governor)
-                self._governor_tunables[governor] = await self.target.list_directory.asyn(tunables_path)
-            except TargetStableError:  # probably an older kernel
+
+        try:
+            return self._governor_tunables[governor]
+        except KeyError:
+            for per_cpu, path in (
+                (True, '/sys/devices/system/cpu/{}/cpufreq/{}'.format(cpu, governor)),
+                # On old kernels
+                (False, '/sys/devices/system/cpu/cpufreq/{}'.format(governor)),
+            ):
                 try:
-                    tunables_path = '/sys/devices/system/cpu/cpufreq/{}'.format(governor)
-                    self._governor_tunables[governor] = await self.target.list_directory.asyn(tunables_path)
-                except TargetStableError:  # governor does not support tunables
-                    self._governor_tunables[governor] = []
-        return self._governor_tunables[governor]
+                    tunables = await self.target.list_directory.asyn(tunables_path)
+                except TargetStableError:
+                    continue
+                else:
+                    break
+            else:
+                per_cpu = False
+                tunables = []
+
+            self._governor_tunables[governor] = (per_cpu, tunables)
+            return (per_cpu, tunables)
+
+    @asyn.asyncf
+    async def list_governor_tunables(self, cpu):
+        """Returns a list of tunables available for the governor on the specified CPU."""
+        _, tunables = await self._list_governor_tunables.asyn(cpu)
+        return tunables
 
     @asyn.asyncf
     async def get_governor_tunables(self, cpu):
@@ -211,7 +249,7 @@ async def get_tunable(tunable):
         return tunables
 
     @asyn.asyncf
-    async def set_governor_tunables(self, cpu, governor=None, **kwargs):
+    async def set_governor_tunables(self, cpu, governor=None, per_cpu=None, **kwargs):
         """
         Set tunables for the specified governor. Tunables should be specified as
         keyword arguments. Which tunables and values are valid depends on the
@@ -220,6 +258,9 @@ async def set_governor_tunables(self, cpu, governor=None, **kwargs):
         :param cpu: The cpu for which the governor will be set. ``int`` or
                     full cpu name as it appears in sysfs, e.g. ``cpu0``.
         :param governor: The name of the governor. Must be all lower case.
+        :param per_cpu: If ``None``, both per-cpu and global governor tunables
+            will be set. If ``True``, only per-CPU tunables will be set and if
+            ``False``, only global tunables will be set.
 
         The rest should be keyword parameters mapping tunable name onto the value to
         be set for it.
@@ -229,29 +270,29 @@ async def set_governor_tunables(self, cpu, governor=None, **kwargs):
                  tunable.
 
         """
+        if not kwargs:
+            return
         if isinstance(cpu, int):
             cpu = 'cpu{}'.format(cpu)
         if governor is None:
             governor = await self.get_governor.asyn(cpu)
-        valid_tunables = await self.list_governor_tunables.asyn(cpu)
+        gov_per_cpu, valid_tunables = await self._list_governor_tunables.asyn(cpu)
         for tunable, value in kwargs.items():
             if tunable in valid_tunables:
-                path = '/sys/devices/system/cpu/{}/cpufreq/{}/{}'.format(cpu, governor, tunable)
-                try:
-                    await self.target.write_value.asyn(path, value)
-                except TargetStableError:
-                    if await self.target.file_exists.asyn(path):
-                        # File exists but we did something wrong
-                        raise
-                    # Expected file doesn't exist, try older sysfs layout.
+                if per_cpu is not None and gov_per_cpu != per_cpu:
+                    pass
+
+                if gov_per_cpu:
+                    path = '/sys/devices/system/cpu/{}/cpufreq/{}/{}'.format(cpu, governor, tunable)
+                else:
                     path = '/sys/devices/system/cpu/cpufreq/{}/{}'.format(governor, tunable)
-                    await self.target.write_value.asyn(path, value)
+
+                await self.target.write_value.asyn(path, value)
             else:
                 message = 'Unexpected tunable {} for governor {} on {}.\n'.format(tunable, governor, cpu)
                 message += 'Available tunables are: {}'.format(valid_tunables)
                 raise TargetStableError(message)
 
-    @memoized
     @asyn.asyncf
     async def list_frequencies(self, cpu):
         """Returns a sorted list of frequencies supported by the cpu or an empty list