Re: [PATCH v4 3/5] dts: add class for virtual functions

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On 23. 9. 2024 20:42, jspewock@iol.unh.edu wrote:
> From: Jeremy Spewock <jspewock@iol.unh.edu>
> 
> In DPDK applications virtual functions are treated the same as ports,
> but within the framework there are benefits to differentiating the two
> in order to add more metadata to VFs about where they originate from.
> For this reason this patch adds a new class for handling virtual
> functions that extends the Port class with some additional information
> about the VF.
> 
> Bugzilla ID: 1500
> 
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>   dts/framework/testbed_model/port.py | 37 ++++++++++++++++++++++++++++-
>   1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/dts/framework/testbed_model/port.py b/dts/framework/testbed_model/port.py
> index 817405bea4..c1d85fec2b 100644
> --- a/dts/framework/testbed_model/port.py
> +++ b/dts/framework/testbed_model/port.py
> @@ -27,7 +27,7 @@ class PortIdentifier:
>       pci: str
>   
>   
> -@dataclass(slots=True)
> +@dataclass

This should be explained in the commit message.

>   class Port:
>       """Physical port on a node.
>   
> @@ -80,6 +80,41 @@ def pci(self) -> str:
>           return self.identifier.pci
>   
>   
> +@dataclass
> +class VirtualFunction(Port):
> +    """Virtual Function (VF) on a port.
> +
> +    DPDK applications often treat VFs the same as they do the physical ports (PFs) on the host.
> +    For this reason VFs are represented in the framework as a type of port with some additional
> +    metadata that allows the framework to more easily identify which device the VF belongs to as
> +    well as where the VF originated from.
> +
> +    Attributes:
> +        created_by_framework: :data:`True` if this VF represents one that the DTS framework created
> +            on the node, :data:`False` otherwise.

I had to go look in the other patches to understand why this is here. 
The patch split should be redone along logical lines (one patch should 
contain related logic, now the related logic is in basically all 
patches), not files (that doesn't help with review and it's also not 
going to result in better git history).

But I figured out this is here because of cleanup. It makes more sense 
to me for framework to remember whether it created the port or not as 
opposed to port remembering it, especially when the framework is doing 
the cleanup and not the ports.

> +        pf_port: The PF that this VF was created on/gathered from.

Maybe it would make more sense to store the VF ports in the PF port. If 
we need to use VF ports, we can just refer to the PF port which has the 
benefit making it easier to use the proper link between ports.

And the framework can store which PF ports needs VF cleanup instead of 
storing which VFs needs cleaning.

> +    """
> +
> +    created_by_framework: bool = False
> +    pf_port: Port | None = None
> +
> +    def __init__(
> +        self, node_name: str, config: PortConfig, created_by_framework: bool, pf_port: Port
> +    ) -> None:
> +        """Extends :meth:`Port.__init__` with VF specific metadata.
> +
> +        Args:
> +            node_name: The name of the node the VF resides on.
> +            config: Configuration information about the VF.
> +            created_by_framework: :data:`True` if DTS created this VF, otherwise :data:`False` if
> +                this class represents a VF that was preexisting on the node.
> +            pf_port: The PF that this VF was created on/gathered from.
> +        """
> +        super().__init__(node_name, config)
> +        self.created_by_framework = created_by_framework
> +        self.pf_port = pf_port
> +
> +
>   @dataclass(slots=True, frozen=True)
>   class PortLink:
>       """The physical, cabled connection between the ports.