[Bugfix][Runtime] Fix sched_setaffinity in Android #11599

sunjiweiswift · 2022-06-07T09:55:47Z

sched_setaffinity should use pid_t instread of pthread_t
https://man7.org/linux/man-pages/man2/sched_setaffinity.2.html

…t/tvm into set_affinity_android_bugfix

sunjiweiswift · 2022-06-08T02:01:45Z

areusch · 2022-06-09T19:39:25Z

i think @huajsj might have context around this. @sunjiweiswift could you update the PR title and body as well similar to the other PR?

areusch

@supersat or @Lunderberg maybe either of you wanna look at this too?

areusch · 2022-06-16T23:02:16Z

src/runtime/threading_backend.cc

@@ -164,9 +172,13 @@ class ThreadGroup::Impl {
      CPU_SET(id, &cpuset);
    }
 #if defined(__ANDROID__)
-    sched_setaffinity(thread, sizeof(cpu_set_t), &cpuset);
+    if (sched_setaffinity(tid, sizeof(cpu_set_t), &cpuset) == -1) {


per the man page we should check for != 0 here, correct?

The man page also specifies that it returns -1 on failure, but that's for glibc. I looked at the bionic (Android C runtime) sources, and it also returns -1 on failure. Bionic tests seem to use a mix of != 0 and == -1.

areusch · 2022-06-16T23:02:27Z

src/runtime/threading_backend.cc

@@ -164,9 +172,13 @@ class ThreadGroup::Impl {
      CPU_SET(id, &cpuset);
    }
 #if defined(__ANDROID__)
-    sched_setaffinity(thread, sizeof(cpu_set_t), &cpuset);
+    if (sched_setaffinity(tid, sizeof(cpu_set_t), &cpuset) == -1) {
+      LOG(WARNING) << "SetThreadAffinity fail!";


possible to obtain more detail from strerror() or similar?

Good advice.

supersat · 2022-06-16T23:43:01Z

It feels a little weird doing something different on regular Linux vs. Android. Can we get rid of the pthread_setaffinity_np call and just use sched_setaffinity on Linux? I think we could get rid of the duplicate thread argument that way. The only reason to keep it is if we supported SetThreadAffinity on platforms other than Linux (and had to use std::thread::native_handle_type) but we don't.

sunjiweiswift · 2022-06-17T02:36:58Z

@supersat Can we get rid of the pthread_setaffinity_np call and just use sched_setaffinity on Linux? I think we can just use seched_setaffinity on Linux and Android

huajsj · 2022-06-17T17:33:38Z

src/runtime/threading_backend.cc

@@ -220,7 +235,7 @@ class ThreadGroup::Impl {
            } else {
              core_id = sorted_order_[i + exclude_worker0];
            }
-            SetThreadAffinity(threads_[i].native_handle(), {core_id});
+            SetThreadAffinity(threads_[i].native_handle(), threads_tid_[i], {core_id});


This part seems like have a timing issue, threads_tid_ shared by main thread and worker threads, current logic not guarantee that the threads_tid_[i] is valid when main thread to read it. I think we should can reproduce such issue by adding a debug "sleep" between line 118 and line 119

areusch · 2022-06-21T19:21:56Z

looks like some of the hexagon tests are segfaulting now, e.g.:

tests/python/contrib/test_hexagon/topi/test_avg_pool2d_slice.py::TestAvgPool2dSlice::test_avg_pool2d_slice[nhwc-8h2w32c2w-2d-stride6-kernel6-float16-dilation6-padding6-True-nhwc-8h2w32c2w-2d-output_shape6-False]

anyone able to look?

huajsj · 2022-06-21T19:36:45Z

src/runtime/threading_backend.cc

-      threads_.emplace_back([worker_callback, i] { worker_callback(i); });
+      threads_.emplace_back([worker_callback, i, this] {
+#ifndef __hexagon__
+        SetTid();


SetTid running in a worker thread, that means the execution order is random, but currently logic assume it follow a order to execution and this would cause logic issue.

we may need to figure out if there is any better and light weight solution.

We need to unify Linux and Android. Providing pid_t and p_thread at the same time is not a good modification.

I agree with that we should unify the interface ,but we may need to fix the bugs first then step in the unify interface change.

Add yeild to wait settid

How the "yield" can help disorder issue in threads_tid_ ? there are two issue in this solution #1 threads_tid_[i] invalid, #2 threads_tid_[i] is not the tid of i thread(disorder issue). the line 112 problem is #2.

I should bind tid to i. Push_back should not be used

@huajsj @areusch

huajsj · 2022-07-22T20:02:05Z

src/runtime/threading_backend.cc

+#endif
+  }
+  void SetTid(size_t index) { threads_tid_[index] = Tid(); }
+  pid_t GetTid(size_t thread_index) { return threads_tid_[thread_index]; }


This not fix the timing issue, that means SetTid may happen after GetTid, and threads_tid_[thread_index] will point to a random value or a '0' value depend on the compiler settings.

If getid() is called before settid(). Gettid will return 0. Call sched_setaffinity will return -1.

Returns -1, indicating that the affinity setting failed. In line with expectations

Returns -1, indicating that the affinity setting failed. In line with expectations

this sounds not make sense, why "affinity setting failed" is " In line with expectations", why need to expect "affinity setting failed"?

SetTid after Gettid means we set affinity for a nonexistent thread. I think should return -1

"SetTid after Gettid" not means "we set affinity for a nonexistent thread", thread already there it just not yet execute the "SetTid" function.

sunjiweiswift added 3 commits June 7, 2022 17:53

[Runtime]bugfix sched_setaffinity should use pid_t instread of pthread_t

0fe480e

[Runtime]bugfix sched_setaffinity should use pid_t instread of pthread_t

24941ad

Merge branch 'set_affinity_android_bugfix' of github.com:sunjiweiswif…

294c961

…t/tvm into set_affinity_android_bugfix

github-actions bot requested a review from areusch June 7, 2022 09:59

ci

5f622e6

sunjiweiswift changed the title ~~[Runtime][threadpool]bugfix~~ [bugfix] sched_setaffinity in Android Jun 10, 2022

sunjiweiswift changed the title ~~[bugfix] sched_setaffinity in Android~~ [Bugfix] sched_setaffinity in Android Jun 10, 2022

sunjiweiswift changed the title ~~[Bugfix] sched_setaffinity in Android~~ [Bugfix][Runtime] sched_setaffinity in Android Jun 10, 2022

sunjiweiswift changed the title ~~[Bugfix][Runtime] sched_setaffinity in Android~~ [Bugfix][Runtime] Fix sched_setaffinity in Android Jun 10, 2022

bugfix

c8dc05b

areusch reviewed Jun 16, 2022

View reviewed changes

add strerror

55b1282

sunjiweiswift added 6 commits June 17, 2022 10:45

add string.h

d5510e0

CI

20a7dde

CI

ae17c2f

CPPlint

2c1a469

CI

8cb2d44

modify head file order

8577550

huajsj reviewed Jun 17, 2022

View reviewed changes

sunjiweiswift added 6 commits June 18, 2022 15:07

modify hexagon syscall

e50446b

clint

3f8807b

clint

7eec91d

modify hexagon

8177b5a

CI

51d122a

CI

dbb571e

huajsj suggested changes Jun 22, 2022

View reviewed changes

sunjiweiswift added 4 commits June 24, 2022 10:05

modify time issue

4d2b246

mofidy tid index

04dea88

modify tid index

70c36be

modify tid Index

fbfb65f

sunjiweiswift requested a review from areusch July 1, 2022 08:41

sunjiweiswift added 5 commits July 20, 2022 15:10

Update threading_backend.cc

a1f24dc

Update threading_backend.cc

bf1a492

Update threading_backend.cc

6d4b901

Update threading_backend.cc

0af6eb0

Update threading_backend.cc

eb77f8e

huajsj suggested changes Jul 22, 2022

View reviewed changes

areusch added needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bugfix][Runtime] Fix sched_setaffinity in Android #11599

[Bugfix][Runtime] Fix sched_setaffinity in Android #11599

sunjiweiswift commented Jun 7, 2022 •

edited by github-actions bot

Loading

sunjiweiswift commented Jun 8, 2022

areusch commented Jun 9, 2022

areusch left a comment

areusch Jun 16, 2022

supersat Jun 16, 2022

areusch Jun 16, 2022

sunjiweiswift Jun 17, 2022

supersat commented Jun 16, 2022

sunjiweiswift commented Jun 17, 2022

huajsj Jun 17, 2022 •

edited

Loading

areusch commented Jun 21, 2022

huajsj Jun 21, 2022

sunjiweiswift Jun 23, 2022

huajsj Jun 23, 2022 •

edited

Loading

sunjiweiswift Jun 24, 2022

huajsj Jun 24, 2022

sunjiweiswift Jun 24, 2022

sunjiweiswift Jul 20, 2022

huajsj Jul 22, 2022

sunjiweiswift Jul 27, 2022

sunjiweiswift Jul 27, 2022

huajsj Aug 5, 2022

sunjiweiswift Aug 5, 2022

huajsj Aug 6, 2022

[Bugfix][Runtime] Fix sched_setaffinity in Android #11599

Are you sure you want to change the base?

[Bugfix][Runtime] Fix sched_setaffinity in Android #11599

Conversation

sunjiweiswift commented Jun 7, 2022 • edited by github-actions bot Loading

sunjiweiswift commented Jun 8, 2022

areusch commented Jun 9, 2022

areusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supersat commented Jun 16, 2022

sunjiweiswift commented Jun 17, 2022

huajsj Jun 17, 2022 • edited Loading

Choose a reason for hiding this comment

areusch commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huajsj Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjiweiswift commented Jun 7, 2022 •

edited by github-actions bot

Loading

huajsj Jun 17, 2022 •

edited

Loading

huajsj Jun 23, 2022 •

edited

Loading