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 2021/01/28 14:17:14 UTC

[GitHub] [incubator-nuttx] anchao opened a new pull request #2767: sched/wqueue/notifier: protect the work notifier with critical section 

anchao opened a new pull request #2767:
URL: https://github.com/apache/incubator-nuttx/pull/2767


   
   
   ## Summary
   
   sched/wqueue/notifier: protect the work notifier with critical section 
   
   replace the semaphore to avoid the notifier holding the lock in the interrupt context
   
   ASSERT:
   
   ```
   libs/libc/assert/lib_assert.c:36   :_assert
   sched/semphore/sem_wait.c:113      :nxsem_wait
   sched/semphore/sem_wait.c:222      :nxsem_wait_uniterruptible
   sched/wqueue/kwork_notifier.c:371  :work_notifier_signal
   mm/iob/iob_free.c:188              :iob_free
   drivers/syslog/syslog_stream.c:272 :syslogstream_destroy
   ...
   sched/irq/irq_dispatch.c:183       :irq_dispatch
   ```
   
   Signed-off-by: chao.an <an...@xiaomi.com>
   
   ## Impact
   
   call syslog() on interrupt context
   
   ## Testing
   
   


----------------------------------------------------------------
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 #2767: sched/wqueue/notifier: protect the work notifier with critical section

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2767:
URL: https://github.com/apache/incubator-nuttx/pull/2767#issuecomment-770206856


   > Ok that looks better.
   > 
   > Do we know what is the lifetime a notification can live in the list?
   > 
   > The older look up `work_notifier_find(key)` seams to imply that collision can happen and most be avoided. Even at 32 bits, the number will still wrap. Will this be an issue?
   
   The lifetime of notifier is short and the generation rate(from tcp, iob and usb) is also slow, so the possibility is very low. And to reduce the possibity even furture, the patch extend key from 16bit to 32bit too.


----------------------------------------------------------------
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 a change in pull request #2767: sched/wqueue/notifier: protect the work notifier with critical section

Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #2767:
URL: https://github.com/apache/incubator-nuttx/pull/2767#discussion_r566600631



##########
File path: sched/wqueue/kwork_notifier.c
##########
@@ -256,6 +250,10 @@ int work_notifier_setup(FAR struct work_notifier_s *info)
 
       memcpy(&notifier->info, info, sizeof(struct work_notifier_s));
 
+      /* Disable interrupts very briefly. */
+
+      flags = enter_critical_section();
+
       /* Generate a unique key for this notification */
 
       notifier->key = work_notifier_key();

Review comment:
       Hi @davids5 
   
   > How long are interrupts off?
    it is  depends on the list size of "g_notifier_pinding", it seems good since there is just traverse of the list




----------------------------------------------------------------
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] davids5 commented on a change in pull request #2767: sched/wqueue/notifier: protect the work notifier with critical section

Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2767:
URL: https://github.com/apache/incubator-nuttx/pull/2767#discussion_r566213821



##########
File path: sched/wqueue/kwork_notifier.c
##########
@@ -256,6 +250,10 @@ int work_notifier_setup(FAR struct work_notifier_s *info)
 
       memcpy(&notifier->info, info, sizeof(struct work_notifier_s));
 
+      /* Disable interrupts very briefly. */
+
+      flags = enter_critical_section();
+
       /* Generate a unique key for this notification */
 
       notifier->key = work_notifier_key();

Review comment:
       Can this be moved out os the CS?
   
   https://github.com/apache/incubator-nuttx/blob/cdd111a1faec9b40b707797e00c4afae4956fb3f/sched/wqueue/kwork_notifier.c#L146
   Has a loop. 
   
   How long are interrupts off?




----------------------------------------------------------------
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] davids5 commented on a change in pull request #2767: sched/wqueue/notifier: protect the work notifier with critical section

Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2767:
URL: https://github.com/apache/incubator-nuttx/pull/2767#discussion_r566660451



##########
File path: sched/wqueue/kwork_notifier.c
##########
@@ -256,6 +250,10 @@ int work_notifier_setup(FAR struct work_notifier_s *info)
 
       memcpy(&notifier->info, info, sizeof(struct work_notifier_s));
 
+      /* Disable interrupts very briefly. */
+
+      flags = enter_critical_section();
+
       /* Generate a unique key for this notification */
 
       notifier->key = work_notifier_key();

Review comment:
       > it is depends on the list size of "g_notifier_pinding", it seems good since there is just traverse of the list
   
   That adds latency and jitters.
   
   Let do as @xiaoxiang781216  suggest and change to free running counter.




----------------------------------------------------------------
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] davids5 merged pull request #2767: sched/wqueue/notifier: protect the work notifier with critical section

Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #2767:
URL: https://github.com/apache/incubator-nuttx/pull/2767


   


----------------------------------------------------------------
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 a change in pull request #2767: sched/wqueue/notifier: protect the work notifier with critical section

Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #2767:
URL: https://github.com/apache/incubator-nuttx/pull/2767#discussion_r566696977



##########
File path: sched/wqueue/kwork_notifier.c
##########
@@ -256,6 +250,10 @@ int work_notifier_setup(FAR struct work_notifier_s *info)
 
       memcpy(&notifier->info, info, sizeof(struct work_notifier_s));
 
+      /* Disable interrupts very briefly. */
+
+      flags = enter_critical_section();
+
       /* Generate a unique key for this notification */
 
       notifier->key = work_notifier_key();

Review comment:
       Done




----------------------------------------------------------------
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] davids5 commented on a change in pull request #2767: sched/wqueue/notifier: protect the work notifier with critical section

Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2767:
URL: https://github.com/apache/incubator-nuttx/pull/2767#discussion_r566660451



##########
File path: sched/wqueue/kwork_notifier.c
##########
@@ -256,6 +250,10 @@ int work_notifier_setup(FAR struct work_notifier_s *info)
 
       memcpy(&notifier->info, info, sizeof(struct work_notifier_s));
 
+      /* Disable interrupts very briefly. */
+
+      flags = enter_critical_section();
+
       /* Generate a unique key for this notification */
 
       notifier->key = work_notifier_key();

Review comment:
       > it is depends on the list size of "g_notifier_pinding", it seems good since there is just traverse of the list
   
   That adds latency and jitter.
   
   Let's do as @xiaoxiang781216  suggest and change to free running counter.




----------------------------------------------------------------
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 a change in pull request #2767: sched/wqueue/notifier: protect the work notifier with critical section

Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #2767:
URL: https://github.com/apache/incubator-nuttx/pull/2767#discussion_r566600631



##########
File path: sched/wqueue/kwork_notifier.c
##########
@@ -256,6 +250,10 @@ int work_notifier_setup(FAR struct work_notifier_s *info)
 
       memcpy(&notifier->info, info, sizeof(struct work_notifier_s));
 
+      /* Disable interrupts very briefly. */
+
+      flags = enter_critical_section();
+
       /* Generate a unique key for this notification */
 
       notifier->key = work_notifier_key();

Review comment:
       Hi @davids5 
   
   > How long are interrupts off?
   
   
    it is  depends on the list size of "g_notifier_pinding", it seems good since there is just traverse of the list




----------------------------------------------------------------
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 a change in pull request #2767: sched/wqueue/notifier: protect the work notifier with critical section

Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #2767:
URL: https://github.com/apache/incubator-nuttx/pull/2767#discussion_r567167791



##########
File path: sched/wqueue/kwork_notifier.c
##########
@@ -143,24 +133,16 @@ static FAR struct work_notifier_entry_s *work_notifier_find(int16_t key)
  *
  ****************************************************************************/
 
-static int16_t work_notifier_key(void)
+static uint32_t work_notifier_key(void)
 {
-  int16_t key;
-
-  /* Loop until a unique key is generated.  Range 1-INT16_MAX. */
+  static uint32_t notifier_key;
 
-  do
+  if (++notifier_key == 0)

Review comment:
       Yes, it looks more clearer, please review 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] davids5 commented on a change in pull request #2767: sched/wqueue/notifier: protect the work notifier with critical section

Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #2767:
URL: https://github.com/apache/incubator-nuttx/pull/2767#discussion_r566794807



##########
File path: sched/wqueue/kwork_notifier.c
##########
@@ -143,24 +133,16 @@ static FAR struct work_notifier_entry_s *work_notifier_find(int16_t key)
  *
  ****************************************************************************/
 
-static int16_t work_notifier_key(void)
+static uint32_t work_notifier_key(void)
 {
-  int16_t key;
-
-  /* Loop until a unique key is generated.  Range 1-INT16_MAX. */
+  static uint32_t notifier_key;
 
-  do
+  if (++notifier_key == 0)

Review comment:
       Would it be better to make it obvious as to intent?
   
   ```suggestion
     ++notifier_key;
     if (notifier_key == 0) 
       {
         notifier_key = 1;
       }
   ```

##########
File path: sched/wqueue/kwork_notifier.c
##########
@@ -143,24 +133,16 @@ static FAR struct work_notifier_entry_s *work_notifier_find(int16_t key)
  *
  ****************************************************************************/
 
-static int16_t work_notifier_key(void)
+static uint32_t work_notifier_key(void)
 {
-  int16_t key;
-
-  /* Loop until a unique key is generated.  Range 1-INT16_MAX. */
+  static uint32_t notifier_key;

Review comment:
       ```suggestion
     static uint32_t notifier_key = 1;
   ```




----------------------------------------------------------------
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 a change in pull request #2767: sched/wqueue/notifier: protect the work notifier with critical section

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2767:
URL: https://github.com/apache/incubator-nuttx/pull/2767#discussion_r566652712



##########
File path: sched/wqueue/kwork_notifier.c
##########
@@ -256,6 +250,10 @@ int work_notifier_setup(FAR struct work_notifier_s *info)
 
       memcpy(&notifier->info, info, sizeof(struct work_notifier_s));
 
+      /* Disable interrupts very briefly. */
+
+      flags = enter_critical_section();
+
       /* Generate a unique key for this notification */
 
       notifier->key = work_notifier_key();

Review comment:
       can we switch to a free running counter?




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