[pve-devel] [PATCH qemu-server v2 1/2] pci: add helpers to (un)reserve pciids for a vm

[Thomas Lamprecht]

On 07.10.21 11:37, Dominik Csapak wrote:
> saves a list of pciid <-> vmid mappings in /var/run
> that we can check when we start a vm
> 
> if we're not given a pid but a timeout, we save the time when the
> reservation will run out (current time + timeout + 5s) since each
> vm start (until we can save the pid) varies from config to config
> 

a few comments, some of them could have happend for v1 so sorry for any
churn..

> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/QemuServer/PCI.pm | 99 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
> index 5608207..364ceb7 100644
> --- a/PVE/QemuServer/PCI.pm
> +++ b/PVE/QemuServer/PCI.pm
> @@ -5,6 +5,7 @@ use strict;
>  
>  use PVE::JSONSchema;
>  use PVE::SysFSTools;
> +use PVE::Tools;
>  
>  use base 'Exporter';
>  
> @@ -461,4 +462,102 @@ sub print_hostpci_devices {
>      return ($kvm_off, $gpu_passthrough, $legacy_igd);
>  }
>  
> +my $PCIID_RESERVATION_FILE = "/var/run/pve-reserved-pciids";
> +my $PCIID_RESERVATION_LCK = "/var/lock/pve-reserved-pciids.lck";
> +
> +my $parse_pci_reservation = sub {
> +    my $pciids = {};
> +
> +    if (my $fh = IO::File->new ($PCIID_RESERVATION_FILE, "r")) {
> +	while (my $line = <$fh>) {
> +	    if ($line =~ m/^($PCIRE)\s(\d+)\stime\:(\d+)$/) {
> +		$pciids->{$1} = {
> +		    vmid => $2,
> +		    timestamp => $3,
> +		};
> +	    } elsif ($line =~ m/^($PCIRE)\s(\d+)\spid\:(\d+)$/s) {
> +		$pciids->{$1} = {
> +		    vmid => $2,
> +		    pid => $3,
> +		};
> +	    }

could probably be (untested):

if ($line =~ m/^($PCIRE)\s(\d+)\s(time|pid)\:(\d+)$/) {
    $pciids->{$1} = {
        vmid => $2,
        "$3" => $4,
    };
}

Alternative: use a format like "$vmid $timestamp [$pid]"

we'd keep the time of initial reservation info that way, not sure how much use that
has though ^^

> +	}
> +    }
> +
> +    return $pciids;
> +};
> +
> +my $write_pci_reservation = sub {
> +    my ($pciids) = @_;
> +
> +    my $data = "";
> +    foreach my $p (keys %$pciids) {
> +	my $entry = $pciids->{$p};
> +	if (defined($entry->{pid})) {
> +	    $data .= "$p $entry->{vmid} pid:$entry->{pid}\n";
> +	} else {
> +	    $data .= "$p $entry->{vmid} time:$entry->{timestamp}\n";
> +	}
> +    }
> +
> +    PVE::Tools::file_set_contents($PCIID_RESERVATION_FILE, $data);
> +};
> +
> +sub remove_pci_reservation {
> +    my ($id) = @_;

it could be an optimization to allow passing a array ref of IDs, less open+writes+rename,
while thanks to /run being a tmpfs no IO is happening but syscalls are expensive, so
when they're as easy to reduce as here I'd opt for that. Could do the same in a similar
spirit for registering then.

> +
> +    my $code = sub {
> +	my $pciids = $parse_pci_reservation->();
> +
> +	delete $pciids->{$id};
> +
> +	$write_pci_reservation->($pciids);
> +    };
> +
> +    PVE::Tools::lock_file($PCIID_RESERVATION_LCK, 10, $code);
> +    die $@ if $@;
> +
> +    return;
> +}
> +
> +sub reserve_pci_usage {
> +    my ($id, $vmid, $timeout, $pid) = @_;
> +
> +    PVE::Tools::lock_file($PCIID_RESERVATION_LCK, 10, sub {
> +
> +	my $ctime = time();
> +	my $errmsg = "PCI device '$id' already in use\n";
> +
> +	my $pciids = $parse_pci_reservation->();
> +
> +	if (my $pciid = $pciids->{$id}) {
> +	    if ($pciid->{vmid} != $vmid) {
> +		# check time based reservation
> +		die $errmsg if defined($pciid->{timestamp}) && $pciid->{timestamp} > $ctime;
> +
> +		# check running vm
> +		my $pid = PVE::QemuServer::Helpers::vm_running_locally($pciid->{vmid});
> +		die $errmsg if defined($pciid->{pid}) && $pid == $pciid->{pid};

Maybe we should warn in the != case, as that would mean a registration was not
cleaned up nicely and recording that in the task log should not hurt.

> +	    }
> +	}
> +
> +	$pciids->{$id} = {
> +	    vmid => $vmid,
> +	};
> +	if (defined($timeout)) {
> +	    # we save the timestamp when the reservation gets invalid (+5s),
> +	    # since the start timeout depends on the config
> +	    $pciids->{$id}->{timestamp} = $ctime + $timeout + 5;
> +	}
> +	$pciids->{$id}->{pid} = $pid if defined($pid);
> +
> +	$write_pci_reservation->($pciids);
> +
> +	return;
> +    });
> +    die $@ if $@;
> +
> +    return;
> +}
> +
>  1;
>