Re: [PATCH v2 3/7] util: Introduce ThreadContext user-creatable object

2022-10-12 Thread David Hildenbrand

Thanks!


I'm adding

"Note that the CPU affinity of previously created threads will not get 
adjusted."

and

"In general, the interface behaves like pthread_setaffinity_np(): host CPU 
numbers that are currently not available are ignored; only host CPU
numbers that are impossible with the current kernel will fail. If the list of 
host CPU numbers does not include a single CPU that is
available, setting the CPU affinity will fail."


This is one of the reasons why reviewing your work is such a pleasure:
not only do you answer my questions with clarity and patience, you
proactively improve your patches before I can even think to ask.

Thank you!


Thanks a lot Markus -- I have to say that your reviews are extremely 
helpful! You ask just the right questions that make one realize which 
parts of the documentation need improvement!


--
Thanks,

David / dhildenb




Re: [PATCH v2 3/7] util: Introduce ThreadContext user-creatable object

2022-10-12 Thread Markus Armbruster
David Hildenbrand  writes:

> Thanks Markus!
>
>> I just tested again, and get the same result as you.  I figure my
>> previous test was with the complete series.
>> PATCH 5 appears to make it work.  Suggest to say something like "The
>> commit after next will make this work".
>
> I'll phrase it like " We'll wire this up next to make it work."

Works for me!

> [...]
>
 So, when a thread is created, its affinity comes from its thread context
 (if any).  When I later change the context's affinity, it does *not*
 affect existing threads, only future ones.  Correct?
>>>
>>> Yes, that's the current state.
>> 
>> Thanks!
>
> I'm adding
>
> "Note that the CPU affinity of previously created threads will not get 
> adjusted."
>
> and
>
> "In general, the interface behaves like pthread_setaffinity_np(): host CPU 
> numbers that are currently not available are ignored; only host CPU 
> numbers that are impossible with the current kernel will fail. If the list of 
> host CPU numbers does not include a single CPU that is 
> available, setting the CPU affinity will fail."

This is one of the reasons why reviewing your work is such a pleasure:
not only do you answer my questions with clarity and patience, you
proactively improve your patches before I can even think to ask.

Thank you!




Re: [PATCH v2 3/7] util: Introduce ThreadContext user-creatable object

2022-10-12 Thread David Hildenbrand

Thanks Markus!


I just tested again, and get the same result as you.  I figure my
previous test was with the complete series.

PATCH 5 appears to make it work.  Suggest to say something like "The
commit after next will make this work".


I'll phrase it like " We'll wire this up next to make it work."

[...]


So, when a thread is created, its affinity comes from its thread context
(if any).  When I later change the context's affinity, it does *not*
affect existing threads, only future ones.  Correct?


Yes, that's the current state.


Thanks!



I'm adding

"Note that the CPU affinity of previously created threads will not get 
adjusted."


and

"In general, the interface behaves like pthread_setaffinity_np(): host 
CPU numbers that are currently not available are ignored; only host CPU 
numbers that are impossible with the current kernel will fail. If the 
list of host CPU numbers does not include a single CPU that is 
available, setting the CPU affinity will fail."


--
Thanks,

David / dhildenb




Re: [PATCH v2 3/7] util: Introduce ThreadContext user-creatable object

2022-10-12 Thread Markus Armbruster
David Hildenbrand  writes:

>>> But note that due to dynamic library loading this example will not work
>>> before we actually make use of thread_context_create_thread() in QEMU
>>> code, because the type will otherwise not get registered.
>> 
>> What do you mean exactly by "not work"?  It's not "CLI option or HMP
>> command fails":
>
> For me, if I compile patch #1-#3 only, I get:
>
> $ ./build/qemu-system-x86_64 -S -display none -nodefaults -monitor stdio 
> -object thread-context,id=tc1,cpu-affinity=0-1,cpu-affinity=6-7
> qemu-system-x86_64: invalid object type: thread-context
>
>
> Reason is that, without a call to thread_context_create_thread(), we won't 
> trigger type_init(thread_context_register_types) and consequently, 
> the type won't be registered.
>
> Is it really different in your environment? Maybe it depends on the QEMU 
> config?

I just tested again, and get the same result as you.  I figure my
previous test was with the complete series.

PATCH 5 appears to make it work.  Suggest to say something like "The
commit after next will make this work".

>>  $ upstream-qemu -S -display none -nodefaults -monitor stdio -object 
>> thread-context,id=tc1,cpu-affinity=0-1,cpu-affinity=6-7
>>  QEMU 7.1.50 monitor - type 'help' for more information
>>  (qemu) qom-get tc1 cpu-affinity
>>  [
>>  0,
>>  1,
>>  6,
>>  7
>>  ]
>>  (qemu) info cpus
>>  * CPU #0: thread_id=1670613
>> 
>> Even though the affinities refer to nonexistent CPUs :)
>
> CPU affinities are CPU numbers on your system (host), not QEMU vCPU numbers. 
> I could talk about physical CPU numbers in the doc here, 
> although I am not sure if that really helps. What about "host CPU numbers" 
> and in patch #4 "host node numbers"?

I think this would reduce the confusion opportunities for noobs like me.

> Seems to match what we document for @MemoryBackendProperties: "@host-nodes: 
> the list of NUMA host nodes to bind the memory to"

Even better.

> But unrelated to that, pthread_setaffinity_np() won't bail out on CPUs that 
> are currently not available in the host -- because one might 
> online/hotplug them later. It only bails out if none of the CPUs is currently 
> available in the host:
>
> https://man7.org/linux/man-pages/man3/pthread_setaffinity_np.3.html
>
>
>EINVAL (pthread_setaffinity_np()) The affinity bit mask mask
>   contains no processors that are currently physically on
>   the system and permitted to the thread according to any
>   restrictions that may be imposed by the "cpuset" mechanism
>   described in cpuset(7).
>
> It will bail out on CPUs that cannot be available in the host though, because 
> it's impossible due to the kernel config:
>
>
>EINVAL (pthread_setaffinity_np()) cpuset specified a CPU that was
>   outside the set supported by the kernel.  (The kernel
>   configuration option CONFIG_NR_CPUS defines the range of
>   the set supported by the kernel data type used to
>   represent CPU sets.)
>
>
>>> A ThreadContext can be reused, simply by reconfiguring the CPU affinity.
>> 
>> So, when a thread is created, its affinity comes from its thread context
>> (if any).  When I later change the context's affinity, it does *not*
>> affect existing threads, only future ones.  Correct?
>
> Yes, that's the current state.

Thanks!

>>> Reviewed-by: Michal Privoznik 
>>> Signed-off-by: David Hildenbrand 

[...]

>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>> index 80dd419b39..67d47f4051 100644
>>> --- a/qapi/qom.json
>>> +++ b/qapi/qom.json
>>> @@ -830,6 +830,21 @@
>>>   'reduced-phys-bits': 'uint32',
>>>   '*kernel-hashes': 'bool' } }
>>>   +##
>>> +# @ThreadContextProperties:
>>> +#
>>> +# Properties for thread context objects.
>>> +#
>>> +# @cpu-affinity: the list of CPU numbers used as CPU affinity for all 
>>> threads
>>> +#created in the thread context (default: QEMU main thread
>>> +#affinity)
>>
>> Another ignorant question: is the QEMU main thread affinity fixed or
>> configurable?  If configurable, how?
>
> AFAIK, it's only configurable externally, for example, via "virsh 
> emulatorpin". There is no QEMU interface to adjust that (because it 
> wouldn't work).
>
> Libvirt will essentially trigger "taskset" on the emulator thread to change 
> its CPU affinity.

I see.

QAPI schema
Acked-by: Markus Armbruster 




Re: [PATCH v2 3/7] util: Introduce ThreadContext user-creatable object

2022-10-11 Thread David Hildenbrand

But note that due to dynamic library loading this example will not work
before we actually make use of thread_context_create_thread() in QEMU
code, because the type will otherwise not get registered.


What do you mean exactly by "not work"?  It's not "CLI option or HMP
command fails":



For me, if I compile patch #1-#3 only, I get:

$ ./build/qemu-system-x86_64 -S -display none -nodefaults -monitor stdio 
-object thread-context,id=tc1,cpu-affinity=0-1,cpu-affinity=6-7

qemu-system-x86_64: invalid object type: thread-context


Reason is that, without a call to thread_context_create_thread(), we 
won't trigger type_init(thread_context_register_types) and consequently, 
the type won't be registered.


Is it really different in your environment? Maybe it depends on the QEMU 
config?



 $ upstream-qemu -S -display none -nodefaults -monitor stdio -object 
thread-context,id=tc1,cpu-affinity=0-1,cpu-affinity=6-7
 QEMU 7.1.50 monitor - type 'help' for more information
 (qemu) qom-get tc1 cpu-affinity
 [
 0,
 1,
 6,
 7
 ]
 (qemu) info cpus
 * CPU #0: thread_id=1670613

Even though the affinities refer to nonexistent CPUs :)


CPU affinities are CPU numbers on your system (host), not QEMU vCPU 
numbers. I could talk about physical CPU numbers in the doc here, 
although I am not sure if that really helps. What about "host CPU 
numbers" and in patch #4 "host node numbers"?


Seems to match what we document for @MemoryBackendProperties: 
"@host-nodes: the list of NUMA host nodes to bind the memory to"




But unrelated to that, pthread_setaffinity_np() won't bail out on CPUs 
that are currently not available in the host -- because one might 
online/hotplug them later. It only bails out if none of the CPUs is 
currently available in the host:


https://man7.org/linux/man-pages/man3/pthread_setaffinity_np.3.html


   EINVAL (pthread_setaffinity_np()) The affinity bit mask mask
  contains no processors that are currently physically on
  the system and permitted to the thread according to any
  restrictions that may be imposed by the "cpuset" mechanism
  described in cpuset(7).

It will bail out on CPUs that cannot be available in the host though, 
because it's impossible due to the kernel config:



   EINVAL (pthread_setaffinity_np()) cpuset specified a CPU that was
  outside the set supported by the kernel.  (The kernel
  configuration option CONFIG_NR_CPUS defines the range of
  the set supported by the kernel data type used to
  represent CPU sets.)





A ThreadContext can be reused, simply by reconfiguring the CPU affinity.


So, when a thread is created, its affinity comes from its thread context
(if any).  When I later change the context's affinity, it does *not*
affect existing threads, only future ones.  Correct?


Yes, that's the current state.




Reviewed-by: Michal Privoznik 
Signed-off-by: David Hildenbrand 
---
  include/qemu/thread-context.h |  57 +++
  qapi/qom.json |  17 +++
  util/meson.build  |   1 +
  util/oslib-posix.c|   1 +
  util/thread-context.c | 278 ++
  5 files changed, 354 insertions(+)
  create mode 100644 include/qemu/thread-context.h
  create mode 100644 util/thread-context.c

diff --git a/include/qemu/thread-context.h b/include/qemu/thread-context.h
new file mode 100644
index 00..2ebd6b7fe1
--- /dev/null
+++ b/include/qemu/thread-context.h
@@ -0,0 +1,57 @@
+/*
+ * QEMU Thread Context
+ *
+ * Copyright Red Hat Inc., 2022
+ *
+ * Authors:
+ *  David Hildenbrand 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef SYSEMU_THREAD_CONTEXT_H
+#define SYSEMU_THREAD_CONTEXT_H
+
+#include "qapi/qapi-types-machine.h"
+#include "qemu/thread.h"
+#include "qom/object.h"
+
+#define TYPE_THREAD_CONTEXT "thread-context"
+OBJECT_DECLARE_TYPE(ThreadContext, ThreadContextClass,
+THREAD_CONTEXT)
+
+struct ThreadContextClass {
+ObjectClass parent_class;
+};
+
+struct ThreadContext {
+/* private */
+Object parent;
+
+/* private */
+unsigned int thread_id;
+QemuThread thread;
+
+/* Semaphore to wait for context thread action. */
+QemuSemaphore sem;
+/* Semaphore to wait for action in context thread. */
+QemuSemaphore sem_thread;
+/* Mutex to synchronize requests. */
+QemuMutex mutex;
+
+/* Commands for the thread to execute. */
+int thread_cmd;
+void *thread_cmd_data;
+
+/* CPU affinity bitmap used for initialization. */
+unsigned long *init_cpu_bitmap;
+int init_cpu_nbits;
+};
+
+void thread_context_create_thread(ThreadContext *tc, QemuThread *thread,
+  const char *name,
+  void *(*start_routine)(void 

Re: [PATCH v2 3/7] util: Introduce ThreadContext user-creatable object

2022-10-10 Thread Markus Armbruster
David Hildenbrand  writes:

> Setting the CPU affinity of QEMU threads is a bit problematic, because
> QEMU doesn't always have permissions to set the CPU affinity itself,
> for example, with seccomp after initialized by QEMU:
> -sandbox enable=on,resourcecontrol=deny
>
> General information about CPU affinities can be found in the man page of
> taskset:
> CPU affinity is a scheduler property that "bonds" a process to a given
> set of CPUs on the system. The Linux scheduler will honor the given CPU
> affinity and the process will not run on any other CPUs.
>
> While upper layers are already aware of how to handle CPU affinities for
> long-lived threads like iothreads or vcpu threads, especially short-lived
> threads, as used for memory-backend preallocation, are more involved to
> handle. These threads are created on demand and upper layers are not even
> able to identify and configure them.
>
> Introduce the concept of a ThreadContext, that is essentially a thread
> used for creating new threads. All threads created via that context
> thread inherit the configured CPU affinity. Consequently, it's
> sufficient to create a ThreadContext and configure it once, and have all
> threads created via that ThreadContext inherit the same CPU affinity.
>
> The CPU affinity of a ThreadContext can be configured two ways:
>
> (1) Obtaining the thread id via the "thread-id" property and setting the
> CPU affinity manually.
>
> (2) Setting the "cpu-affinity" property and letting QEMU try set the
> CPU affinity itself. This will fail if QEMU doesn't have permissions
> to do so anymore after seccomp was initialized.
>
> A simple QEMU example to set the CPU affinity to CPU 0,1,6,7 would be:
> qemu-system-x86_64 -S \
>   -object thread-context,id=tc1,cpu-affinity=0-1,cpu-affinity=6-7
>
> And we can query it via HMP/QMP:
> (qemu) qom-get tc1 cpu-affinity
> [
> 0,
> 1,
> 6,
> 7
> ]
>
> But note that due to dynamic library loading this example will not work
> before we actually make use of thread_context_create_thread() in QEMU
> code, because the type will otherwise not get registered.

What do you mean exactly by "not work"?  It's not "CLI option or HMP
command fails":

$ upstream-qemu -S -display none -nodefaults -monitor stdio -object 
thread-context,id=tc1,cpu-affinity=0-1,cpu-affinity=6-7
QEMU 7.1.50 monitor - type 'help' for more information
(qemu) qom-get tc1 cpu-affinity
[
0,
1,
6,
7
]
(qemu) info cpus
* CPU #0: thread_id=1670613

Even though the affinities refer to nonexistent CPUs :)

> A ThreadContext can be reused, simply by reconfiguring the CPU affinity.

So, when a thread is created, its affinity comes from its thread context
(if any).  When I later change the context's affinity, it does *not*
affect existing threads, only future ones.  Correct?

> Reviewed-by: Michal Privoznik 
> Signed-off-by: David Hildenbrand 
> ---
>  include/qemu/thread-context.h |  57 +++
>  qapi/qom.json |  17 +++
>  util/meson.build  |   1 +
>  util/oslib-posix.c|   1 +
>  util/thread-context.c | 278 ++
>  5 files changed, 354 insertions(+)
>  create mode 100644 include/qemu/thread-context.h
>  create mode 100644 util/thread-context.c
>
> diff --git a/include/qemu/thread-context.h b/include/qemu/thread-context.h
> new file mode 100644
> index 00..2ebd6b7fe1
> --- /dev/null
> +++ b/include/qemu/thread-context.h
> @@ -0,0 +1,57 @@
> +/*
> + * QEMU Thread Context
> + *
> + * Copyright Red Hat Inc., 2022
> + *
> + * Authors:
> + *  David Hildenbrand 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef SYSEMU_THREAD_CONTEXT_H
> +#define SYSEMU_THREAD_CONTEXT_H
> +
> +#include "qapi/qapi-types-machine.h"
> +#include "qemu/thread.h"
> +#include "qom/object.h"
> +
> +#define TYPE_THREAD_CONTEXT "thread-context"
> +OBJECT_DECLARE_TYPE(ThreadContext, ThreadContextClass,
> +THREAD_CONTEXT)
> +
> +struct ThreadContextClass {
> +ObjectClass parent_class;
> +};
> +
> +struct ThreadContext {
> +/* private */
> +Object parent;
> +
> +/* private */
> +unsigned int thread_id;
> +QemuThread thread;
> +
> +/* Semaphore to wait for context thread action. */
> +QemuSemaphore sem;
> +/* Semaphore to wait for action in context thread. */
> +QemuSemaphore sem_thread;
> +/* Mutex to synchronize requests. */
> +QemuMutex mutex;
> +
> +/* Commands for the thread to execute. */
> +int thread_cmd;
> +void *thread_cmd_data;
> +
> +/* CPU affinity bitmap used for initialization. */
> +unsigned long *init_cpu_bitmap;
> +int init_cpu_nbits;
> +};
> +
> +void thread_context_create_thread(ThreadContext *tc, QemuThread *thread,
> +

[PATCH v2 3/7] util: Introduce ThreadContext user-creatable object

2022-10-10 Thread David Hildenbrand
Setting the CPU affinity of QEMU threads is a bit problematic, because
QEMU doesn't always have permissions to set the CPU affinity itself,
for example, with seccomp after initialized by QEMU:
-sandbox enable=on,resourcecontrol=deny

General information about CPU affinities can be found in the man page of
taskset:
CPU affinity is a scheduler property that "bonds" a process to a given
set of CPUs on the system. The Linux scheduler will honor the given CPU
affinity and the process will not run on any other CPUs.

While upper layers are already aware of how to handle CPU affinities for
long-lived threads like iothreads or vcpu threads, especially short-lived
threads, as used for memory-backend preallocation, are more involved to
handle. These threads are created on demand and upper layers are not even
able to identify and configure them.

Introduce the concept of a ThreadContext, that is essentially a thread
used for creating new threads. All threads created via that context
thread inherit the configured CPU affinity. Consequently, it's
sufficient to create a ThreadContext and configure it once, and have all
threads created via that ThreadContext inherit the same CPU affinity.

The CPU affinity of a ThreadContext can be configured two ways:

(1) Obtaining the thread id via the "thread-id" property and setting the
CPU affinity manually.

(2) Setting the "cpu-affinity" property and letting QEMU try set the
CPU affinity itself. This will fail if QEMU doesn't have permissions
to do so anymore after seccomp was initialized.

A simple QEMU example to set the CPU affinity to CPU 0,1,6,7 would be:
qemu-system-x86_64 -S \
  -object thread-context,id=tc1,cpu-affinity=0-1,cpu-affinity=6-7

And we can query it via HMP/QMP:
(qemu) qom-get tc1 cpu-affinity
[
0,
1,
6,
7
]

But note that due to dynamic library loading this example will not work
before we actually make use of thread_context_create_thread() in QEMU
code, because the type will otherwise not get registered.

A ThreadContext can be reused, simply by reconfiguring the CPU affinity.

Reviewed-by: Michal Privoznik 
Signed-off-by: David Hildenbrand 
---
 include/qemu/thread-context.h |  57 +++
 qapi/qom.json |  17 +++
 util/meson.build  |   1 +
 util/oslib-posix.c|   1 +
 util/thread-context.c | 278 ++
 5 files changed, 354 insertions(+)
 create mode 100644 include/qemu/thread-context.h
 create mode 100644 util/thread-context.c

diff --git a/include/qemu/thread-context.h b/include/qemu/thread-context.h
new file mode 100644
index 00..2ebd6b7fe1
--- /dev/null
+++ b/include/qemu/thread-context.h
@@ -0,0 +1,57 @@
+/*
+ * QEMU Thread Context
+ *
+ * Copyright Red Hat Inc., 2022
+ *
+ * Authors:
+ *  David Hildenbrand 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef SYSEMU_THREAD_CONTEXT_H
+#define SYSEMU_THREAD_CONTEXT_H
+
+#include "qapi/qapi-types-machine.h"
+#include "qemu/thread.h"
+#include "qom/object.h"
+
+#define TYPE_THREAD_CONTEXT "thread-context"
+OBJECT_DECLARE_TYPE(ThreadContext, ThreadContextClass,
+THREAD_CONTEXT)
+
+struct ThreadContextClass {
+ObjectClass parent_class;
+};
+
+struct ThreadContext {
+/* private */
+Object parent;
+
+/* private */
+unsigned int thread_id;
+QemuThread thread;
+
+/* Semaphore to wait for context thread action. */
+QemuSemaphore sem;
+/* Semaphore to wait for action in context thread. */
+QemuSemaphore sem_thread;
+/* Mutex to synchronize requests. */
+QemuMutex mutex;
+
+/* Commands for the thread to execute. */
+int thread_cmd;
+void *thread_cmd_data;
+
+/* CPU affinity bitmap used for initialization. */
+unsigned long *init_cpu_bitmap;
+int init_cpu_nbits;
+};
+
+void thread_context_create_thread(ThreadContext *tc, QemuThread *thread,
+  const char *name,
+  void *(*start_routine)(void *), void *arg,
+  int mode);
+
+#endif /* SYSEMU_THREAD_CONTEXT_H */
diff --git a/qapi/qom.json b/qapi/qom.json
index 80dd419b39..67d47f4051 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -830,6 +830,21 @@
 'reduced-phys-bits': 'uint32',
 '*kernel-hashes': 'bool' } }
 
+##
+# @ThreadContextProperties:
+#
+# Properties for thread context objects.
+#
+# @cpu-affinity: the list of CPU numbers used as CPU affinity for all threads
+#created in the thread context (default: QEMU main thread
+#affinity)
+#
+# Since: 7.2
+##
+{ 'struct': 'ThreadContextProperties',
+  'data': { '*cpu-affinity': ['uint16'] } }
+
+
 ##
 # @ObjectType:
 #
@@ -882,6 +897,7 @@
 { 'name': 'secret_keyring',
   'if': 'CONFIG_SECRET_KEYRING' },