sched/taskfiles: skip unnecessary file open/close operations to improve performance

The task files should consult the "spawn action" and "O_CLOEXEC flags"
to determine further whether the file should be duplicated.

This PR will further optimize file list duplicating to avoid the performance
regression caused by additional file operations.

Signed-off-by: chao an <anchao@xiaomi.com>

binfmt/CMakeLists.txt

@@ -36,6 +36,7 @@ list(
   binfmt_execmodule.c
   binfmt_exec.c
   binfmt_copyargv.c
+  binfmt_copyactions.c
   binfmt_dumpmodule.c
   binfmt_coredump.c)
 

binfmt/Makefile

@@ -24,7 +24,7 @@ include $(TOPDIR)/Make.defs
 
 CSRCS  = binfmt_globals.c binfmt_initialize.c binfmt_register.c binfmt_unregister.c
 CSRCS += binfmt_loadmodule.c binfmt_unloadmodule.c binfmt_execmodule.c
-CSRCS += binfmt_exec.c binfmt_copyargv.c binfmt_dumpmodule.c
+CSRCS += binfmt_exec.c binfmt_copyargv.c binfmt_copyactions.c binfmt_dumpmodule.c
 CSRCS += binfmt_coredump.c
 
 ifeq ($(CONFIG_BINFMT_LOADABLE),y)

binfmt/binfmt.h

@@ -165,6 +165,51 @@ void binfmt_freeargv(FAR char * const *argv);
 #  define binfmt_freeenv(envp)
 #endif
 
+/****************************************************************************
+ * Name: binfmt_copyactions
+ *
+ * Description:
+ *   In the kernel build, the file actions will likely lie in the caller's
+ *   address environment and, hence, be inaccessible when we switch to the
+ *   address environment of the new process address environment.  So we
+ *   do not have any real option other than to copy the callers action list.
+ *
+ * Input Parameters:
+ *   copy     - Pointer of file actions
+ *
+ * Returned Value:
+ *   A non-zero copy is returned on success.
+ *
+ ****************************************************************************/
+
+#if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_BUILD_KERNEL)
+int binfmt_copyactions(FAR const posix_spawn_file_actions_t **copy,
+                       FAR const posix_spawn_file_actions_t *actions);
+#else
+#  define binfmt_copyactions(copy, actp) \
+          (*(copy) = (FAR posix_spawn_file_actions_t *)(actp), 0)
+#endif
+
+/****************************************************************************
+ * Name: binfmt_freeactions
+ *
+ * Description:
+ *   Release the copied file action list.
+ *
+ * Input Parameters:
+ *   copy     - Pointer of file actions
+ *
+ * Returned Value:
+ *   None
+ *
+ ****************************************************************************/
+
+#if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_BUILD_KERNEL)
+void binfmt_freeactions(FAR const posix_spawn_file_actions_t *copy);
+#else
+#  define binfmt_freeactions(copy)
+#endif
+
 #ifdef CONFIG_BUILTIN
 /****************************************************************************
  * Name: builtin_initialize

binfmt/binfmt_copyactions.c

@@ -0,0 +1,194 @@
+/****************************************************************************
+ * binfmt/binfmt_copyactions.c
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <string.h>
+#include <debug.h>
+#include <errno.h>
+
+#include <nuttx/kmalloc.h>
+#include <nuttx/binfmt/binfmt.h>
+
+#include "binfmt.h"
+
+#if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_BUILD_KERNEL) && !defined(CONFIG_BINFMT_DISABLE)
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define ROUNDUP(x, y)     (((x) + (y) - 1) / (y) * (y))
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: binfmt_copyactions
+ *
+ * Description:
+ *   In the kernel build, the file actions will likely lie in the caller's
+ *   address environment and, hence, be inaccessible when we switch to the
+ *   address environment of the new process address environment.  So we
+ *   do not have any real option other than to copy the callers action list.
+ *
+ * Input Parameters:
+ *   copy     - Pointer of file actions
+ *
+ * Returned Value:
+ *   A non-zero copy is returned on success.
+ *
+ ****************************************************************************/
+
+int binfmt_copyactions(FAR const posix_spawn_file_actions_t **copy,
+                       FAR const posix_spawn_file_actions_t *actions)
+{
+  FAR struct spawn_general_file_action_s *entry;
+  FAR struct spawn_general_file_action_s *prev;
+  FAR struct spawn_close_file_action_s *close;
+  FAR struct spawn_open_file_action_s *open;
+  FAR struct spawn_open_file_action_s *tmp;
+  FAR struct spawn_dup2_file_action_s *dup2;
+  FAR void *buffer;
+  int size = 0;
+
+  if (actions == NULL)
+    {
+      *copy = NULL;
+      return OK;
+    }
+
+  for (entry = (FAR struct spawn_general_file_action_s *)actions;
+       entry != NULL;
+       entry = entry->flink)
+    {
+      switch (entry->action)
+        {
+          case SPAWN_FILE_ACTION_CLOSE:
+            size += sizeof(struct spawn_close_file_action_s);
+            break;
+
+          case SPAWN_FILE_ACTION_DUP2:
+            size += sizeof(struct spawn_dup2_file_action_s);
+            break;
+
+          case SPAWN_FILE_ACTION_OPEN:
+            open = (FAR struct spawn_open_file_action_s *)entry;
+            size += ROUNDUP(SIZEOF_OPEN_FILE_ACTION_S(strlen(open->path)),
+                            sizeof(FAR void *));
+            break;
+
+          default:
+            return -EINVAL;
+        }
+    }
+
+  *copy = buffer = kmm_malloc(size);
+  if (buffer == NULL)
+    {
+      return -ENOMEM;
+    }
+
+  for (entry = (FAR struct spawn_general_file_action_s *)actions,
+       prev = NULL; entry != NULL; prev = entry, entry = entry->flink)
+    {
+      switch (entry->action)
+        {
+          case SPAWN_FILE_ACTION_CLOSE:
+            close = buffer;
+            memcpy(close, entry, sizeof(struct spawn_close_file_action_s));
+            close->flink = NULL;
+            if (prev)
+              {
+                prev->flink = (FAR void *)close;
+              }
+
+            buffer = close + 1;
+            break;
+
+          case SPAWN_FILE_ACTION_DUP2:
+            dup2 = buffer;
+            memcpy(dup2, entry, sizeof(struct spawn_dup2_file_action_s));
+            dup2->flink = NULL;
+            if (prev)
+              {
+                prev->flink = (FAR void *)dup2;
+              }
+
+            buffer = dup2 + 1;
+            break;
+
+          case SPAWN_FILE_ACTION_OPEN:
+            tmp = (FAR struct spawn_open_file_action_s *)entry;
+            open = buffer;
+            memcpy(open, entry, sizeof(struct spawn_open_file_action_s));
+            open->flink = NULL;
+            if (prev)
+              {
+                prev->flink = (FAR void *)open;
+              }
+
+            strcpy(open->path, tmp->path);
+
+            buffer = (FAR char *)buffer +
+                     ROUNDUP(SIZEOF_OPEN_FILE_ACTION_S(strlen(tmp->path)),
+                             sizeof(FAR void *));
+            break;
+
+          default:
+            break;
+        }
+    }
+
+  return OK;
+}
+
+/****************************************************************************
+ * Name: binfmt_freeactions
+ *
+ * Description:
+ *   Release the copied file action list.
+ *
+ * Input Parameters:
+ *   copy     - Pointer of file actions
+ *
+ * Returned Value:
+ *   None
+ *
+ ****************************************************************************/
+
+void binfmt_freeactions(FAR const posix_spawn_file_actions_t *copy)
+{
+  /* Is there an allocated argument buffer */
+
+  if (copy != NULL)
+    {
+      /* Free the argument buffer */
+
+      kmm_free((FAR void *)copy);
+    }
+}
+
+#endif

binfmt/binfmt_execmodule.c

@@ -256,6 +256,12 @@ int exec_module(FAR struct binary_s *binp,
       goto errout_with_args;
     }
 
+  ret = binfmt_copyactions(&actions, actions);
+  if (ret < 0)
+    {
+      goto errout_with_envp;
+    }
+
 #if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_BUILD_KERNEL)
   /* If there is no argument vector, the process name must be copied here */
 
@@ -271,7 +277,7 @@ int exec_module(FAR struct binary_s *binp,
   if (ret < 0)
     {
       berr("ERROR: addrenv_select() failed: %d\n", ret);
-      goto errout_with_envp;
+      goto errout_with_actions;
     }
 
   ret = up_addrenv_vheap(addrenv, &vheap);
@@ -310,12 +316,14 @@ int exec_module(FAR struct binary_s *binp,
   if (argv && argv[0])
     {
       ret = nxtask_init(tcb, argv[0], binp->priority, stackaddr,
-                        binp->stacksize, binp->entrypt, &argv[1], envp);
+                        binp->stacksize, binp->entrypt, &argv[1],
+                        envp, actions);
     }
   else
     {
       ret = nxtask_init(tcb, filename, binp->priority, stackaddr,
-                        binp->stacksize, binp->entrypt, argv, envp);
+                        binp->stacksize, binp->entrypt, argv,
+                        envp, actions);
     }
 
   if (ret < 0)
@@ -326,6 +334,7 @@ int exec_module(FAR struct binary_s *binp,
 
   /* The copied argv and envp can now be released */
 
+  binfmt_freeactions(actions);
   binfmt_freeargv(argv);
   binfmt_freeenv(envp);
 
@@ -376,10 +385,6 @@ int exec_module(FAR struct binary_s *binp,
     }
 #endif
 
-  /* Close the file descriptors with O_CLOEXEC before active task */
-
-  files_close_onexec(&tcb->cmn);
-
   if (!spawn)
     {
       exec_swap(this_task(), (FAR struct tcb_s *)tcb);
@@ -400,17 +405,6 @@ int exec_module(FAR struct binary_s *binp,
     }
 #endif
 
-  /* Perform file actions */
-
-  if (actions != NULL)
-    {
-      ret = spawn_file_actions(&tcb->cmn, actions);
-      if (ret < 0)
-        {
-          goto errout_with_tcbinit;
-        }
-    }
-
   /* Set the attributes */
 
   if (attr)
@@ -442,8 +436,10 @@ errout_with_tcbinit:
 errout_with_addrenv:
 #if defined(CONFIG_ARCH_ADDRENV) && defined(CONFIG_BUILD_KERNEL)
   addrenv_restore(binp->oldenv);
-errout_with_envp:
+errout_with_actions:
+  binfmt_freeactions(actions);
 #endif
+errout_with_envp:
   binfmt_freeenv(envp);
 errout_with_args:
   binfmt_freeargv(argv);