From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 86E3071D34 for ; Tue, 5 Oct 2021 16:14:48 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 733A0291F7 for ; Tue, 5 Oct 2021 16:14:18 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id C762B291EB for ; Tue, 5 Oct 2021 16:14:17 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 955974567A for ; Tue, 5 Oct 2021 16:14:17 +0200 (CEST) Message-ID: <90c1f81e-6cc0-36ce-9e84-d72ad7e7b578@proxmox.com> Date: Tue, 5 Oct 2021 16:13:07 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:93.0) Gecko/20100101 Thunderbird/93.0 Content-Language: en-US To: Proxmox VE development discussion , Dominik Csapak References: <20211005131200.791836-1-d.csapak@proxmox.com> <20211005131200.791836-3-d.csapak@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20211005131200.791836-3-d.csapak@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.225 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [pci.pm] Subject: Re: [pve-devel] [PATCH qemu-server 2/3] pci: add helpers to (un)reserve pciids for a vm X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Oct 2021 14:14:48 -0000 On 05.10.21 15:11, Dominik Csapak wrote: > saves a list of pciid <-> vmid mappings in /var/run > that we can check when we start a vm a few style nits but also one serious note inline >=20 > Signed-off-by: Dominik Csapak > --- > PVE/QemuServer/PCI.pm | 89 +++++++++++++++++++++++++++++++++++++++++++= > 1 file changed, 89 insertions(+) >=20 > diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm > index 5608207..0626b76 100644 > --- a/PVE/QemuServer/PCI.pm > +++ b/PVE/QemuServer/PCI.pm > @@ -5,6 +5,7 @@ use strict; > =20 > use PVE::JSONSchema; > use PVE::SysFSTools; > +use PVE::Tools; > =20 > use base 'Exporter'; > =20 > @@ -461,4 +462,92 @@ sub print_hostpci_devices { > return ($kvm_off, $gpu_passthrough, $legacy_igd); > } > =20 > +my $PCIID_RESERVATION_FILE =3D "/var/run/pve-reserved-pciids"; > +my $PCIID_RESERVATION_LCK =3D "/var/lock/pve-reserved-pciids.lck"; > + > +my $parse_pci_reservation =3D sub { > + my $pciids =3D {}; > + > + if (my $fh =3D IO::File->new ($PCIID_RESERVATION_FILE, "r")) { > + while (my $line =3D <$fh>) { > + if ($line =3D~ m/^($PCIRE)\s(\d+)\s(\d+)$/) { > + $pciids->{$1} =3D { > + timestamp =3D> $2, > + vmid =3D> $3, > + }; > + } > + } > + } > + > + return $pciids; > +}; > + > +my $write_pci_reservation =3D sub { > + my ($pciids) =3D @_; > + > + my $data =3D ""; > + foreach my $p (keys %$pciids) { prefer for over foreach > + $data .=3D "$p $pciids->{$p}->{timestamp} $pciids->{$p}->{vmid}\n"; > + } my $data =3D join("\n", map { "$_ $pciids->{$_}->{timestamp} $pciids->{$_= }->{vmid}" }, keys $pciids->%*); > + > + PVE::Tools::file_set_contents($PCIID_RESERVATION_FILE, $data); > +}; > + > +sub remove_pci_reservation { > + my ($id) =3D @_; > + > + my $code =3D sub { > + my $pciids =3D $parse_pci_reservation->(); > + > + delete $pciids->{$id}; > + > + $write_pci_reservation->($pciids); > + }; > + > + PVE::Tools::lock_file($PCIID_RESERVATION_LCK, 10, $code); IMO it has some benefit for passing the closure directly, less lines and = slightly more obvious about the locking (as at least I read methods from top to bo= ttom): PVE::Tools::lock_file($PCIID_RESERVATION_LCK, 10, sub { my $pciids =3D $parse_pci_reservation->(); ... }); but we have no clear style guide regarding this and def. use both variant= s, so no hard feelings here. > + die $@ if $@; > + > + return; > +} > + > +sub reserve_pci_usage { > + my ($id, $vmid) =3D @_; > + > + my $code =3D sub { > + > + # have a 60 second grace period, since we reserve before > + # we actually start the vm huh, whats the use on that? so I can "steal" PCI devices the first 60s, f= eels weird... Why not either: * catch any start error somewhere centrally and clear the reservation in = that case again, a kill/crash could still result into false-positives though= * save timestamp now and then once we know it the PID of the VM as third param, VMID + PID are quite good in being resistent against PID-reuse and an future start could check if the process still lives to decide if the reservation is still valid > + my $graceperiod =3D 60; > + my $ctime =3D time(); > + > + my $pciids =3D $parse_pci_reservation->(); > + > + if (my $pciid =3D $pciids->{$id}) { > + if ($pciid->{vmid} =3D=3D $vmid) { > + return; # already reserved > + } I'd prefer a onliner for above, less lines/noise while not yet being code-golfy, so easier to read IMO. i.e.: return if $pciid->{vmid} =3D=3D $vmid; # already reserved > + > + if (($pciid->{timestamp} + $graceperiod > $ctime) || > + PVE::QemuServer::Helpers::vm_running_locally($vmid)) > + { style nit=C2=B2, we (nowadays) normally place the if's closing ) also on = the new line: if (($pciid->{timestamp} + $graceperiod > $ctime) || PVE::QemuServer::Helpers::vm_running_locally($vmid) ) { .... } honestly I'd like it 1000% more the rust way, but well..