Message-ID: <AM0PR05MB4866169E38D7F157F0B4DC49D14C0@AM0PR05MB4866.eurprd05.prod.outlook.com>
Date:   Tue, 19 Nov 2019 04:31:56 +0000
From:   Parav Pandit <parav@...lanox.com>
To:     "Ertman, David M" <david.m.ertman@...el.com>,
        "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "jgg@...pe.ca" <jgg@...pe.ca>,
        "Patil, Kiran" <kiran.patil@...el.com>
Subject: RE: [net-next v2 1/1] virtual-bus: Implementation of Virtual Bus

Hi David,

> From: Ertman, David M <david.m.ertman@...el.com>
> Sent: Monday, November 18, 2019 9:59 PM
> Subject: RE: [net-next v2 1/1] virtual-bus: Implementation of Virtual Bus
> 
> > -----Original Message-----
> > From: Parav Pandit <parav@...lanox.com>
> > Sent: Friday, November 15, 2019 3:26 PM
> > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@...el.com>;
> > davem@...emloft.net; gregkh@...uxfoundation.org
> > Cc: Ertman, David M <david.m.ertman@...el.com>;
> > netdev@...r.kernel.org; linux-rdma@...r.kernel.org;
> > nhorman@...hat.com; sassmann@...hat.com; jgg@...pe.ca; Patil, Kiran
> > <kiran.patil@...el.com>
> > Subject: RE: [net-next v2 1/1] virtual-bus: Implementation of Virtual
> > Bus
> >
> > Hi Jeff,
> >
> > > From: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> > > Sent: Friday, November 15, 2019 4:34 PM
> > >
> > > From: Dave Ertman <david.m.ertman@...el.com>
> > >
> > > This is the initial implementation of the Virtual Bus,
> > > virtbus_device and virtbus_driver.  The virtual bus is a software
> > > based bus intended to support lightweight devices and drivers and
> > > provide matching between them and probing of the registered drivers.
> > >
> > > The primary purpose of the virual bus is to provide matching
> > > services and to pass the data pointer contained in the
> > > virtbus_device to the virtbus_driver during its probe call.  This
> > > will allow two separate kernel objects to match up and start
> communication.
> > >
> > It is fundamental to know that rdma device created by virtbus_driver
> > will be anchored to which bus for an non abusive use.
> > virtbus or parent pci bus?
> > I asked this question in v1 version of this patch.
> 
> The model we will be using is a PCI LAN driver that will allocate and register a
> virtbus_device.  The virtbus_device will be anchored to the virtual bus, not the
> PCI bus.
o.k.

> 
> The virtbus does not have a requirement that elements registering with it have
> any association with another outside bus or device.
> 
This is what I want to capture in cover letter and documentation.

> RDMA is not attached to any bus when it's init is called.  The virtbus_driver that
> it will create will be attached to the virtual bus.
> 
> The RDMA driver will register a virtbus_driver object.  Its probe will accept the
> data pointer from the virtbus_device that the PCI LAN driver created.
> 
What I am saying that RDMA device created by the irdma driver or mlx5_ib driver should be anchored to the PCI device and not the virtbus device.

struct ib_device.dev.parent = &pci_dev->dev;

if this is not done, and if it is,

struct ib_device.dev.parent = &virtbus_dev->dev;

Than we are inviting huge burden as below.
(a) user compatibility with several tools, orchestration etc is broken, because rdma cannot be reached back to its PCI device as before.
This is some internal kernel change for 'better code handling', which is surfacing to rdma device name changing - systemd/udev broken, until all distros upgrade and implement this virtbus naming scheme.
Even with that orchestration tools shipped outside of distro are broken.

(b) virtbus must extend iommu support in intel, arm, amd, ppc systems otherwise straight away rdma is broken in those environments with this 'internal code restructure'.
These iommu doesn't support non PCI buses.

(c) anchoring on virtbus brings challenge to get unique id for persistent naming when irdma/mlx5_ib device is not created by 'user'.

This improvement by bus matching service != 'ethX to ens2f0 improvement of netdevs happened few years back'.
Hence, my input is,

irdma_virtubus_probe() {
	struct ib_device.dev.parent = &pci_dev->dev;
	ib_register_driver();
}

> >
> > Also since it says - 'to support lightweight devices', documenting
> > that information is critical to avoid ambiguity.
> >
> > Since for a while I am working on the subbus/subdev_bus/xbus/mdev [1]
> > whatever we want to call it, it overlaps with your comment about 'to
> > support lightweight devices'.
> > Hence let's make things crystal clear weather the purpose is 'only
> > matching service' or also 'lightweight devices'.
> > If this is only matching service, lets please remove lightweight devices part..
> >
> 
> This is only for matching services for kernel objects, I will work on phrasing this
> clearer.
> 

Ok. Great. Due to above two fundamental points, we clearly write up the matching service.
Not sure naming virtbus to 'virbus_service' in bus_type is an extreme way to make this clear.

> > You additionally need modpost support for id table integration to
> > modifo, modprobe and other tools.
> > A small patch similar to this one [2] is needed.
> > Please include in the series.
> >
> 
> modpost support added - thanks for that catch!
> 
> > [..]
> >
> > > +static const
> > > +struct virtbus_dev_id *virtbus_match_id(const struct virtbus_dev_id *id,
> > > +					struct virtbus_device *vdev)
> > > +{
> > > +	while (id->name[0]) {
> > > +		if (!strcmp(vdev->name, id->name)) {
> > > +			vdev->dev_id = id;
> > Matching function shouldn't be modifying the id.
> 
> This is not the main id of the virtbus_device.  This is copying the element in the
> driver id_table that was matched into the virtbus_device struct, so that when
> the virtbus_device struct is passed to the virtbus_driver's probe, it can access
> the correct driver_data.
> 
> I chose a poor name for this field, I will change the name of this part of the
> struct to matched_element and include a comment on what is going on here.
> 
When virtbus_device is created, its class_id or device_id identifying the device is decided.
So it should be set in the create() routine.

Powered by blists - more mailing lists