[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6320: use rmutex inside of all repeated implementation

[GitBox <gi...@apache.org>]

pkarashchenko commented on code in PR #6320:
URL: https://github.com/apache/incubator-nuttx/pull/6320#discussion_r895219754


##########
drivers/syslog/syslog_device.c:
##########
@@ -123,63 +120,6 @@ static const uint8_t g_syscrlf[2] =
  * Private Functions
  ****************************************************************************/
 
-/****************************************************************************
- * Name: syslog_dev_takesem
- ****************************************************************************/
-
-static inline int syslog_dev_takesem(FAR struct syslog_dev_s *syslog_dev)
-{
-  pid_t me = getpid();
-  int ret;
-
-  /* Does this thread already hold the semaphore?  That could happen if
-   * we were called recursively, i.e., if the logic kicked off by
-   * file_write() where to generate more debug output.  Return an
-   * error in that case.
-   */
-
-  if (syslog_dev->sl_holder == me)
-    {
-      /* Return an error (instead of deadlocking) */
-
-      return -EWOULDBLOCK;
-    }
-
-  /* Either the semaphore is available or is currently held by another
-   * thread.  Wait for it to become available.
-   */
-
-  ret = nxsem_wait(&syslog_dev->sl_sem);
-  if (ret < 0)
-    {
-      return ret;
-    }
-
-  /* We hold the semaphore.  We can safely mark ourself as the holder
-   * of the semaphore.
-   */
-
-  syslog_dev->sl_holder = me;
-  return OK;
-}

Review Comment:
   @xiaoxiang781216 I observe deadlock after I migrate my application to the latest NuttX master. I narrow down the issue and see that it happens when multiple tasks try to use `syslog`. I have a syslog configuration with 2 channels:
   1. Channel attached to console
   2. Channel attached to file on `/mnt/sdcard0/nuttx.log`
   When the issue happens I observe next picture:
   ![Screenshot from 2022-06-12 20-09-50](https://user-images.githubusercontent.com/17704713/173248434-507ad3e4-2f6e-459f-978b-b5288752bf2a.png)
   
   I've noticed that before this change `syslog_dev_takesem` returned `return -EWOULDBLOCK;` with a comment `/* Return an error (instead of deadlocking) */` and seems like this functionality has been broken now.
   
   In general I think that we rush too much with replacing of semaphores with mutexes. For example the FLAT build functioning changed after this change since all `nxsem_` API calls became replaced with `sem_` API calls and introduced unnecessary cancellation points in kernel. I'm not sure about the side effect of this, but still investigating.
   I think we need to revert part of the changes from this PR especially syslog part and part related to replacement in libc and get back `nxmutex_` to use `nxsem_` APIs only and use it in kernel part only for now. Then make a step-by-step change to user space.



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