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 2022/05/06 10:24:46 UTC

[GitHub] [incubator-nuttx] minabeoki opened a new pull request, #6215: sched/group: addrenv: allocate g_pid_current for each cpu

minabeoki opened a new pull request, #6215:
URL: https://github.com/apache/incubator-nuttx/pull/6215

   ## Summary
   This PR contains 2 commits:
   - In case of SMP and ADDRENV, allocate `g_pid_current` for each cpu
     - `g_pid_current` holds the pid of the group and uses for addrenv switching
     - This is required for each cpu
   - DEBUGASSERT is moved into if block
     - If old `g_pid_current`'s task has already been finished,
       oldgroup is NULL and DEBUGASSERT is wrong
   
   ## Impact
   - ADDRENV=y and SMP=y
   
   ## Testing
   - sabre-6quad:smp w/ qemu
   - sabre-6quad:knsh w/ qemu
   - sabre-6quad:knsh_smp w/ qemu (WIP)


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6215: sched/group: addrenv: allocate g_pid_current for each cpu

Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6215:
URL: https://github.com/apache/incubator-nuttx/pull/6215#discussion_r866932827


##########
sched/group/group_addrenv.c:
##########
@@ -119,15 +146,15 @@ int group_addrenv(FAR struct tcb_s *tcb)
   /* Are we going to change address environments? */
 
   flags = enter_critical_section();
-  if (pid != g_pid_current)
+  if (pid != g_pid_current[cpu])

Review Comment:
   Suggestion:
   Implement a getter for g_pid_current[this_cpu] and store its value upon entry.
   
   Something like this:
   
   int get_pid_current(void)
   {
     int pid = g_pid_current[cpu];
     return pid == IDLE_PROCESS_ID ? INVALID_PROCESS_ID : pid;
   }
   
   This way you don't need to initialize the struct



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] minabeoki commented on a diff in pull request #6215: sched/group: addrenv: allocate g_pid_current for each cpu

Posted by GitBox <gi...@apache.org>.
minabeoki commented on code in PR #6215:
URL: https://github.com/apache/incubator-nuttx/pull/6215#discussion_r866712813


##########
sched/group/group_addrenv.c:
##########
@@ -46,7 +46,19 @@
  * This must only be accessed with interrupts disabled.
  */
 
-pid_t g_pid_current = INVALID_PROCESS_ID;
+pid_t g_pid_current[CONFIG_SMP_NCPUS] =
+{
+  INVALID_PROCESS_ID,
+#if CONFIG_SMP_NCPUS > 1
+  INVALID_PROCESS_ID,
+#if CONFIG_SMP_NCPUS > 2
+  INVALID_PROCESS_ID,
+#if CONFIG_SMP_NCPUS > 3

Review Comment:
   Yes. Is it better to increase to 8?  Or more increasing?



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6215: sched/group: addrenv: allocate g_pid_current for each cpu

Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6215:
URL: https://github.com/apache/incubator-nuttx/pull/6215#discussion_r866932827


##########
sched/group/group_addrenv.c:
##########
@@ -119,15 +146,15 @@ int group_addrenv(FAR struct tcb_s *tcb)
   /* Are we going to change address environments? */
 
   flags = enter_critical_section();
-  if (pid != g_pid_current)
+  if (pid != g_pid_current[cpu])

Review Comment:
   Suggestion:
   Implement a getter for g_pid_current[this_cpu] and store its value upon entry.
   
   Something like this:
   
   ```
   int get_pid_current(void)
   {
     int pid = g_pid_current[cpu];
     return pid == IDLE_PROCESS_ID ? INVALID_PROCESS_ID : pid;
   }
   ```
   
   Just an idea, this way you don't need to initialize the struct



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6215: sched/group: addrenv: allocate g_pid_current for each cpu

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6215:
URL: https://github.com/apache/incubator-nuttx/pull/6215#discussion_r867488156


##########
sched/group/group_addrenv.c:
##########
@@ -85,9 +85,11 @@ int group_addrenv(FAR struct tcb_s *tcb)
   FAR struct task_group_s *group;
   FAR struct task_group_s *oldgroup;
   irqstate_t flags;
-  pid_t pid;
+  int cpu;
   int ret;
 
+  cpu = this_cpu();

Review Comment:
   need move after line 118, otherwise it may stale if this task migrate to other cpu in the middle of execution.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6215: sched/group: addrenv: allocate g_pid_current for each cpu

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6215:
URL: https://github.com/apache/incubator-nuttx/pull/6215#discussion_r866701081


##########
sched/group/group_addrenv.c:
##########
@@ -46,7 +46,19 @@
  * This must only be accessed with interrupts disabled.
  */
 
-pid_t g_pid_current = INVALID_PROCESS_ID;
+pid_t g_pid_current[CONFIG_SMP_NCPUS] =
+{
+  INVALID_PROCESS_ID,
+#if CONFIG_SMP_NCPUS > 1
+  INVALID_PROCESS_ID,
+#if CONFIG_SMP_NCPUS > 2
+  INVALID_PROCESS_ID,
+#if CONFIG_SMP_NCPUS > 3

Review Comment:
   can we make it more extensible?



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6215: sched/group: addrenv: allocate g_pid_current for each cpu

Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6215:
URL: https://github.com/apache/incubator-nuttx/pull/6215#discussion_r866932827


##########
sched/group/group_addrenv.c:
##########
@@ -119,15 +146,15 @@ int group_addrenv(FAR struct tcb_s *tcb)
   /* Are we going to change address environments? */
 
   flags = enter_critical_section();
-  if (pid != g_pid_current)
+  if (pid != g_pid_current[cpu])

Review Comment:
   Suggestion:
   Implement a getter for g_pid_current[this_cpu] and store its value upon entry.
   
   Something like this:
   
   ```
   int get_pid_current(void)
   {
     int pid = g_pid_current[cpu];
     return (pid == IDLE_PROCESS_ID) ? INVALID_PROCESS_ID : pid;
   }
   ```
   
   Just an idea, this way you don't need to initialize the struct



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6215: sched/group: addrenv: allocate g_pid_current for each cpu

Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6215:
URL: https://github.com/apache/incubator-nuttx/pull/6215#discussion_r866932827


##########
sched/group/group_addrenv.c:
##########
@@ -119,15 +146,15 @@ int group_addrenv(FAR struct tcb_s *tcb)
   /* Are we going to change address environments? */
 
   flags = enter_critical_section();
-  if (pid != g_pid_current)
+  if (pid != g_pid_current[cpu])

Review Comment:
   Suggestion:
   Implement a getter for g_pid_current[this_cpu()] and store its value upon entry.
   
   Something like this:
   
   ```
   int get_pid_current(void)
   {
     int pid = g_pid_current[this_cpu()];
     return (pid == IDLE_PROCESS_ID) ? INVALID_PROCESS_ID : pid;
   }
   ```
   
   Just an idea, this way you don't need to initialize the struct



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] pkarashchenko merged pull request #6215: sched/group: addrenv: allocate current group for each cpu

Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #6215:
URL: https://github.com/apache/incubator-nuttx/pull/6215


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] pussuw commented on a diff in pull request #6215: sched/group: addrenv: allocate g_pid_current for each cpu

Posted by GitBox <gi...@apache.org>.
pussuw commented on code in PR #6215:
URL: https://github.com/apache/incubator-nuttx/pull/6215#discussion_r866932827


##########
sched/group/group_addrenv.c:
##########
@@ -119,15 +146,15 @@ int group_addrenv(FAR struct tcb_s *tcb)
   /* Are we going to change address environments? */
 
   flags = enter_critical_section();
-  if (pid != g_pid_current)
+  if (pid != g_pid_current[cpu])

Review Comment:
   Suggestion:
   Implement a getter for g_pid_current[this_cpu()] and store its value upon entry.
   
   Something like this:
   
   ```
   int get_pid_current(void)
   {
     int pid = g_pid_current[cpu];
     return (pid == IDLE_PROCESS_ID) ? INVALID_PROCESS_ID : pid;
   }
   ```
   
   Just an idea, this way you don't need to initialize the struct



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6215: sched/group: addrenv: allocate g_pid_current for each cpu

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6215:
URL: https://github.com/apache/incubator-nuttx/pull/6215#discussion_r866904975


##########
sched/group/group_addrenv.c:
##########
@@ -46,7 +46,19 @@
  * This must only be accessed with interrupts disabled.
  */
 
-pid_t g_pid_current = INVALID_PROCESS_ID;
+pid_t g_pid_current[CONFIG_SMP_NCPUS] =
+{
+  INVALID_PROCESS_ID,
+#if CONFIG_SMP_NCPUS > 1
+  INVALID_PROCESS_ID,
+#if CONFIG_SMP_NCPUS > 2
+  INVALID_PROCESS_ID,
+#if CONFIG_SMP_NCPUS > 3

Review Comment:
   can we support any number?:), do we really need to initialize it? or zero value is also good since idle thread never has the userspace map which mean use zero to identify the invalid case is also good.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6215: sched/group: addrenv: allocate g_pid_current for each cpu

Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6215:
URL: https://github.com/apache/incubator-nuttx/pull/6215#issuecomment-1120428322

   Changes are fine, but seems like PR description is outdated and does not describe the current changes


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org