Repository: ARM-software/devlib

Issue:
Stop using ftrace non-toplevel instance
ftrace allows collecting events in multiple user-defined instances. In trace-cmd, this is enabled by the `-B` switch.
Unfortunately, using non-toplevel instance has brought a number of problems:
1. Some events simply cannot be collected anywhere else, like bprint emitted by trace_printk(). This is "by design" in how ftrace work, so unlikely to ever get solved.
2. Other ftrace features such as synthetic events [do not play well with non-toplevel instance](https://bugzilla.kernel.org/show_bug.cgi?id=219876)

The motivation behind using instances in the first place was an attempt at isolating the impact of devlib collector so that other userspace tools could cooperate. Unfortunately, some ftrace tunables are not per-instance, and most other tools don't seem to be particularly good citizen when it comes to instances they don't own (last time we tried with Perfetto it did not allow trace-cmd running at the same time with success).

As a result, I think we should revert that choice and simply use the top-level instance. At the moment, the only ways this is exposed in devlib API are:
1. Generated traces have events in the "devlib" buffer. This would show up in `trace-cmd report` output, but I don't think anyone particularly depends on that. Even if they do, they would already have issues with bprint anyway.
2. FtraceCollector() has separate `buffer_size` and `top_buffer_size` parameters. We could simply aggregate both with `max()` and preserve the API as it is, so that we can re-introduce it in the future if the situation improves.

Hints from issue comments:
@douglas-raillard-arm: Fixed by https://github.com/ARM-software/devlib/pull/719

Solution patch:
diff --git a/devlib/collector/ftrace.py b/devlib/collector/ftrace.py
index 0aea8ee..d060f36 100644
--- a/devlib/collector/ftrace.py
+++ b/devlib/collector/ftrace.py
@@ -200,6 +200,19 @@ class FtraceCollector(CollectorBase):
 
         self._selected_events = selected_events
 
+    @property
+    def _buffer_size(self):
+        top = self.top_buffer_size
+        nontop = self.buffer_size
+        if top is None and nontop is None:
+            return None
+        elif top is None:
+            return nontop
+        elif nontop is None:
+            return top
+        else:
+            return max(top, nontop)
+
     @property
     def event_string(self):
         return _build_trace_events(self._selected_events)
@@ -254,19 +267,25 @@ class FtraceCollector(CollectorBase):
         except TargetStableError:
             kprobe_events = None
 
-        self.target.execute('{} reset -B devlib'.format(self.target_binary),
+        self.target.execute('{} reset'.format(self.target_binary),
                             as_root=True, timeout=TIMEOUT)
 
+
+        # This code is currently not necessary as we are not using alternate
+        # instances (not using -B parameter). If we end up using it again, it
+        # may very well be that trace-cmd at that point takes care of that
+        # problem itself somehow, so this should be re-evaluated.
+
         # trace-cmd start will not set the top-level buffer size if passed -B
         # parameter, but unfortunately some events still end up there (e.g.
         # print event). So we still need to set that size, otherwise the buffer
         # might be too small and some event lost.
-        top_buffer_size = self.top_buffer_size if self.top_buffer_size else self.buffer_size
-        if top_buffer_size:
-            self.target.write_value(
-                self.target.path.join(self.tracing_path, 'buffer_size_kb'),
-                top_buffer_size, verify=False
-            )
+        #  top_buffer_size = self.top_buffer_size or self.buffer_size
+        #  if top_buffer_size:
+            #  self.target.write_value(
+                #  self.target.path.join(self.tracing_path, 'buffer_size_kb'),
+                #  top_buffer_size, verify=False
+            #  )
 
         if self.functions:
             self.target.write_value(self.function_profile_file, 0, verify=False)
@@ -315,11 +334,11 @@ class FtraceCollector(CollectorBase):
         with contextlib.suppress(TargetStableError):
             self.target.write_value('/proc/sys/kernel/kptr_restrict', 0)
 
-        params = '-B devlib {buffer_size} {cmdlines_size} {clock} {events} {tracer} {functions}'.format(
+        params = '{buffer_size} {cmdlines_size} {clock} {events} {tracer} {functions}'.format(
             events=self.event_string,
             tracer=tracer_string,
             functions=tracecmd_functions,
-            buffer_size='-b {}'.format(self.buffer_size) if self.buffer_size is not None else '',
+            buffer_size='-b {}'.format(self._buffer_size) if self._buffer_size is not None else '',
             clock='-C {}'.format(self.trace_clock) if self.trace_clock else '',
             cmdlines_size='--cmdlines-size {}'.format(self.saved_cmdlines_nr) if self.saved_cmdlines_nr is not None else '',
         )
@@ -381,7 +400,7 @@ class FtraceCollector(CollectorBase):
             bg_cmd.communicate()
             bg_cmd.__exit__(None, None, None)
         elif mode == 'write-to-memory':
-            self.target.execute('{} stop -B devlib'.format(self.target_binary),
+            self.target.execute('{} stop'.format(self.target_binary),
                                 timeout=TIMEOUT, as_root=True)
         else:
             raise ValueError(f'Unknown mode {mode}')
@@ -404,7 +423,7 @@ class FtraceCollector(CollectorBase):
             # Interrupting trace-cmd record will make it create the file
             pass
         elif mode == 'write-to-memory':
-            cmd = f'{self.target_binary} extract -B devlib -o {self.target_output_file} && {busybox} chmod 666 {self.target_output_file}'
+            cmd = f'{self.target_binary} extract -o {self.target_output_file} && {busybox} chmod 666 {self.target_output_file}'
             self.target.execute(cmd, timeout=TIMEOUT, as_root=True)
         else:
             raise ValueError(f'Unknown mode {mode}')
diff --git a/devlib/utils/asyn.py b/devlib/utils/asyn.py
index dd6d42d..c4df021 100644
--- a/devlib/utils/asyn.py
+++ b/devlib/utils/asyn.py
@@ -123,6 +123,10 @@ class AsyncManager:
         except BaseException:
             for task in tasks:
                 task.cancel()
+                try:
+                    await task
+                except asyncio.CancelledError:
+                    pass
             raise
         finally:
 


Test patch:
diff --git a/tests/test_asyn.py b/tests/test_asyn.py
index 8fe9a22..cc11bea 100644
--- a/tests/test_asyn.py
+++ b/tests/test_asyn.py
@@ -23,7 +23,7 @@ from contextlib import contextmanager
 
 from pytest import skip, raises
 
-from devlib.utils.asyn import run, asynccontextmanager
+from devlib.utils.asyn import run, asynccontextmanager, AsyncManager
 
 
 class AsynTestExcep(Exception):
@@ -517,6 +517,53 @@ def _do_test_run(top_run):
 
     test_async_gen1()
 
+    def test_async_map_concurrently():
+        async def agen_f():
+            manager = AsyncManager()
+
+            async def f1():
+                return 1
+
+            async def f2():
+                return 2
+
+            return await manager.concurrently([f1(), f2()])
+
+        agen = agen_f()
+        assert top_run(agen) == [1, 2]
+
+    test_async_map_concurrently()
+
+    def test_async_map_concurrently_cancel():
+        class MyException(Exception):
+            pass
+
+        async def agen_f():
+            manager = AsyncManager()
+            cancelled1 = False
+
+            async def f1():
+                nonlocal cancelled1
+
+                try:
+                    # Await on a future that will never be available. We should
+                    # get canceled at some point, so it does not matter.
+                    await asyncio.Future()
+                except asyncio.CancelledError:
+                    cancelled1 = True
+
+            async def f2():
+                raise MyException('from f2')
+
+            with raises(MyException):
+                await manager.concurrently([f1(), f2()])
+
+            assert cancelled1
+
+        top_run(agen_f())
+
+    test_async_map_concurrently_cancel()
+
 
 def _test_in_thread(setup, test):
     def f():