You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/07/01 13:38:29 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
anchao opened a new pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346
## Summary
sched/kthread: extend kthread_create() to support configurable stack
Extended parameter to support configurable stack
unify the semantics with nxthread_create()
## Impact
kthread_create()
## Testing
create the kernel thread use reserve stack
```
static uint8_t g_stack[8192];
kthread_create("thread test", 100, g_stack, sizeof(g_stack), (main_t)test_thread, NULL);
nsh> hello &
hello [3:100]
Hello, World!!
Hello, thread!!
nsh> ps
PID PPID PRI POLICY TYPE NPX STATE EVENT SIGMASK STACK COMMAND
0 0 0 FIFO Kthread N-- Ready 00000000 000000 Idle Task
2 1 100 FIFO Task --- Running 00000000 008192 /system/bin/nsh
3 2 100 FIFO Task --- Waiting Signal 00000000 008208 hello
4 3 100 FIFO Kthread --- Waiting Signal 00000000 008160 thread test
nsh>
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652755059
This change does not add value to the OS. It do not want it to be merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652428408
Related code involves macro definition of mixed case, I think we can ignore this check
```
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/boards/arm/sama5/sama5d3x-ek/src/sam_usb.c:364:23: error: Mixed case identifier found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/boards/arm/sama5/sama5d3x-ek/src/sam_usb.c:365:29: error: Mixed case identifier found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/boards/arm/sama5/sama5d3x-ek/src/sam_usb.c:386:39: error: Mixed case identifier found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/boards/arm/sama5/sama5d3x-ek/src/sam_usb.c:387:29: error: Mixed case identifier found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/boards/arm/sama5/sama5d3x-ek/src/sam_usb.c:364:23: error: Mixed case identifier found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/boards/arm/sama5/sama5d3x-ek/src/sam_usb.c:365:29: error: Mixed case identifier found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/boards/arm/sama5/sama5d3x-ek/src/sam_usb.c:386:39: error: Mixed case identifier found
##[error]/home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/boards/arm/sama5/sama5d3x-ek/src/sam_usb.c:387:29: error: Mixed case identifier found
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652467695
We have already gone throught this before with PR #1099 . I will not accept this change. The mechanism to provide a configurable status is already in place.
I provided in the instructions in that PR: Here is how you need to create a kernel thread with a custom stack:
Allocate the TCB with kmm_zalloc().
Initialize the TCB. See binfmt/binfmt_execmodule.c for an example for ELF modules.
Make certain that the tcb->flags field is set to indicate a kernel thread:
Allocate a stack from whatever memory you wish.
Call task_init(), passing it the TCB, stack and other parameters.
Do another other things you would like, and finally
Call task_activate()
That is the whole purpose of task_init(). Do not create other implementations with the correct implemtation is already in place.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652574377
> No.. no duplicate functionality
If so, should we provide a patch to remove kthread_create and change all place to task_init/task_activate?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652555710
> We have already gone throught this before with PR #1099 . I will not accept this change. The mechanism to provide a configurable status is already in place.
>
> I provided in the instructions in that PR: Here is how you need to create a kernel thread with a custom stack:
>
> ```
> Allocate the TCB with kmm_zalloc().
> Initialize the TCB. See binfmt/binfmt_execmodule.c for an example for ELF modules.
> Make certain that the tcb->flags field is set to indicate a kernel thread:
> Allocate a stack from whatever memory you wish.
> Call task_init(), passing it the TCB, stack and other parameters.
> Do another other things you would like, and finally
> Call task_activate()
> ```
>
> That is the whole purpose of task_init(). Do not create other implementations with the correct implemtation is already in place.
We need more discuss here: since you and many other people(include me) agree that the current kernel thread implemenation(reuse the most userspace task infrastructure) isn't a perfect solution and will most likely change in the furture.
https://github.com/apache/incubator-nuttx/issues/1108
If we encourage people directly go through task_init/task_activate for the kernel thread creation, this will make the redesgin more harder and from the desgin perspective:
1.It's an internal design decision that kernel thread is built on top of task(I guess only few people know this fact), we shouldn't expose this detail to the normal developer.
2.It's a very bad idea to give an offical guide(create the kernel thread through task_init/task_activate) but we have plan to modify the implementation in an incompatible way.
3.It is a common requirement to create a thread(either kernel or pthread) with a custom stack, why we don't provide this functionality directly just like pthread API?
So @patacongo please reconsider this patch again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652574377
> No.. no duplicate functionality
If so, should we provide a patch to remove all kthread_create and change to task_init/task_activate?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652567672
No.. no duplicate functionality. There is no functionality added by this PR that does not already exist in the OS. Perhaps you would would want your own private wrapper function to simplify the interfaces (like binfmt/binfmt_exec.c), but it is WRONG to force this pointless duplication onto the OS.
Not only does this change duplicate existing functionality, but subverts a common OS internal interface to support special operations used on by Xiaomi and not for any other users. Subverting the OS for your own personal business development is not an acceptable practice.
When you joined the Apache group, you agreed to support the OS for the good of the public with NO influence from your business. This is a bad technical change and it supports only Xaiomi's private interests and hence, has no business in a open operating system.
If you were truly representing the interests of the OS and not your private interests you would understand this. You would understand perfectly if there were some other large business trying subvert the common interfaces for their personal interest.
That is not an acceptable practice in an Apache project.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao edited a comment on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
anchao edited a comment on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652751672
I still do not understand why you should bypass kthread_create and use the task_init/task_active interface directly,
Just for the interface design, the nxtask_ style should be a lower-level interface and talk directly with task struct,
but for kernel developers should be to use kthread_, this is the correct way.
------------------
| Kernel Module |
------------------
|
------------------
| kthread_create |
------------------
|
------------------
| task_init |
| task_active |
------------------
------------------
| Kernel Module | ----- Why?
------------------ |
|
------------------ |
| kthread_create | |
------------------ |
| |
------------------ |
| task_init |<-----
| task_active |
------------------
maybe you will say that we can implement kthread_init/kthread_active to comply better our requirements,
but I do not think so beacuse which will increase the interface complexity, which is not friendly.
In most cases, kernel developers do not need to care about what happens inside the TASK,
developers do not care about what is "tcb", what is "ttcb", what is "task type",
Do you want every kernel module with special requirements for stack to complete such complex implementation?
```
static int nxthread_create(FAR const char *name, uint8_t ttype, int priority,
FAR void *stack, int stack_size, main_t entry,
FAR char * const argv[])
{
FAR struct task_tcb_s *tcb;
pid_t pid;
int ret;
/* Allocate a TCB for the new task. */
tcb = (FAR struct task_tcb_s *)kmm_zalloc(sizeof(struct task_tcb_s));
if (!tcb)
{
serr("ERROR: Failed to allocate TCB\n");
return -ENOMEM;
}
/* Setup the task type */
tcb->cmn.flags = ttype;
/* Initialize the task */
ret = nxtask_init(tcb, name, priority, stack, stack_size, entry, argv);
if (ret < OK)
{
kmm_free(tcb);
return ret;
}
/* Get the assigned pid before we start the task */
pid = tcb->cmn.pid;
/* Activate the task */
nxtask_activate(&tcb->cmn);
return (int)pid;
}
```
> Not only does this change duplicate existing functionality, but subverts a common OS internal interface to support special operations used on by Xiaomi and not for any other users. Subverting the OS for your own personal business development is not an acceptable practice.
Please don't always speculate on our intention, our goal is consistent, In order to make nuttx better, we have been still hard to enhancing nuttx's compatibility, ease-using and reducing unnecessary coupling
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652555710
> We have already gone throught this before with PR #1099 . I will not accept this change. The mechanism to provide a configurable status is already in place.
>
> I provided in the instructions in that PR: Here is how you need to create a kernel thread with a custom stack:
>
> ```
> Allocate the TCB with kmm_zalloc().
> Initialize the TCB. See binfmt/binfmt_execmodule.c for an example for ELF modules.
> Make certain that the tcb->flags field is set to indicate a kernel thread:
> Allocate a stack from whatever memory you wish.
> Call task_init(), passing it the TCB, stack and other parameters.
> Do another other things you would like, and finally
> Call task_activate()
> ```
>
> That is the whole purpose of task_init(). Do not create other implementations with the correct implemtation is already in place.
We need more discuss here: since you and many other people(include me) agree that the current kernel thread implemenation(reuse the most userspace task infrastructure) isn't a perfect solution and will most likely change in the furture.
https://github.com/apache/incubator-nuttx/issues/1108
If we encourage people directly go through task_init/task_activate for the kernel thread creation, this will make the redesgin more harder and from the desgin perspective:
1.It's an internal design decision that kernel thread is build on top of task(I guess only few people know this fact), we shouldn't expose this detail to the normal developer.
2.It's a very bad idea to give an offical guide(create the kernel thread through task_init/task_activate) but we have plan to modify the implementation in an incompatible way.
3.It is a common requirement to create a thread(either kernel or pthread) with a custom stack, why we don't provide this functionality directly just like pthread API?
So @patacongo please reconsider this patch again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao edited a comment on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
anchao edited a comment on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652751672
I still do not understand why you should bypass kthread_create and use the task_init/task_active interface directly,
Just for the interface design, the nxtask_ style should be a lower-level interface and talk directly with task struct,
but for kernel developers should be to use kthread_, this is the correct way.
------------------
| Kernel Module |
------------------
|
------------------
| kthread_create |
------------------
|
------------------
| task_init |
| task_active |
------------------
------------------
| Kernel Module | ----- Why?
------------------ |
|
------------------ |
| kthread_create | |
------------------ |
| |
------------------ |
| task_init |<-----
| task_active |
------------------
maybe you will say that we can implement kthread_init/kthread_active to comply better our requirements,
but I do not think so beacuse which will increase the interface complexity, which is not friendly.
In most cases, kernel developers do not need to care about what happens inside the TASK,
developers do not care about what is "tcb", what is "ttcb", what is "task type",
Do you want every kernel module with special requirements for tasks to complete such complex implementation?
```
static int nxthread_create(FAR const char *name, uint8_t ttype, int priority,
FAR void *stack, int stack_size, main_t entry,
FAR char * const argv[])
{
FAR struct task_tcb_s *tcb;
pid_t pid;
int ret;
/* Allocate a TCB for the new task. */
tcb = (FAR struct task_tcb_s *)kmm_zalloc(sizeof(struct task_tcb_s));
if (!tcb)
{
serr("ERROR: Failed to allocate TCB\n");
return -ENOMEM;
}
/* Setup the task type */
tcb->cmn.flags = ttype;
/* Initialize the task */
ret = nxtask_init(tcb, name, priority, stack, stack_size, entry, argv);
if (ret < OK)
{
kmm_free(tcb);
return ret;
}
/* Get the assigned pid before we start the task */
pid = tcb->cmn.pid;
/* Activate the task */
nxtask_activate(&tcb->cmn);
return (int)pid;
}
```
> Not only does this change duplicate existing functionality, but subverts a common OS internal interface to support special operations used on by Xiaomi and not for any other users. Subverting the OS for your own personal business development is not an acceptable practice.
Please don't always speculate on our intention, our goal is consistent, In order to make nuttx better, we have been still hard to enhancing nuttx's compatibility, ease-using and reducing unnecessary coupling
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo closed pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
patacongo closed pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652467695
We have already gone throught this before with PR #1099 . I will not accept this change. The mechanism to provide a configurable status is already in place.
I provided in the instructions in that PR: Here is how you need to create a kernel thread with a custom stack:
Allocate the TCB with kmm_zalloc().
Initialize the TCB. See binfmt/binfmt_execmodule.c for an example for ELF modules.
Make certain that the tcb->flags field is set to indicate a kernel thread:
Allocate a stack from whatever memory you wish.
Call task_init(), passing it the TCB, stack and other parameters.
Do another other things you would like, and finally
Call task_activate()
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652567672
No.. no duplicate functionality
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo closed pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
patacongo closed pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652751672
I still do not understand why you should bypass kthread_create and use the task_init/task_active interface directly,
Just for the interface design, the nxtask_ style should be a lower-level interface and talk directly with task struct,
but for kernel developers should be to use kthread_, this is the correct way.
------------------
| kernel developer |
------------------
|
------------------
| kthread_create |
------------------
|
------------------
| task_init |
| task_active |
------------------
------------------
| Kernel Module | ----- Why?
------------------ |
|
------------------ |
| kthread_create | |
------------------ |
| |
------------------ |
| task_init |<-----
| task_active |
------------------
maybe you will say that we can implement kthread_init/kthread_active to comply better our requirements,
but I do not think so beacuse which will increase the interface complexity, which is not friendly.
In most cases, kernel developers do not need to care about what happens inside the TASK,
developers do not care about what is "tcb", what is "ttcb", what is "task type",
Do you want every kernel module with special requirements for tasks to complete such complex implementation?
```
static int nxthread_create(FAR const char *name, uint8_t ttype, int priority,
FAR void *stack, int stack_size, main_t entry,
FAR char * const argv[])
{
FAR struct task_tcb_s *tcb;
pid_t pid;
int ret;
/* Allocate a TCB for the new task. */
tcb = (FAR struct task_tcb_s *)kmm_zalloc(sizeof(struct task_tcb_s));
if (!tcb)
{
serr("ERROR: Failed to allocate TCB\n");
return -ENOMEM;
}
/* Setup the task type */
tcb->cmn.flags = ttype;
/* Initialize the task */
ret = nxtask_init(tcb, name, priority, stack, stack_size, entry, argv);
if (ret < OK)
{
kmm_free(tcb);
return ret;
}
/* Get the assigned pid before we start the task */
pid = tcb->cmn.pid;
/* Activate the task */
nxtask_activate(&tcb->cmn);
return (int)pid;
}
```
> Not only does this change duplicate existing functionality, but subverts a common OS internal interface to support special operations used on by Xiaomi and not for any other users. Subverting the OS for your own personal business development is not an acceptable practice.
Please don't always speculate on our intention, our goal is consistent, In order to make nuttx better, we have been still hard to enhancing nuttx's compatibility, ease-using and reducing unnecessary coupling
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1346: sched/kthread: extend kthread_create() to support configurable stack
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1346:
URL: https://github.com/apache/incubator-nuttx/pull/1346#issuecomment-652567672
No.. no duplicate functionality. There is no functionality added by this PR that does not already exist in the OS. Perhaps you would would want your own private wrapper function to simplify the interfaces (like binfmt/binfmt_exec.c), but it is WRONG to force this pointless duplication onto the OS.
Not only does this change duplicate existing functionality, but subverts a common OS internal interface to support special operations used on by Xiaomi and not for any other users. Subverting the OS for your own personal business development is not an acceptable practice.
When you joined the Apache group, you agreed to support the OS for the good of the public with NO influence from your business. This is a bad technical change and it supports only Xaiomi's private interests and hence, has no business in a open operating system.
If you were truly representing the interests of the OS and not your private interests you would understand this. You would understand perfectly if there were some other large business trying subvert the common interfaces for their personal interest.
That is not an acceptable practice in an Apache project and I will resist every attempt by Xiaomi to piss on the operating system for their personal, private interests. If you cannot think of the open source community before your business, you should not be working on an open source project.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org