All of lore.kernel.org
 help / color / mirror / Atom feed
* [kvmtool PATCH] net: Use vfork() instead of fork() for script execution
@ 2022-08-09 12:48 Suzuki K Poulose
  2022-08-17 10:18 ` Alexandru Elisei
  2022-09-22 12:45 ` Will Deacon
  0 siblings, 2 replies; 3+ messages in thread
From: Suzuki K Poulose @ 2022-08-09 12:48 UTC (permalink / raw)
  To: will; +Cc: kvm, alexandru.elisei, jean-philippe, maz, Suzuki K Poulose

When a script is specified for a guest nic setup, we fork() and execl()s
the script when it is time to execute the script. However this is not
optimal, given we are running a VM. The fork() will trigger marking the
entire page-table of the current process as CoW, which will trigger
unmapping the entire stage2 page tables from the guest. Anyway, the
child process will exec the script as soon as we fork(), making all
these mm operations moot. Also, this operation could be problematic
for confidential compute VMs, where it may be expensive (and sometimes
destructive) to make changes to the stage2 page tables.

So, instead we could use vfork() and avoid the CoW and unmap of the stage2.

Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 virtio/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virtio/net.c b/virtio/net.c
index c4e302bd..a5e0cea5 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -295,7 +295,7 @@ static int virtio_net_exec_script(const char* script, const char *tap_name)
 	pid_t pid;
 	int status;
 
-	pid = fork();
+	pid = vfork();
 	if (pid == 0) {
 		execl(script, script, tap_name, NULL);
 		_exit(1);
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [kvmtool PATCH] net: Use vfork() instead of fork() for script execution
  2022-08-09 12:48 [kvmtool PATCH] net: Use vfork() instead of fork() for script execution Suzuki K Poulose
@ 2022-08-17 10:18 ` Alexandru Elisei
  2022-09-22 12:45 ` Will Deacon
  1 sibling, 0 replies; 3+ messages in thread
From: Alexandru Elisei @ 2022-08-17 10:18 UTC (permalink / raw)
  To: Suzuki K Poulose; +Cc: will, kvm, jean-philippe, maz

Hi Suzuki,

On Tue, Aug 09, 2022 at 01:48:16PM +0100, Suzuki K Poulose wrote:
> When a script is specified for a guest nic setup, we fork() and execl()s
> the script when it is time to execute the script. However this is not
> optimal, given we are running a VM. The fork() will trigger marking the
> entire page-table of the current process as CoW, which will trigger
> unmapping the entire stage2 page tables from the guest. Anyway, the

This looks correct to me, virtio_notify_status() is called when the guest
writes to the status register, which in turn can end up calling
virtio_net_exec_script(). This happens when the guest is up and running,
when stage 2 has been created and populated.

> child process will exec the script as soon as we fork(), making all
> these mm operations moot. Also, this operation could be problematic
> for confidential compute VMs, where it may be expensive (and sometimes
> destructive) to make changes to the stage2 page tables.
> 
> So, instead we could use vfork() and avoid the CoW and unmap of the stage2.
> 
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  virtio/net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virtio/net.c b/virtio/net.c
> index c4e302bd..a5e0cea5 100644
> --- a/virtio/net.c
> +++ b/virtio/net.c
> @@ -295,7 +295,7 @@ static int virtio_net_exec_script(const char* script, const char *tap_name)
>  	pid_t pid;
>  	int status;
>  
> -	pid = fork();
> +	pid = vfork();
>  	if (pid == 0) {
>  		execl(script, script, tap_name, NULL);

This matches the man 2 page for vfork, which basically says that vfork can
be used only if the child immediately issues one of the exec family of
functions. Which is what is happening here.

The man page for vfork also says this:

"vfork() differs from fork(2) in that the calling thread is suspended until
the child terminates (either normally, by calling _exit(2), or abnormally,
after delivery  of a  fatal  signal), or it makes a call to execve(2)".

waitpid (which is invoked in the parent in the else branch) waits until the
child has changed state, which is defined in man 2 waitpid as:

"A state change is considered to be: the child terminated; the child was
stopped by a signal; or the child was resumed by a signal."

It doesn't mention anything about waitpid returning after the child make a
call to execve, so I guess it's correct to keep the call to waitpid in the
parent:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex

>  		_exit(1);
> -- 
> 2.37.1
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [kvmtool PATCH] net: Use vfork() instead of fork() for script execution
  2022-08-09 12:48 [kvmtool PATCH] net: Use vfork() instead of fork() for script execution Suzuki K Poulose
  2022-08-17 10:18 ` Alexandru Elisei
@ 2022-09-22 12:45 ` Will Deacon
  1 sibling, 0 replies; 3+ messages in thread
From: Will Deacon @ 2022-09-22 12:45 UTC (permalink / raw)
  To: Suzuki K Poulose
  Cc: catalin.marinas, kernel-team, Will Deacon, jean-philippe, kvm,
	alexandru.elisei, maz

On Tue, 9 Aug 2022 13:48:16 +0100, Suzuki K Poulose wrote:
> When a script is specified for a guest nic setup, we fork() and execl()s
> the script when it is time to execute the script. However this is not
> optimal, given we are running a VM. The fork() will trigger marking the
> entire page-table of the current process as CoW, which will trigger
> unmapping the entire stage2 page tables from the guest. Anyway, the
> child process will exec the script as soon as we fork(), making all
> these mm operations moot. Also, this operation could be problematic
> for confidential compute VMs, where it may be expensive (and sometimes
> destructive) to make changes to the stage2 page tables.
> 
> [...]

Applied to kvmtool (master), thanks!

[1/1] net: Use vfork() instead of fork() for script execution
      https://git.kernel.org/will/kvmtool/c/9987a37cfc57

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-09-22 12:46 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-09 12:48 [kvmtool PATCH] net: Use vfork() instead of fork() for script execution Suzuki K Poulose
2022-08-17 10:18 ` Alexandru Elisei
2022-09-22 12:45 ` Will Deacon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.