Skip to content

Commit

Permalink
sched/taskfiles: skip unnecessary file open/close operations to impro…
Browse files Browse the repository at this point in the history
…ve 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 <[email protected]>
  • Loading branch information
anchao committed Nov 16, 2023
1 parent 7b1969b commit 5add009
Show file tree
Hide file tree
Showing 17 changed files with 390 additions and 104 deletions.
1 change: 1 addition & 0 deletions binfmt/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ list(
binfmt_execmodule.c
binfmt_exec.c
binfmt_copyargv.c
binfmt_copyactions.c
binfmt_dumpmodule.c
binfmt_coredump.c)

Expand Down
2 changes: 1 addition & 1 deletion binfmt/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
45 changes: 45 additions & 0 deletions binfmt/binfmt.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
194 changes: 194 additions & 0 deletions binfmt/binfmt_copyactions.c
Original file line number Diff line number Diff line change
@@ -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
34 changes: 15 additions & 19 deletions binfmt/binfmt_execmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 */

Expand All @@ -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);
Expand Down Expand Up @@ -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)
Expand All @@ -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);

Expand Down Expand Up @@ -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);
Expand All @@ -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)
Expand Down Expand Up @@ -442,8 +436,10 @@ int exec_module(FAR struct binary_s *binp,
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);
Expand Down
Loading

0 comments on commit 5add009

Please sign in to comment.