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/05/22 02:21:13 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #1099: sched/task: add use stack support for kernel thread
anchao opened a new pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099
## Summary
sched/task: add use stack support for kernel thread
An extended version of kthread_create(), use a reserved stack area for kernel thread.
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-634607385
> 255 /* Then activate the task at the provided priority */
> 256
> 257 ret = task_activate((FAR struct tcb_s *)tcb);
> 258 if (ret < 0)
> 259 {
> _kmm_free(tcb);_
> kumm_free(stack);
> return ret;
> 262 }
There is an error in that. After task_init() is called, `sched_releasetcb() `should be called to free the TCB.
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-633337868
> If you are uncomfortable with that interface change, let me know. I believe that it is very important and I would be happy to make this change for you.
Yes, it is indeed an ugly name for the nuttx naming style, but this is a simple way to support this feature, please keep this PR, I will reconsider the implementation here, upload will be later
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo closed pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-633349266
> > If you are uncomfortable with that interface change, let me know. I believe that it is very important and I would be happy to make this change for you.
>
> Yes, it is indeed an ugly name for the nuttx naming style, but this is a simple way to support this feature, please keep this PR, I will reconsider the implementation here, upload will be later
Simple is not important NuttX. The more important value is consistency. Read the INVIOLABLES.txt
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-634607385
> 255 /* Then activate the task at the provided priority */
> 256
> 257 ret = task_activate((FAR struct tcb_s *)tcb);
> 258 if (ret < 0)
> 259 {
> _kmm_free(tcb);_
> kumm_free(stack);
> return ret;
> 262 }
There is an error in that. After task_init() is called, `sched_releasetcb() `should be called to free the TCB. In addition, the TCB needs to be removed from the active task list. The correct error handling for this case is as in thread_create():
146 /* Activate the task */
147
148 ret = nxtask_activate((FAR struct tcb_s *)tcb);
149 if (ret < OK)
150 {
151 /* The TCB was added to the active task list by
152 * nxtask_setup_scheduler()
153 */
154
155 _dq_rem((FAR dq_entry_t *)tcb, (FAR dq_queue_t *)&g_inactivetasks);_
156 goto errout_with_tcb;
157 }
158
159 return pid;
160
161 errout_with_tcb:
162 _nxsched_release_tcb((FAR struct tcb_s *)tcb, ttype);_
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-634607385
> 255 /* Then activate the task at the provided priority */
> 256
> 257 ret = task_activate((FAR struct tcb_s *)tcb);
> 258 if (ret < 0)
> 259 {
> _kmm_free(tcb);_
> kumm_free(stack);
> return ret;
> 262 }
There is an error in that. After task_init() is called, `sched_releasetcb() `should be called to free the TCB. In addition, the TCB needs to be removed from the active task list. The correct error handling for this case is as in `thread_create()`:
146 /* Activate the task */
147
148 ret = nxtask_activate((FAR struct tcb_s *)tcb);
149 if (ret < OK)
150 {
151 /* The TCB was added to the active task list by
152 * nxtask_setup_scheduler()
153 */
154
155 dq_rem((FAR dq_entry_t *)tcb, (FAR dq_queue_t *)&g_inactivetasks);
156 goto errout_with_tcb;
157 }
158
159 return pid;
160
161 errout_with_tcb:
162 nxsched_release_tcb((FAR struct tcb_s *)tcb, ttype);
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-632939991
> * ktrhead_init which does all of the setup of thread_create without creating the stack and without calling task_activate.
> * kthread_activate which just calls task_activate() with the caller provided stack.
After calling up_usestack() ,of course.
> * thread_create() which just calls kthread_init(), allocates the stack, and calls thread_activate().
Allocating the stack via up_createstack, of course
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-634607385
> 255 /* Then activate the task at the provided priority */
> 256
> 257 ret = task_activate((FAR struct tcb_s *)tcb);
> 258 if (ret < 0)
> 259 {
> _kmm_free(tcb);_
> kumm_free(stack);
> return ret;
> 262 }
There is an error in that. After task_init() is called, `sched_releasetcb() `should be called to free the TCB. In addition, the TCB needs to be removed from the active task list. The correct error handling for this case is as in thread_create():
146 /* Activate the task */
147
148 ret = nxtask_activate((FAR struct tcb_s *)tcb);
149 if (ret < OK)
150 {
151 /* The TCB was added to the active task list by
152 * nxtask_setup_scheduler()
153 */
154
155 dq_rem((FAR dq_entry_t *)tcb, (FAR dq_queue_t *)&g_inactivetasks);
156 goto errout_with_tcb;
157 }
158
159 return pid;
160
161 errout_with_tcb:
162 nxsched_release_tcb((FAR struct tcb_s *)tcb, ttype);
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-633350257
We can keep this open, but it should not 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] patacongo commented on pull request #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-634841546
@anchao This has gotten very messy. Sorry. I put a cleaned up version here: https://cwiki.apache.org/confluence/display/NUTTX/Kernel+Threads+with+Custom+Stacks
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-633719356
Here is a summary of the relevant lines from binfmt/binfmt_execmodule.c (with a few embellishments). This is all that you need to do:
135 /* Allocate a TCB for the new task. */
136
137 tcb = (FAR struct task_tcb_s *)kmm_zalloc(sizeof(struct task_tcb_s));
138 if (tcb == NULL)
139 {
140 return -ENOMEM;
141 }
tcb->flags = TCB_FLAG_TTYPE_KERNEL;
153
154 /* Allocate the stack for the new task.
155 *
156 * REVISIT: This allocation is currently always from the user heap. That
157 * will need to change if/when we want to support dynamic stack allocation.
158 */
159
160 stack = (FAR uint32_t *)kumm_malloc(binp->stacksize);
161 if (stack == NULL)
162 {
kmm_free(tcb);
return -ENOMEM;
165 }
166
167 /* Initialize the task */
168
169 ret = task_init((FAR struct tcb_s *)tcb, binp->filename, binp->priority,
170 stack, binp->stacksize, binp->entrypt, binp->argv);
171 if (ret < 0)
172 {
kmm_free(tcb);
174 kumm_free(stack);
return ret;
176 }
254
255 /* Then activate the task at the provided priority */
256
257 ret = task_activate((FAR struct tcb_s *)tcb);
258 if (ret < 0)
259 {
kmm_free(tcb);
kumm_free(stack);
return ret;
262 }
the task_init() and task_activate interfaces were created to permit complete control over how tasks are created. You have complete flexibility but with the cost of some slightly more complex interfaces. This is a good thing and a proper use of the interfaces within the OS.
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-633349266
> > If you are uncomfortable with that interface change, let me know. I believe that it is very important and I would be happy to make this change for you.
>
> Yes, it is indeed an ugly name for the nuttx naming style, but this is a simple way to support this feature, please keep this PR, I will reconsider the implementation here, upload will be later
Simple is not important NuttX. The more important value is consistency. Read the INVIOLABLES.txt
Simple is what businesses do to save money. Open source projects should be me concerned with quality and not low effort.
Again, I could implement this for you in probably one hour and would be happy to do so if you would like me to.
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-633717174
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-632967392
If you are uncomfortable with that interface change, let me know. I believe that it is very important and I would be happy to make this change for you.
----------------------------------------------------------------
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 #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#issuecomment-634804769
I added task_uninit() with PR #1140 to simplify the error recovery if nxtask_activate() fails after a sucessful call to nxtask_init(). Then the error recovery is much cleaner and not so mysterious:
/* Then activate the task at the provided priority */
ret = task_activate((FAR struct tcb_s *)tcb);
if (ret < 0)
{
nxtask_uninit(tcb);
}
----------------------------------------------------------------
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 a change in pull request #1099: sched/task: add use stack support for kernel thread
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1099:
URL: https://github.com/apache/incubator-nuttx/pull/1099#discussion_r429345837
##########
File path: sched/task/task_create.c
##########
@@ -286,6 +297,36 @@ int task_create(FAR const char *name, int priority,
int kthread_create(FAR const char *name, int priority,
int stack_size, main_t entry, FAR char * const argv[])
{
- return nxthread_create(name, TCB_FLAG_TTYPE_KERNEL, priority, stack_size,
- entry, argv);
+ return nxthread_create(name, TCB_FLAG_TTYPE_KERNEL, priority, NULL,
+ stack_size, entry, argv);
+}
+
+/****************************************************************************
+ * Name: kthread_create2
Review comment:
I really dislike this naming.
----------------------------------------------------------------
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