sched/tcb: use shared group for kthreads #12320

yf13 · 2024-05-10T11:21:49Z

Summary

Current task group data is embedded in the task TCB, this is also used for kernel threads, which seems can share the group as they all live in kernel space.

This patch uses tcb_s for kthreads with a shared group instance. Thus the overhead is reduced when more kthreads are in use.

Memory usage of group instance, i.e. sizeof(task_group_s), on rv-virt device:

config	size
nsh	720
nsh64	1160
knsh32	248
knsh64	440

Kernel memory footprints collected from attached logs:

config	kthreads #	upstream	patched	saved
nsh	1	7044	7044	0
nsh64	1	10344	10344	0
knsh32	2	10348	9660	588
knsh64	2	13304	12168	1136

The savings are beyond sizeof(task_group_s) due to subordinate data of groups.

Impact

This may affect all configs.

Testing

QEMU 6.2 on Ubuntu 22.04 with configs nsh, nsh64, knsh32 and knsh64. See logs-12320.tar.gz for more details:
- the embedded folder has logs from upstream version.
- the kthreads folder has logs from patched version.
Github CI checks

sched/group/group_create.c

boards/risc-v/qemu-rv/rv-virt/configs/nsh/defconfig

sched/group/group_create.c

yf13 · 2024-05-10T23:31:24Z

@anchao, let's discuss here as these comments are visible in gh tool as well.

Can you teach your opinion about the MM_TLSF_MANAGER? It seems having O(1) alloc() and works in flat build mode. I added *.tlsf.log files for your review.

anchao · 2024-05-11T03:13:17Z

@anchao, let's discuss here as these comments are visible in gh tool as well.

Can you teach your opinion about the MM_TLSF_MANAGER? It seems having O(1) alloc() and works in flat build mode. I added *.tlsf.log files for your review.

Although TLSF has more advantages than Best fit in terms of time complexity O(1), it will cause serious memory fragmentation issues in aging and persistence tests.

I have added an attachment containing the test results of different allocators (Sheet from @XuNeo Thanks), You can generate a chart and observe the memory trend (This chart may not show the worst case scenario of TLSF)
20221110_mem_test.xlsx

yf13 · 2024-05-11T04:16:37Z

@anchao, thanks for sharing the details.

Here I am trying to keep task_tcb_s for userspace tasks but use tcb_s for kthreads as suggested by @xiaoxiang781216 and it looks working for nsh64, I still check other configs. Is this approach fine for you?

anchao · 2024-05-11T04:23:45Z

@anchao, thanks for sharing the details.

Here I am trying to keep task_tcb_s for userspace tasks but use tcb_s for kthreads as suggested by @xiaoxiang781216 and it looks working for nsh64, I still check other configs. Is this approach fine for you?

Sounds good, could you update this PR, I'd like to review the code directly

acassis · 2024-05-11T11:11:31Z

Please fix this failure:

In function 'nxthread_create',
    inlined from 'kthread_create_with_stack' at task/task_create.c:254:10:
Error: task/task_create.c:97:18: error: array subscript 'struct task_tcb_s[0]' is partly outside array bounds of 'unsigned char[368]' [-Werror=array-bounds]
   97 |   tcb->cmn.flags = ttype | TCB_FLAG_FREE_TCB;
      |                  ^
In file included from task/task_create.c:33:
task/task_create.c:88:9: note: object of size 368 allocated by 'zalloc'
   88 |   tcb = kmm_zalloc(ret);
      |         ^~~~~~~~~~
Error: task/task_create.c:111:7: error: array subscript 'struct task_tcb_s[0]' is partly outside array bounds of 'unsigned char[368]' [-Werror=array-bounds]
  111 |   pid = tcb->cmn.pid;
      |   ~~~~^~~~~~~~~~~~~~
task/task_create.c:88:9: note: object of size 368 allocated by 'zalloc'
   88 |   tcb = kmm_zalloc(ret);
      |         ^~~~~~~~~~
In function 'nxthread_create',
    inlined from 'kthread_create_with_stack' at task/task_create.c:254:10,
    inlined from 'kthread_create' at task/task_create.c:285:10:
Error: task/task_create.c:97:18: error: array subscript 'struct task_tcb_s[0]' is partly outside array bounds of 'unsigned char[368]' [-Werror=array-bounds]
   97 |   tcb->cmn.flags = ttype | TCB_FLAG_FREE_TCB;
      |                  ^
task/task_create.c:88:9: note: object of size 368 allocated by 'zalloc'
   88 |   tcb = kmm_zalloc(ret);
      |         ^~~~~~~~~~
Error: task/task_create.c:111:7: error: array subscript 'struct task_tcb_s[0]' is partly outside array bounds of 'unsigned char[368]' [-Werror=array-bounds]
  111 |   pid = tcb->cmn.pid;
      |   ~~~~^~~~~~~~~~~~~~
task/task_create.c:88:9: note: object of size 368 allocated by 'zalloc'
   88 |   tcb = kmm_zalloc(ret);
      |         ^~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:61: task_create.o] Error 1
make[1]: Target 'libsched.a' not remade because of errors.

yf13 · 2024-05-11T11:18:08Z

@acassis I am wondering that is this GCC 12 issue, just pushed a version using $progma to see if it can be fixed. Please teach if there are better fixes.

acassis · 2024-05-11T13:09:04Z

@yf13 adding #pragma in the generic code is not a good idea, maybe you can try to fix the structure to avoid the issue, please see here: micropython/micropython#7064

He create a union to storage the right size

yf13 · 2024-05-11T22:52:46Z

@yf13 adding #pragma in the generic code is not a good idea, maybe you can try to fix the structure to avoid the issue, please see here: micropython/micropython#7064

@acassis thanks, will remove #pragma later after passing all checks.

Now case test_ltp_interfaces_lio_listio_2_1 still fails, need more investigation about it, hints are highly welcomed.

yf13 · 2024-05-23T08:40:39Z

@xiaoxiang781216 it looks that the LTP timeouts in sim/citest still happens, so I disabled them again to see if there are remaining issues. Then a few CI attempts failed due to connectivity issues encountered during CI. Then the usrsocktest issues come back again in my last run.

So looks like that I have to wait until CI is normal to trigger another check.

xiaoxiang781216 · 2024-05-23T10:01:38Z

@xiaoxiang781216 it looks that the LTP timeouts in sim/citest still happens, so I disabled them again to see if there are remaining issues. Then a few CI attempts failed due to connectivity issues encountered during CI. Then the usrsocktest issues come back again in my last run.

So looks like that I have to wait until CI is normal to trigger another check.

@Donny9 is looking this problem.

yf13 · 2024-05-30T01:01:36Z

@anchao, @xiaoxiang781216 looks like the CI system is okay now. Please give this patch a review when free and let me know if there is anything I can do.

include/nuttx/tls.h

sched/group/group_create.c

xiaoxiang781216 · 2024-05-30T18:11:32Z

sched/group/group_create.c

@@ -222,5 +238,6 @@ void group_postinitialize(FAR struct task_tcb_s *tcb)
 * task has exited.
 */

- group->tg_pid = tcb->cmn.pid;
+ group->tg_pid = (group == &g_kthread_group) ? group->tg_pid :


The kthreads group's pid shall stay the same when new kthreads are created, right?

The latest updtae used if for this.

sched/init/nx_start.c

sched/task/task_create.c

sched/task/task_start.c

xiaoxiang781216 · 2024-05-30T18:16:08Z

sched/task/task_setup.c

@@ -527,6 +528,7 @@ static int nxtask_setup_stackargs(FAR struct task_tcb_s *tcb,
 FAR const char *name,
 FAR char * const argv[])
 {
+ const uint8_t ttype = tcb->cmn.flags & TCB_FLAG_TTYPE_MASK;


remove const

not addressed yet

fixed in latest update

sched/group/group_create.c

sched/task/task_create.c

yf13 · 2024-06-05T03:00:33Z

@xiaoxiang781216 looks like our CI got connectivity issue:

Configuration/Tool: ucans32k146/se05x,CONFIG_ARM_TOOLCHAIN_GNU_EABI
ccuurrll::  ((2288))  FFaaiilleedd  ttoo  cocnonnencet ctto  tgoi tghiutbh.ucbo.mc opmo rpto r4t4 3 4a4f3t earf t1e3r5 110365 2m4s4:  mCso:n nCeocntinoenc ttiimoend  toiumte

I forgot somewhere I saw other repositories using sub-module instead of download, not sure if it can reduce this kind of connectivity issues?

Looks like the Linux (sim-xxx) checks passed so our compilation warning fixes shall work now. I will trigger another check soon.

Thread args have already been saved to stack after the TLS section by nxtask_setup_stackargs. This is to retrieve it for use. Signed-off-by: Yanfeng Liu <[email protected]>

xiaoxiang781216 · 2024-06-08T08:02:03Z

sched/task/task_setup.c

@@ -527,6 +528,7 @@ static int nxtask_setup_stackargs(FAR struct task_tcb_s *tcb,
 FAR const char *name,
 FAR char * const argv[])
 {
+ const uint8_t ttype = tcb->cmn.flags & TCB_FLAG_TTYPE_MASK;


not addressed yet

sched/task/task_create.c

xiaoxiang781216 · 2024-06-08T08:06:49Z

sched/task/task_start.c

@@ -87,33 +88,32 @@ void nxtask_start(void)
 /* Execute the start hook if one has been registered */

 #ifdef CONFIG_SCHED_STARTHOOK
- if (ttcb->starthook != NULL)
+ if (ttype != TCB_FLAG_TTYPE_KERNEL && ttcb->starthook != NULL)


why check TCB_FLAG_TTYPE_KERNEL

The kthread doesn't have starthook as it is in task_tcb_s.

sched/group/group_setuptaskfiles.c

xiaoxiang781216 · 2024-06-08T08:16:26Z

libs/libc/stdlib/lib_getprogname.c


- info = task_get_info();
- return info->ta_argv[0];
+ return (rc >= 0 && ret && ((uintptr_t)si.stack_base_ptr) > ret) ?


why ned check stack_base_ptr

The stack_args don't exist for pthreads yet, this check avoids returning wrong result when used from pthread. Previous logic was dangerous as it returns address in main thread's stack.

Kthreads can share the group data so that to reduce overheads. This implements shared kthread group via: - use `tcb_s` instead of `task_tcb_s` for kthreads - use `g_kthread_group` when creating kthreads - use stackargs to start tasks and kthreads see pull/12320 for test logs. Signed-off-by: Yanfeng Liu <[email protected]>

- replaces `ta_argv` with `stackargs` - drops `ta_argv` from `task_info_s` - drops `g_idleargv` and checks idle accordingly Signed-off-by: Yanfeng Liu <[email protected]>

yf13 · 2024-06-10T23:34:54Z

@xiaoxiang781216 the const in task_setup.c was removed in latest push, but I couldn't reply this comment

directly from my browser. Please take a look when free.

anchao reviewed May 10, 2024

View reviewed changes

sched/group/group_create.c Outdated Show resolved Hide resolved

yf13 force-pushed the sched branch from 44ca128 to 85c332b Compare May 10, 2024 13:38

acassis reviewed May 10, 2024

View reviewed changes

boards/risc-v/qemu-rv/rv-virt/configs/nsh/defconfig Outdated Show resolved Hide resolved

yf13 force-pushed the sched branch from 85c332b to 65223c5 Compare May 10, 2024 13:56

acassis approved these changes May 10, 2024

View reviewed changes

davids5 reviewed May 10, 2024

View reviewed changes

sched/group/group_create.c Outdated Show resolved Hide resolved

yf13 force-pushed the sched branch from 65223c5 to 6ac487c Compare May 10, 2024 22:37

yf13 force-pushed the sched branch from 6ac487c to a5f2948 Compare May 11, 2024 04:54

yf13 changed the title ~~sched/tcb: drop task_tcb_s.group~~ sched/tcb: reduce kthread overhead May 11, 2024

yf13 force-pushed the sched branch 3 times, most recently from e93ddd2 to f443ffc Compare May 11, 2024 05:23

yf13 requested review from anchao and xiaoxiang781216 May 11, 2024 05:33

yf13 force-pushed the sched branch 5 times, most recently from 1461cbe to c9a6669 Compare May 11, 2024 10:14

yf13 force-pushed the sched branch from c9a6669 to 74afcfc Compare May 11, 2024 11:11

yf13 force-pushed the sched branch 2 times, most recently from 229d4d6 to 69ba63d Compare May 11, 2024 12:20

yf13 force-pushed the sched branch from b949bca to 0b17782 Compare May 22, 2024 22:44

yf13 changed the title ~~sched/tcb: share group for kthreads~~ sched/tcb: use shared group for kthreads May 22, 2024

yf13 force-pushed the sched branch 3 times, most recently from f4ff342 to 75cafed Compare May 23, 2024 05:18

yf13 force-pushed the sched branch from 75cafed to 08b185f Compare May 23, 2024 08:48

yf13 force-pushed the sched branch 2 times, most recently from a7e0136 to 9868fb6 Compare May 29, 2024 11:07

xiaoxiang781216 reviewed May 30, 2024

View reviewed changes

xiaoxiang781216 reviewed Jun 4, 2024

View reviewed changes

sched/group/group_create.c Outdated Show resolved Hide resolved

sched/task/task_create.c Outdated Show resolved Hide resolved

yf13 force-pushed the sched branch 3 times, most recently from 84d791e to b3039b9 Compare June 5, 2024 00:35

yf13 force-pushed the sched branch 2 times, most recently from 999578c to 8917fdb Compare June 5, 2024 07:51

yf13 requested a review from xiaoxiang781216 June 5, 2024 23:36

sched/tls: add nxsched_get_stackargs

e0523d4

Thread args have already been saved to stack after the TLS section by nxtask_setup_stackargs. This is to retrieve it for use. Signed-off-by: Yanfeng Liu <[email protected]>

yf13 force-pushed the sched branch from 8917fdb to 80dc819 Compare June 7, 2024 21:48

xiaoxiang781216 reviewed Jun 8, 2024

View reviewed changes

yf13 force-pushed the sched branch 2 times, most recently from d246950 to 0cbccbb Compare June 9, 2024 01:37

yf13 added 2 commits June 9, 2024 09:45

sched/tls: drop ta_argv and g_idleargv

44e12be

- replaces `ta_argv` with `stackargs` - drops `ta_argv` from `task_info_s` - drops `g_idleargv` and checks idle accordingly Signed-off-by: Yanfeng Liu <[email protected]>

yf13 force-pushed the sched branch from 0cbccbb to 44e12be Compare June 9, 2024 01:46

yf13 requested a review from xiaoxiang781216 June 10, 2024 06:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sched/tcb: use shared group for kthreads #12320

sched/tcb: use shared group for kthreads #12320

yf13 commented May 10, 2024 •

edited

yf13 commented May 10, 2024 •

edited

anchao commented May 11, 2024

yf13 commented May 11, 2024

anchao commented May 11, 2024

acassis commented May 11, 2024

yf13 commented May 11, 2024

acassis commented May 11, 2024

yf13 commented May 11, 2024

yf13 commented May 23, 2024 •

edited

xiaoxiang781216 commented May 23, 2024

yf13 commented May 30, 2024

xiaoxiang781216 May 30, 2024

yf13 Jun 1, 2024 •

edited

yf13 Jun 10, 2024

xiaoxiang781216 May 30, 2024

xiaoxiang781216 Jun 8, 2024

yf13 Jun 10, 2024

yf13 commented Jun 5, 2024

xiaoxiang781216 Jun 8, 2024

xiaoxiang781216 Jun 8, 2024

yf13 Jun 10, 2024 •

edited

xiaoxiang781216 Jun 8, 2024

yf13 Jun 9, 2024 •

edited

yf13 commented Jun 10, 2024 •

edited

sched/tcb: use shared group for kthreads #12320

Are you sure you want to change the base?

sched/tcb: use shared group for kthreads #12320

Conversation

yf13 commented May 10, 2024 • edited

Summary

Impact

Testing

yf13 commented May 10, 2024 • edited

anchao commented May 11, 2024

yf13 commented May 11, 2024

anchao commented May 11, 2024

acassis commented May 11, 2024

yf13 commented May 11, 2024

acassis commented May 11, 2024

yf13 commented May 11, 2024

yf13 commented May 23, 2024 • edited

xiaoxiang781216 commented May 23, 2024

yf13 commented May 30, 2024

Choose a reason for hiding this comment

yf13 Jun 1, 2024 • edited

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

yf13 commented Jun 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yf13 Jun 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yf13 Jun 9, 2024 • edited

Choose a reason for hiding this comment

yf13 commented Jun 10, 2024 • edited

yf13 commented May 10, 2024 •

edited

yf13 commented May 10, 2024 •

edited

yf13 commented May 23, 2024 •

edited

yf13 Jun 1, 2024 •

edited

yf13 Jun 10, 2024 •

edited

yf13 Jun 9, 2024 •

edited

yf13 commented Jun 10, 2024 •

edited