bus/pci: optimize pci device probe

Message ID 20200426173811.49788-1-jerinj@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series bus/pci: optimize pci device probe |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/travis-robot warning Travis build: errored
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-testing fail Testing issues
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Jerin Jacob Kollanukkaran April 26, 2020, 5:38 p.m. UTC
  From: Jerin Jacob <jerinj@marvell.com>

If the PCI device is not attached to any driver then there is no
point in probing it. As an optimization, skip the PCI device probe if
the PCI device driver of type RTE_KDRV_NONE.

Signed-off-by: Jerin Jacob <jerinj@marvell.com>
---
Notes:
------
- virtio drivers does special treatment based on RTE_KDRV_UNKNOWN, That is
the reason allowing RTE_KDRV_UNKNOWN in this patch.
- virio devices uses RTE_KDRV_UNKNOWN for some special meaning, IMO, if it would
  be better, if
a) Introduce the KDRV for virio
b) If the PCIe device of driver type NONE or UNKNOWN then not even add in pci
list
in the scan, It will improve the boot time by avoiding operation on
unwanted device like sorting the PCI devices, scanning it, probe it, managing
it etc.

- Initial problem reported at http://patches.dpdk.org/patch/64999/ as
boot time printf clutter on octeontx2 devices with a lot PCI devices which are
of type RTE_KDRV_NONE.

 drivers/bus/pci/pci_common.c | 2 ++
 1 file changed, 2 insertions(+)

--
2.26.2
  

Comments

Thomas Monjalon April 26, 2020, 6:08 p.m. UTC | #1
26/04/2020 19:38, jerinj@marvell.com:
> From: Jerin Jacob <jerinj@marvell.com>
> 
> If the PCI device is not attached to any driver then there is no
> point in probing it. As an optimization, skip the PCI device probe if
> the PCI device driver of type RTE_KDRV_NONE.
> 
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> ---
> Notes:
> ------
> - virtio drivers does special treatment based on RTE_KDRV_UNKNOWN, That is
> the reason allowing RTE_KDRV_UNKNOWN in this patch.
> - virio devices uses RTE_KDRV_UNKNOWN for some special meaning, IMO, if it would
>   be better, if
> a) Introduce the KDRV for virio
> b) If the PCIe device of driver type NONE or UNKNOWN then not even add in pci
> list
> in the scan, It will improve the boot time by avoiding operation on
> unwanted device like sorting the PCI devices, scanning it, probe it, managing
> it etc.

mlx4/mlx4 uses RTE_KDRV_UNKNOWN.

> - Initial problem reported at http://patches.dpdk.org/patch/64999/ as
> boot time printf clutter on octeontx2 devices with a lot PCI devices which are
> of type RTE_KDRV_NONE.

Add a logtype for PCI driver and adjust log level accordingly
to your preferences.

> @@ -271,6 +271,8 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
>  	FOREACH_DRIVER_ON_PCIBUS(dr) {
> +		if (dev->kdrv == RTE_KDRV_NONE)
> +			continue;
>  		rc = rte_pci_probe_one_driver(dr, dev);

Nack
  
Jerin Jacob April 26, 2020, 6:41 p.m. UTC | #2
On Sun, Apr 26, 2020 at 11:38 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 26/04/2020 19:38, jerinj@marvell.com:
> > From: Jerin Jacob <jerinj@marvell.com>
> >
> > If the PCI device is not attached to any driver then there is no
> > point in probing it. As an optimization, skip the PCI device probe if
> > the PCI device driver of type RTE_KDRV_NONE.
> >
> > Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> > ---
> > Notes:
> > ------
> > - virtio drivers does special treatment based on RTE_KDRV_UNKNOWN, That is
> > the reason allowing RTE_KDRV_UNKNOWN in this patch.
> > - virio devices uses RTE_KDRV_UNKNOWN for some special meaning, IMO, if it would
> >   be better, if
> > a) Introduce the KDRV for virio
> > b) If the PCIe device of driver type NONE or UNKNOWN then not even add in pci
> > list
> > in the scan, It will improve the boot time by avoiding operation on
> > unwanted device like sorting the PCI devices, scanning it, probe it, managing
> > it etc.
>
> mlx4/mlx4 uses RTE_KDRV_UNKNOWN.

OK.

>
> > - Initial problem reported at http://patches.dpdk.org/patch/64999/ as
> > boot time printf clutter on octeontx2 devices with a lot PCI devices which are
> > of type RTE_KDRV_NONE.
>
> Add a logtype for PCI driver and adjust log level accordingly
> to your preferences.
>
> > @@ -271,6 +271,8 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
> >       FOREACH_DRIVER_ON_PCIBUS(dr) {
> > +             if (dev->kdrv == RTE_KDRV_NONE)
> > +                     continue;
> >               rc = rte_pci_probe_one_driver(dr, dev);
>
> Nack

I understand mlx4/mlx5 is using RTE_KDRV_UNKNOWN, Here we are skipping
the RTE_KDRV_NONE,
What is the use case for probing the devices with RTE_KDRV_NONE?


>
>
  
Thomas Monjalon April 26, 2020, 8:06 p.m. UTC | #3
26/04/2020 20:41, Jerin Jacob:
> On Sun, Apr 26, 2020 at 11:38 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 26/04/2020 19:38, jerinj@marvell.com:
> > > From: Jerin Jacob <jerinj@marvell.com>
> > >
> > > If the PCI device is not attached to any driver then there is no
> > > point in probing it. As an optimization, skip the PCI device probe if
> > > the PCI device driver of type RTE_KDRV_NONE.
> > >
> > > Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> > > ---
> > > Notes:
> > > ------
> > > - virtio drivers does special treatment based on RTE_KDRV_UNKNOWN, That is
> > > the reason allowing RTE_KDRV_UNKNOWN in this patch.
> > > - virio devices uses RTE_KDRV_UNKNOWN for some special meaning, IMO, if it would
> > >   be better, if
> > > a) Introduce the KDRV for virio
> > > b) If the PCIe device of driver type NONE or UNKNOWN then not even add in pci
> > > list
> > > in the scan, It will improve the boot time by avoiding operation on
> > > unwanted device like sorting the PCI devices, scanning it, probe it, managing
> > > it etc.
> >
> > mlx4/mlx4 uses RTE_KDRV_UNKNOWN.
> 
> OK.
> 
> > > - Initial problem reported at http://patches.dpdk.org/patch/64999/ as
> > > boot time printf clutter on octeontx2 devices with a lot PCI devices which are
> > > of type RTE_KDRV_NONE.
> >
> > Add a logtype for PCI driver and adjust log level accordingly
> > to your preferences.
> >
> > > @@ -271,6 +271,8 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
> > >       FOREACH_DRIVER_ON_PCIBUS(dr) {
> > > +             if (dev->kdrv == RTE_KDRV_NONE)
> > > +                     continue;
> > >               rc = rte_pci_probe_one_driver(dr, dev);
> >
> > Nack
> 
> I understand mlx4/mlx5 is using RTE_KDRV_UNKNOWN, Here we are skipping
> the RTE_KDRV_NONE,
> What is the use case for probing the devices with RTE_KDRV_NONE?

Maybe you are right. I don't remember the use case.
I think I remember these virtio and vmxnet3 PMD were not using UIO:
	http://git.dpdk.org/old/virtio-net-pmd/tree/virtio_user.c
	http://git.dpdk.org/old/vmxnet3-usermap/tree/pmd/vmxnet3.c

We need to know which case is using following code:

    case RTE_KDRV_NONE:
#if defined(RTE_ARCH_X86)
        ret = pci_ioport_map(dev, bar, p);
#endif
        break;

David, please could you refresh our memory?
  
Jerin Jacob April 27, 2020, 5:59 p.m. UTC | #4
On Mon, Apr 27, 2020 at 1:36 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 26/04/2020 20:41, Jerin Jacob:
> > On Sun, Apr 26, 2020 at 11:38 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > 26/04/2020 19:38, jerinj@marvell.com:
> > > > From: Jerin Jacob <jerinj@marvell.com>
> > > >
> > > > If the PCI device is not attached to any driver then there is no
> > > > point in probing it. As an optimization, skip the PCI device probe if
> > > > the PCI device driver of type RTE_KDRV_NONE.
> > > >
> > > > Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> > > > ---
> > > > Notes:
> > > > ------
> > > > - virtio drivers does special treatment based on RTE_KDRV_UNKNOWN, That is
> > > > the reason allowing RTE_KDRV_UNKNOWN in this patch.
> > > > - virio devices uses RTE_KDRV_UNKNOWN for some special meaning, IMO, if it would
> > > >   be better, if
> > > > a) Introduce the KDRV for virio
> > > > b) If the PCIe device of driver type NONE or UNKNOWN then not even add in pci
> > > > list
> > > > in the scan, It will improve the boot time by avoiding operation on
> > > > unwanted device like sorting the PCI devices, scanning it, probe it, managing
> > > > it etc.
> > >
> > > mlx4/mlx4 uses RTE_KDRV_UNKNOWN.
> >
> > OK.
> >
> > > > - Initial problem reported at http://patches.dpdk.org/patch/64999/ as
> > > > boot time printf clutter on octeontx2 devices with a lot PCI devices which are
> > > > of type RTE_KDRV_NONE.
> > >
> > > Add a logtype for PCI driver and adjust log level accordingly
> > > to your preferences.
> > >
> > > > @@ -271,6 +271,8 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
> > > >       FOREACH_DRIVER_ON_PCIBUS(dr) {
> > > > +             if (dev->kdrv == RTE_KDRV_NONE)
> > > > +                     continue;
> > > >               rc = rte_pci_probe_one_driver(dr, dev);
> > >
> > > Nack
> >
> > I understand mlx4/mlx5 is using RTE_KDRV_UNKNOWN, Here we are skipping
> > the RTE_KDRV_NONE,
> > What is the use case for probing the devices with RTE_KDRV_NONE?
>
> Maybe you are right. I don't remember the use case.
> I think I remember these virtio and vmxnet3 PMD were not using UIO:
>         http://git.dpdk.org/old/virtio-net-pmd/tree/virtio_user.c
>         http://git.dpdk.org/old/vmxnet3-usermap/tree/pmd/vmxnet3.c

Looks it is OLD stale drivers with DPDK 1.3 or something.

Currently, virtio is using, following drivers.

RTE_PMD_REGISTER_PCI_TABLE(net_virtio, pci_id_virtio_map);
RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci");

>
> We need to know which case is using following code:
>
>     case RTE_KDRV_NONE:
> #if defined(RTE_ARCH_X86)
>         ret = pci_ioport_map(dev, bar, p);
> #endif
>         break;

AFAIK, it does not affect the scanning. virtio maintainers?

> David, please could you refresh our memory?
>
>
  
David Marchand April 28, 2020, 8:50 a.m. UTC | #5
On Sun, Apr 26, 2020 at 10:06 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 26/04/2020 20:41, Jerin Jacob:
> > On Sun, Apr 26, 2020 at 11:38 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > 26/04/2020 19:38, jerinj@marvell.com:
> > > > From: Jerin Jacob <jerinj@marvell.com>
> > > >
> > > > If the PCI device is not attached to any driver then there is no
> > > > point in probing it. As an optimization, skip the PCI device probe if
> > > > the PCI device driver of type RTE_KDRV_NONE.
> > > >
> > > > Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> > > > ---
> > > > Notes:
> > > > ------
> > > > - virtio drivers does special treatment based on RTE_KDRV_UNKNOWN, That is
> > > > the reason allowing RTE_KDRV_UNKNOWN in this patch.
> > > > - virio devices uses RTE_KDRV_UNKNOWN for some special meaning, IMO, if it would
> > > >   be better, if
> > > > a) Introduce the KDRV for virio
> > > > b) If the PCIe device of driver type NONE or UNKNOWN then not even add in pci
> > > > list
> > > > in the scan, It will improve the boot time by avoiding operation on
> > > > unwanted device like sorting the PCI devices, scanning it, probe it, managing
> > > > it etc.
> > >
> > > mlx4/mlx4 uses RTE_KDRV_UNKNOWN.
> >
> > OK.
> >
> > > > - Initial problem reported at http://patches.dpdk.org/patch/64999/ as
> > > > boot time printf clutter on octeontx2 devices with a lot PCI devices which are
> > > > of type RTE_KDRV_NONE.
> > >
> > > Add a logtype for PCI driver and adjust log level accordingly
> > > to your preferences.
> > >
> > > > @@ -271,6 +271,8 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
> > > >       FOREACH_DRIVER_ON_PCIBUS(dr) {
> > > > +             if (dev->kdrv == RTE_KDRV_NONE)
> > > > +                     continue;
> > > >               rc = rte_pci_probe_one_driver(dr, dev);
> > >
> > > Nack
> >
> > I understand mlx4/mlx5 is using RTE_KDRV_UNKNOWN, Here we are skipping
> > the RTE_KDRV_NONE,
> > What is the use case for probing the devices with RTE_KDRV_NONE?
>
> Maybe you are right. I don't remember the use case.
> I think I remember these virtio and vmxnet3 PMD were not using UIO:
>         http://git.dpdk.org/old/virtio-net-pmd/tree/virtio_user.c
>         http://git.dpdk.org/old/vmxnet3-usermap/tree/pmd/vmxnet3.c
>
> We need to know which case is using following code:
>
>     case RTE_KDRV_NONE:
> #if defined(RTE_ARCH_X86)
>         ret = pci_ioport_map(dev, bar, p);
> #endif
>         break;
>
> David, please could you refresh our memory?

The in-tree virtio-net driver directly calls rte_pci_map_device /
rte_pci_ioport_map depending on virtio legacy/modern modes.
This is why the virtio pci driver does not ask for
RTE_PCI_DRV_NEED_MAPPING to the pci bus.


In ioport mode, there were two options for historical reasons because
of the virtio driver you mention.
This driver did not rely on uio, and this behavior was later merged to
the current in-tree driver.
It ended up (not clean) in the pci bus driver because virtio was the
only user of this code.


Removing this special case could break x86 applications running with
legacy virtio.


On the plus side, we have been announcing for some time in virtio:
RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci");
  
Jerin Jacob April 28, 2020, 9:34 a.m. UTC | #6
On Tue, Apr 28, 2020 at 2:21 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Sun, Apr 26, 2020 at 10:06 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 26/04/2020 20:41, Jerin Jacob:
> > > On Sun, Apr 26, 2020 at 11:38 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > >
> > > > 26/04/2020 19:38, jerinj@marvell.com:
> > > > > From: Jerin Jacob <jerinj@marvell.com>
> > > > >
> > > > > If the PCI device is not attached to any driver then there is no
> > > > > point in probing it. As an optimization, skip the PCI device probe if
> > > > > the PCI device driver of type RTE_KDRV_NONE.
> > > > >
> > > > > Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> > > > > ---
> > > > > Notes:
> > > > > ------
> > > > > - virtio drivers does special treatment based on RTE_KDRV_UNKNOWN, That is
> > > > > the reason allowing RTE_KDRV_UNKNOWN in this patch.
> > > > > - virio devices uses RTE_KDRV_UNKNOWN for some special meaning, IMO, if it would
> > > > >   be better, if
> > > > > a) Introduce the KDRV for virio
> > > > > b) If the PCIe device of driver type NONE or UNKNOWN then not even add in pci
> > > > > list
> > > > > in the scan, It will improve the boot time by avoiding operation on
> > > > > unwanted device like sorting the PCI devices, scanning it, probe it, managing
> > > > > it etc.
> > > >
> > > > mlx4/mlx4 uses RTE_KDRV_UNKNOWN.
> > >
> > > OK.
> > >
> > > > > - Initial problem reported at http://patches.dpdk.org/patch/64999/ as
> > > > > boot time printf clutter on octeontx2 devices with a lot PCI devices which are
> > > > > of type RTE_KDRV_NONE.
> > > >
> > > > Add a logtype for PCI driver and adjust log level accordingly
> > > > to your preferences.
> > > >
> > > > > @@ -271,6 +271,8 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
> > > > >       FOREACH_DRIVER_ON_PCIBUS(dr) {
> > > > > +             if (dev->kdrv == RTE_KDRV_NONE)
> > > > > +                     continue;
> > > > >               rc = rte_pci_probe_one_driver(dr, dev);
> > > >
> > > > Nack
> > >
> > > I understand mlx4/mlx5 is using RTE_KDRV_UNKNOWN, Here we are skipping
> > > the RTE_KDRV_NONE,
> > > What is the use case for probing the devices with RTE_KDRV_NONE?
> >
> > Maybe you are right. I don't remember the use case.
> > I think I remember these virtio and vmxnet3 PMD were not using UIO:
> >         http://git.dpdk.org/old/virtio-net-pmd/tree/virtio_user.c
> >         http://git.dpdk.org/old/vmxnet3-usermap/tree/pmd/vmxnet3.c
> >
> > We need to know which case is using following code:
> >
> >     case RTE_KDRV_NONE:
> > #if defined(RTE_ARCH_X86)
> >         ret = pci_ioport_map(dev, bar, p);
> > #endif
> >         break;
> >
> > David, please could you refresh our memory?
>
> The in-tree virtio-net driver directly calls rte_pci_map_device /
> rte_pci_ioport_map depending on virtio legacy/modern modes.
> This is why the virtio pci driver does not ask for
> RTE_PCI_DRV_NEED_MAPPING to the pci bus.
>
>
> In ioport mode, there were two options for historical reasons because
> of the virtio driver you mention.
> This driver did not rely on uio, and this behavior was later merged to
> the current in-tree driver.
> It ended up (not clean) in the pci bus driver because virtio was the
> only user of this code.
>
>
> Removing this special case could break x86 applications running with
> legacy virtio.
>
>
> On the plus side, we have been announcing for some time in virtio:
> RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci");

What is to conclude?
# The In-tree virtio driver uses ""* igb_uio | uio_pci_generic |
vfio-pci"" driver as backend and it does not need RTE_KDRV_NONE?
OR
# The in-tree, legacy virtio(const struct virtio_pci_ops legacy_op)
can work without any kernel driver in the backend. So RTE_KDRV_NONE
need?




>
>
> --
> David Marchand
>
  
Jerin Jacob May 5, 2020, 3:50 p.m. UTC | #7
On Tue, Apr 28, 2020 at 3:04 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Tue, Apr 28, 2020 at 2:21 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Sun, Apr 26, 2020 at 10:06 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > >
> > > 26/04/2020 20:41, Jerin Jacob:
> > > > On Sun, Apr 26, 2020 at 11:38 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > >
> > > > > 26/04/2020 19:38, jerinj@marvell.com:
> > > > > > From: Jerin Jacob <jerinj@marvell.com>
> > > > > >
> > > > > > If the PCI device is not attached to any driver then there is no
> > > > > > point in probing it. As an optimization, skip the PCI device probe if
> > > > > > the PCI device driver of type RTE_KDRV_NONE.
> > > > > >
> > > > > > Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> > > > > > ---
> > > > > > Notes:
> > > > > > ------
> > > > > > - virtio drivers does special treatment based on RTE_KDRV_UNKNOWN, That is
> > > > > > the reason allowing RTE_KDRV_UNKNOWN in this patch.
> > > > > > - virio devices uses RTE_KDRV_UNKNOWN for some special meaning, IMO, if it would
> > > > > >   be better, if
> > > > > > a) Introduce the KDRV for virio
> > > > > > b) If the PCIe device of driver type NONE or UNKNOWN then not even add in pci
> > > > > > list
> > > > > > in the scan, It will improve the boot time by avoiding operation on
> > > > > > unwanted device like sorting the PCI devices, scanning it, probe it, managing
> > > > > > it etc.
> > > > >
> > > > > mlx4/mlx4 uses RTE_KDRV_UNKNOWN.
> > > >
> > > > OK.
> > > >
> > > > > > - Initial problem reported at http://patches.dpdk.org/patch/64999/ as
> > > > > > boot time printf clutter on octeontx2 devices with a lot PCI devices which are
> > > > > > of type RTE_KDRV_NONE.
> > > > >
> > > > > Add a logtype for PCI driver and adjust log level accordingly
> > > > > to your preferences.
> > > > >
> > > > > > @@ -271,6 +271,8 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
> > > > > >       FOREACH_DRIVER_ON_PCIBUS(dr) {
> > > > > > +             if (dev->kdrv == RTE_KDRV_NONE)
> > > > > > +                     continue;
> > > > > >               rc = rte_pci_probe_one_driver(dr, dev);
> > > > >
> > > > > Nack
> > > >
> > > > I understand mlx4/mlx5 is using RTE_KDRV_UNKNOWN, Here we are skipping
> > > > the RTE_KDRV_NONE,
> > > > What is the use case for probing the devices with RTE_KDRV_NONE?
> > >
> > > Maybe you are right. I don't remember the use case.
> > > I think I remember these virtio and vmxnet3 PMD were not using UIO:
> > >         http://git.dpdk.org/old/virtio-net-pmd/tree/virtio_user.c
> > >         http://git.dpdk.org/old/vmxnet3-usermap/tree/pmd/vmxnet3.c
> > >
> > > We need to know which case is using following code:
> > >
> > >     case RTE_KDRV_NONE:
> > > #if defined(RTE_ARCH_X86)
> > >         ret = pci_ioport_map(dev, bar, p);
> > > #endif
> > >         break;
> > >
> > > David, please could you refresh our memory?
> >
> > The in-tree virtio-net driver directly calls rte_pci_map_device /
> > rte_pci_ioport_map depending on virtio legacy/modern modes.
> > This is why the virtio pci driver does not ask for
> > RTE_PCI_DRV_NEED_MAPPING to the pci bus.
> >
> >
> > In ioport mode, there were two options for historical reasons because
> > of the virtio driver you mention.
> > This driver did not rely on uio, and this behavior was later merged to
> > the current in-tree driver.
> > It ended up (not clean) in the pci bus driver because virtio was the
> > only user of this code.
> >
> >
> > Removing this special case could break x86 applications running with
> > legacy virtio.
> >
> >
> > On the plus side, we have been announcing for some time in virtio:
> > RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci");
>
> What is to conclude?
> # The In-tree virtio driver uses ""* igb_uio | uio_pci_generic |
> vfio-pci"" driver as backend and it does not need RTE_KDRV_NONE?
> OR
> # The in-tree, legacy virtio(const struct virtio_pci_ops legacy_op)
> can work without any kernel driver in the backend. So RTE_KDRV_NONE
> need?

Ping. What is the conclusion? If it is former then this patch is valid.



>
>
>
>
> >
> >
> > --
> > David Marchand
> >
  
David Marchand May 5, 2020, 4:16 p.m. UTC | #8
On Tue, May 5, 2020 at 5:50 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > >
> > > Removing this special case could break x86 applications running with
> > > legacy virtio.
> > >
> > >
> > > On the plus side, we have been announcing for some time in virtio:
> > > RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci");
> >
> > What is to conclude?
> > # The In-tree virtio driver uses ""* igb_uio | uio_pci_generic |
> > vfio-pci"" driver as backend and it does not need RTE_KDRV_NONE?
> > OR
> > # The in-tree, legacy virtio(const struct virtio_pci_ops legacy_op)
> > can work without any kernel driver in the backend. So RTE_KDRV_NONE
> > need?
>
> Ping. What is the conclusion? If it is former then this patch is valid.

I am fine with dropping the legacy part, but I wanted to hear from
Maxime at least.
  
Maxime Coquelin May 6, 2020, 6:34 a.m. UTC | #9
Hi,

On 5/5/20 6:16 PM, David Marchand wrote:
> On Tue, May 5, 2020 at 5:50 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>>>>
>>>> Removing this special case could break x86 applications running with
>>>> legacy virtio.
>>>>
>>>>
>>>> On the plus side, we have been announcing for some time in virtio:
>>>> RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci");
>>>
>>> What is to conclude?
>>> # The In-tree virtio driver uses ""* igb_uio | uio_pci_generic |
>>> vfio-pci"" driver as backend and it does not need RTE_KDRV_NONE?
>>> OR
>>> # The in-tree, legacy virtio(const struct virtio_pci_ops legacy_op)
>>> can work without any kernel driver in the backend. So RTE_KDRV_NONE
>>> need?
>>
>> Ping. What is the conclusion? If it is former then this patch is valid.
> 
> I am fine with dropping the legacy part, but I wanted to hear from
> Maxime at least.
> 
> 

IIUC, it means that with Jerin patch, Virtio Legacy devices support will
be dropped as they won't be probed anymore?

Maxime
  
Jerin Jacob May 6, 2020, 6:43 a.m. UTC | #10
On Wed, May 6, 2020 at 12:05 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> Hi,
>
> On 5/5/20 6:16 PM, David Marchand wrote:
> > On Tue, May 5, 2020 at 5:50 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >>>>
> >>>> Removing this special case could break x86 applications running with
> >>>> legacy virtio.
> >>>>
> >>>>
> >>>> On the plus side, we have been announcing for some time in virtio:
> >>>> RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci");
> >>>
> >>> What is to conclude?
> >>> # The In-tree virtio driver uses ""* igb_uio | uio_pci_generic |
> >>> vfio-pci"" driver as backend and it does not need RTE_KDRV_NONE?
> >>> OR
> >>> # The in-tree, legacy virtio(const struct virtio_pci_ops legacy_op)
> >>> can work without any kernel driver in the backend. So RTE_KDRV_NONE
> >>> need?
> >>
> >> Ping. What is the conclusion? If it is former then this patch is valid.
> >
> > I am fine with dropping the legacy part, but I wanted to hear from
> > Maxime at least.
> >
> >
>
> IIUC, it means that with Jerin patch, Virtio Legacy devices support will
> be dropped as they won't be probed anymore?

The device drivers with RTE_KDRV_NONE as the backend will not be probed.
1) Are Virtio Legacy devices are type of RTE_KDRV_NONE?
2) if yes, Would you like to support for virtio legacy device?
3) if yes, Please fix RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio
| uio_pci_generic | vfio-pci");




>
> Maxime
>
  
Maxime Coquelin May 6, 2020, 7:52 a.m. UTC | #11
On 5/6/20 8:43 AM, Jerin Jacob wrote:
> On Wed, May 6, 2020 at 12:05 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> Hi,
>>
>> On 5/5/20 6:16 PM, David Marchand wrote:
>>> On Tue, May 5, 2020 at 5:50 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>>>>>>
>>>>>> Removing this special case could break x86 applications running with
>>>>>> legacy virtio.
>>>>>>
>>>>>>
>>>>>> On the plus side, we have been announcing for some time in virtio:
>>>>>> RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci");
>>>>>
>>>>> What is to conclude?
>>>>> # The In-tree virtio driver uses ""* igb_uio | uio_pci_generic |
>>>>> vfio-pci"" driver as backend and it does not need RTE_KDRV_NONE?
>>>>> OR
>>>>> # The in-tree, legacy virtio(const struct virtio_pci_ops legacy_op)
>>>>> can work without any kernel driver in the backend. So RTE_KDRV_NONE
>>>>> need?
>>>>
>>>> Ping. What is the conclusion? If it is former then this patch is valid.
>>>
>>> I am fine with dropping the legacy part, but I wanted to hear from
>>> Maxime at least.
>>>
>>>
>>
>> IIUC, it means that with Jerin patch, Virtio Legacy devices support will
>> be dropped as they won't be probed anymore?
> 
> The device drivers with RTE_KDRV_NONE as the backend will not be probed.
> 1) Are Virtio Legacy devices are type of RTE_KDRV_NONE?

Virtio Legacy devices can be probed with no kernel driver.

> 2) if yes, Would you like to support for virtio legacy device?

I am OK to remove legacy + RTE_KDRV_NONE case, but I think it needs an
announcement and being done in a later release to let end-users using
that configuration time to do the change.

> 3) if yes, Please fix RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio
> | uio_pci_generic | vfio-pci");

While support gets removed, what about:

RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic |
vfio-pci | none"); ?

Maxime
  
Jerin Jacob May 6, 2020, 10:51 a.m. UTC | #12
On Wed, May 6, 2020 at 1:23 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
>
>
> On 5/6/20 8:43 AM, Jerin Jacob wrote:
> > On Wed, May 6, 2020 at 12:05 PM Maxime Coquelin
> > <maxime.coquelin@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 5/5/20 6:16 PM, David Marchand wrote:
> >>> On Tue, May 5, 2020 at 5:50 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >>>>>>
> >>>>>> Removing this special case could break x86 applications running with
> >>>>>> legacy virtio.
> >>>>>>
> >>>>>>
> >>>>>> On the plus side, we have been announcing for some time in virtio:
> >>>>>> RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci");
> >>>>>
> >>>>> What is to conclude?
> >>>>> # The In-tree virtio driver uses ""* igb_uio | uio_pci_generic |
> >>>>> vfio-pci"" driver as backend and it does not need RTE_KDRV_NONE?
> >>>>> OR
> >>>>> # The in-tree, legacy virtio(const struct virtio_pci_ops legacy_op)
> >>>>> can work without any kernel driver in the backend. So RTE_KDRV_NONE
> >>>>> need?
> >>>>
> >>>> Ping. What is the conclusion? If it is former then this patch is valid.
> >>>
> >>> I am fine with dropping the legacy part, but I wanted to hear from
> >>> Maxime at least.
> >>>
> >>>
> >>
> >> IIUC, it means that with Jerin patch, Virtio Legacy devices support will
> >> be dropped as they won't be probed anymore?
> >
> > The device drivers with RTE_KDRV_NONE as the backend will not be probed.
> > 1) Are Virtio Legacy devices are type of RTE_KDRV_NONE?
>
> Virtio Legacy devices can be probed with no kernel driver.
>
> > 2) if yes, Would you like to support for virtio legacy device?
>
> I am OK to remove legacy + RTE_KDRV_NONE case, but I think it needs an
> announcement and being done in a later release to let end-users using
> that configuration time to do the change.

OK

>
> > 3) if yes, Please fix RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio
> > | uio_pci_generic | vfio-pci");
>
> While support gets removed, what about:
>
> RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic |
> vfio-pci | none"); ?

+1

>
> Maxime
>
  
David Marchand May 6, 2020, 11:37 a.m. UTC | #13
On Wed, May 6, 2020 at 12:51 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > 3) if yes, Please fix RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio
> > > | uio_pci_generic | vfio-pci");
> >
> > While support gets removed, what about:
> >
> > RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic |
> > vfio-pci | none"); ?
>
> +1

I know some scripts rely on such info to modprobe and bind the devices
based on such info.
So "none" does not make sense, there is no "none" kmod.

Since we want to deprecate this capability, let's not advertise this.
  
Maxime Coquelin May 6, 2020, 11:44 a.m. UTC | #14
On 5/6/20 1:37 PM, David Marchand wrote:
> On Wed, May 6, 2020 at 12:51 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>>>> 3) if yes, Please fix RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio
>>>> | uio_pci_generic | vfio-pci");
>>>
>>> While support gets removed, what about:
>>>
>>> RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic |
>>> vfio-pci | none"); ?
>>
>> +1
> 
> I know some scripts rely on such info to modprobe and bind the devices
> based on such info.
> So "none" does not make sense, there is no "none" kmod.
> 
> Since we want to deprecate this capability, let's not advertise this.

Makes sense.

Jerin, will add this announcement in the release note? I think we can
remove it in v20.08.

Thanks,
Maxime
  

Patch

diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
index 3f5542076..2fa3d85ae 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -271,6 +271,8 @@  pci_probe_all_drivers(struct rte_pci_device *dev)
 		return -EINVAL;

 	FOREACH_DRIVER_ON_PCIBUS(dr) {
+		if (dev->kdrv == RTE_KDRV_NONE)
+			continue;
 		rc = rte_pci_probe_one_driver(dr, dev);
 		if (rc < 0)
 			/* negative value is an error */