Message-ID: <VI1PR0501MB227136BD07607006599D1F71D1410@VI1PR0501MB2271.eurprd05.prod.outlook.com>
Date:   Wed, 20 Mar 2019 23:39:26 +0000
From:   Parav Pandit <parav@...lanox.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     Jiri Pirko <jiri@...nulli.us>,
        "Samudrala, Sridhar" <sridhar.samudrala@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "oss-drivers@...ronome.com" <oss-drivers@...ronome.com>
Subject: RE: [PATCH net-next v2 4/7] devlink: allow subports on devlink PCI
 ports



> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Sent: Wednesday, March 20, 2019 3:23 PM
> To: Parav Pandit <parav@...lanox.com>
> Cc: Jiri Pirko <jiri@...nulli.us>; Samudrala, Sridhar
> <sridhar.samudrala@...el.com>; davem@...emloft.net;
> netdev@...r.kernel.org; oss-drivers@...ronome.com
> Subject: Re: [PATCH net-next v2 4/7] devlink: allow subports on devlink PCI
> ports
> 
> On Wed, 20 Mar 2019 18:24:15 +0000, Parav Pandit wrote:
> > Hi Jiri, Jakub, Samudrala Sridhar,
> > > > > > > And physical port in include/uapi/linux/devlink.h also
> > > > > > > describe that.
> > > > > >
> > > > > > By "that" you must mean that the physical is a user facing port.
> > > > >
> > > > > Can you please describe the difference between 'PF port' and
> > > > > 'physical port of include/uapi/linux/devlink.h'? I must have
> > > > > missed this crisp definition in discussion between you and Jiri.
> > > > > I am in meantime checking the thread.
> > > >
> > > > Perhaps start with the cover letter which includes an ASCII drawing?
> > > >
> > > > Using Mellanox nomenclature - PF port is a "representor" for the
> > > > PF which may be on another Host (SmartNIC or multihost).  It's
> > > > pretty much the same thing as a VF port/"representor".
> > > >
> > > Yes. We are aligned here. :-)
> > > I see your point, where in multi-host scenario, a physical port may
> > > be 1, but PF ports are 4, because of 4 PFs for 4 hosts.
> > > (just an example of 4 hosts with their own mac address sharing 1
> > > physical port).
> > >
> > > When there is no multihost and one to one mapping between a PF and
> > > physical links, there is some overlap between PF port and physical
> > > port attributes.
> > > I believe, such overlap is fine as long as we have unique indices for the
> ports.
> > >
> > > So I am ok to have flavours as physical/cpu/dsa/pf/vf/mdev/switchport.
> > > (last 4 as new port flavours).
> > >
> > > > Physical port is the hole on the panel of the adapter where cable goes.
> >
> > So my take away from above discussion are:
> > 1. Following new port flavours should be added
> pci_pf/pci_vf/mdev/switchport.
> > a. Switchport indicates port on the eswitch. Normally this port has rep-
> netdev attached to it.
> 
> I don't understand the "switchport".  Surely physical ports are also attached
> to the eswitch?  
Yes, physical port is attached to eswitch too.
switchport is the one which is not directly visible to the user on the physical panel.
Jiri captured that in the diagram visible here.

[1] https://www.mail-archive.com/netdev@vger.kernel.org/msg289477.html

> And one of the main purpose of adding the pci_pf/pci_vf
> flavours was to generate phys_port_name for the port netdevs.
> 
Yes, this is surely needed due to current direction of placement of pf/vf info in rep-netdev phys_port_name.
So this information is available from the past example in thread.
I copied here and extended some parts of it.

Below example is for one PCI function that has two physical ports, one VF, and one mdev.
It enumerates 6 devlink ports.
VF 1 is physical port 1 of PF 0.
mdev uuidX's port 0 is connected to PF 0, port 0.

$ devlink port show
pci/0000:05:00.0/0 eth netdev repndev_pf0_p0 flavour physical switch_id 00154d130d2f
pci/0000:05:00.0/1 eth netdev repndev_pf0_p1 flavour physical switch_id 00154d130d2f
pci/0000:05:00.0/10001 eth netdev repndev_pf0_vf_1 flavour switchport switch_id 00154d130d2f peer pci/0000:05:00.0/1
pci/0000:05:00.0/10002 eth netdev repndev_pf0_p0_mdev_8000 flavour switchport switch_id 00154d130d2f peer mdev/uuidX/0

pci/0000:05:00.0/1 eth netdev flavour vf_ctrl vf 1
mdev/uuidX/0 eth netdev flavour mdev_ctrl

Here eswitch side creates phys_port_name from the peer port's information.
Something like,

struct devlink_peer_info {
	struct devlink *dev;
	struct devlink_port *port;
};

struct devlink_port {
	[...] existing fields;
	struct devlink_peer_info peer_info;
};

Depending on peer port flavour, it can construct appropriate phys_port_name ndo_ops return value.
I changed port flavour name to vf_ctrl and  mdev_ctrl to make it more explicit for clarity.

Let's discuss what is missing in this model.. or part that's incorrect...

> Please don't use the term representor if possible.  Representor for most
> developers describes the way the netdev is implemented in the driver, so for
> Mellanox and Netronome different ports will be representors and non-
> representors.  That's why I prefer port netdev (attached to eswitch, has
> switch_id) and host netdev (PF/VF netdev, vNIC, VSI, etc).
> 
Frankly I don't see much difference in both the proposals.
No matter which way we go, peer_info is needed anyway for vf/pf/mdev.
Only exception is introduction of explicit host_port_side object (instead of indirect peer way).
This gives clear visibility to users and addresses rdma non_sriov case too.

> > b. host side port flavours are pci_pf/pci_vf/mdev which may be
> > connected to switchport
> 
> See above, pci_pf/pci_vf are needed for phys_port_name generation.
> 
> > 2. host side port flavours are not limited to Ethernet, as it is for devlink's
> port instance.
> >
> > 3. Each port is continue to be accessed using unique port index.
> >
> > 4. host side ports and switchport are control objects.
> > a. switch side ports reside where current eswitch object of devlink
> > instance reside b. for a given VF/PF/mdev such host side ports may be
> > in hypervisor or VM or both depending on the privilege
> >
> > 5. eth.mac_address, rdma.port_guid can be programmed at host port
> > flavours by extending as $ devlink port param set...
> > (similar to devlink dev param set)
> 
> You can keep restating that's your position, but I have *not* conceded to
> that.
> 
I understand. Lets discuss any short comings of it (if any) due to which switch side flavours to be defined via indirect peer programming.

> > 6. more host port params can be added in future when user need arise
> >
> > 7. rep-netdev continue to be eswitch (switchport) representor at the switch
> side.
> > a. Hence rep-netdev cannot be used for programming host port's
> parameters.
> >
> > 8. eswitch devlink instance knows when VF/PF/mdev's switchport are
> created/removed.
> > Hence, those will be created/deleted by eswitch.
> > Similarly for host port flavours too.
> >
> > Does it look fine? Did I miss something?
> > We would like to progress on incremental patches for item-4 and any
> > prep work needed to reach to item-4.

Powered by blists - more mailing lists