[RFC,v3,00/26] Bus and device cleanup for 22.11

  This is a PoC for hiding the rte_bus, rte_driver and rte_device objects.
And mark associated driver only API as internal.

A good amount of the patches are preparation work on rte_bus.h,
rte_dev.h, rte_devargs.h and rte_eal.h headers, removing dependencies
between them.

PCI bus specific handling are removed from testpmd, unit tests and
examples.

After this series, driver-only API headers for registering to buses are
not exported anymore, unless the enable_driver_sdk meson option is
selected.

New accessors for rte_bus, rte_driver and rte_device have been added,
marked with an experimental tag first when introducing them, and later
in the series marked as stable since external users will want to use
those drop-in replacements right away.

A check is added to ensure we won't pollute app/ and examples/ again,
though some unit tests are left intentionnally untouched as they test
some internals of DPDK.

Comments welcome.

Changes since RFC v2:
- added check for additions of include .*_(driver|pmd)\.h in apps and
  examples,
- dropped legacy/debug testpmd commands to read PCI BAR0 registers,
- dropped patches on bbdev, ethdev, rawdev driver headers for now,
- reordered patches and separated changes per bus type to ease review,
- added more accessor for device,
- introduced rte_dev_bus_info to provide a Bus specific description of
  a device, a first use is for providing a PCI device vendor / device
  identifiers that are otherwise unavailable through a generic existing
  API,

Changes since RFC v1:
- added two more cleanups (new patch 3 and 4) for unit test and examples
  relying on PCI specific info,
- went on with masking rte_driver and rte_device too,
  

> On Jul 28, 2022, at 8:26 AM, David Marchand <david.marchand@redhat.com> wrote:
> 
> This is a PoC for hiding the rte_bus, rte_driver and rte_device objects.
> And mark associated driver only API as internal.
> 
> A good amount of the patches are preparation work on rte_bus.h,
> rte_dev.h, rte_devargs.h and rte_eal.h headers, removing dependencies
> between them.
> 
> PCI bus specific handling are removed from testpmd, unit tests and
> examples.
> 
> After this series, driver-only API headers for registering to buses are
> not exported anymore, unless the enable_driver_sdk meson option is
> selected.
> 
> New accessors for rte_bus, rte_driver and rte_device have been added,
> marked with an experimental tag first when introducing them, and later
> in the series marked as stable since external users will want to use
> those drop-in replacements right away.
> 
> A check is added to ensure we won't pollute app/ and examples/ again,
> though some unit tests are left intentionnally untouched as they test
> some internals of DPDK.
> 
> Comments welcome.

Hi David,

I certainly understand wanting to hide those objects to avoid ABI
maintenance.  SPDK is using fields directly from some of those structures
today, but I think all of them could be replaced with associated helper
functions.  This was discussed a bit late last year related to Chenbo’s
earlier patch set:

https://www.mail-archive.com/dev@dpdk.org/msg222560.html

It looks like SPDK never got back to the DPDK project on exactly what APIs
we would need, to see if DPDK could still support a minimal API without
requiring the enable_driver_sdk flag.  I’m generating an exact list, but it
would be rendered moot depending on the next question...

Can we keep rte_pci_register(), or a new variation of it that keeps the
rte_pci_driver structure hidden?  Hiding rte_pci_register() would mean
SPDK can no longer work with a packaged DPDK.  Or the DPDK packages
would need to set enable_driver_sdk which I suspect is not the intent.

If you’re open to this, we (SPDK) can make a proposal on exactly what we
would need to keep SPDK working with a standard DPDK build.

Thanks,

Jim
  

Hello,

On Fri, Aug 5, 2022 at 1:19 AM Harris, James R <james.r.harris@intel.com> wrote:
> Can we keep rte_pci_register(), or a new variation of it that keeps the
> rte_pci_driver structure hidden?  Hiding rte_pci_register() would mean
> SPDK can no longer work with a packaged DPDK.  Or the DPDK packages
> would need to set enable_driver_sdk which I suspect is not the intent.

What do you think if SPDK maintains a copy of the internal headers?

The internal API are not supposed to change that often, but we (DPDK)
won't guarantee it.
This would still put some maintenance burden on SPDK but I think it is
a good compromise.

I did a PoC this morning and put patches in my forked repo:
https://github.com/david-marchand/spdk/commits/master
  

> From: David Marchand <david.marchand@redhat.com>
> 
> Hello,
> 
> On Fri, Aug 5, 2022 at 1:19 AM Harris, James R <james.r.harris@intel.com>
> wrote:
> > Can we keep rte_pci_register(), or a new variation of it that keeps
> > the rte_pci_driver structure hidden?  Hiding rte_pci_register() would
> > mean SPDK can no longer work with a packaged DPDK.  Or the DPDK
> > packages would need to set enable_driver_sdk which I suspect is not the
> intent.
> 
> What do you think if SPDK maintains a copy of the internal headers?
> 
> The internal API are not supposed to change that often, but we (DPDK) won't
> guarantee it.
> This would still put some maintenance burden on SPDK but I think it is a good
> compromise.
>

Would these internal symbols be considered part of the public/official ABI? When
SPDK goes to dynamically load a shared DPDK library, how can we detect
whether it's a version that we support linking against?

 
> I did a PoC this morning and put patches in my forked repo:
> https://github.com/david-marchand/spdk/commits/master
> 
> 
> --
> David Marchand
  

On Mon, Aug 29, 2022 at 7:12 PM Walker, Benjamin
<benjamin.walker@intel.com> wrote:
> > > Can we keep rte_pci_register(), or a new variation of it that keeps
> > > the rte_pci_driver structure hidden?  Hiding rte_pci_register() would
> > > mean SPDK can no longer work with a packaged DPDK.  Or the DPDK
> > > packages would need to set enable_driver_sdk which I suspect is not the
> > intent.
> >
> > What do you think if SPDK maintains a copy of the internal headers?
> >
> > The internal API are not supposed to change that often, but we (DPDK) won't
> > guarantee it.
> > This would still put some maintenance burden on SPDK but I think it is a good
> > compromise.
> >
>
> Would these internal symbols be considered part of the public/official ABI? When

What do you mean by "public/official"?
If you mean the "stable" ABI (as described in the ABI policy document
and for which compatibility is preserved across minor versions of the
ABI), the answer is no: internal symbols are not part of it.


> SPDK goes to dynamically load a shared DPDK library, how can we detect
> whether it's a version that we support linking against?

The runtime version of a DPDK library is available via rte_version().


As for the PCI drivers that SPDK wants to register in DPDK, what do
you think if SPDK people added and maintained a "generic" PCI driver
in DPDK.
This driver would expose a new API (which can not re-expose internal
structures, like rte_pci_driver and consorts) and ensure its ABI is
maintained in the long term.
This makes me think of pci-stub, but in DPDK.

I did not think too much about it and I don't understand what SPDK
requires, but is there something wrong with this approach?
  

> On Aug 30, 2022, at 8:09 AM, David Marchand <david.marchand@redhat.com> wrote:
> 
> On Mon, Aug 29, 2022 at 7:12 PM Walker, Benjamin
> <benjamin.walker@intel.com> wrote:
>>>> Can we keep rte_pci_register(), or a new variation of it that keeps
>>>> the rte_pci_driver structure hidden?  Hiding rte_pci_register() would
>>>> mean SPDK can no longer work with a packaged DPDK.  Or the DPDK
>>>> packages would need to set enable_driver_sdk which I suspect is not the
>>> intent.
>>> 
>>> What do you think if SPDK maintains a copy of the internal headers?
>>> 
>>> The internal API are not supposed to change that often, but we (DPDK) won't
>>> guarantee it.
>>> This would still put some maintenance burden on SPDK but I think it is a good
>>> compromise.
>>> 
>> 
>> Would these internal symbols be considered part of the public/official ABI? When
> 
> What do you mean by "public/official"?
> If you mean the "stable" ABI (as described in the ABI policy document
> and for which compatibility is preserved across minor versions of the
> ABI), the answer is no: internal symbols are not part of it.
> 
> 
>> SPDK goes to dynamically load a shared DPDK library, how can we detect
>> whether it's a version that we support linking against?
> 
> The runtime version of a DPDK library is available via rte_version().
> 
> 
> As for the PCI drivers that SPDK wants to register in DPDK, what do
> you think if SPDK people added and maintained a "generic" PCI driver
> in DPDK.
> This driver would expose a new API (which can not re-expose internal
> structures, like rte_pci_driver and consorts) and ensure its ABI is
> maintained in the long term.
> This makes me think of pci-stub, but in DPDK.
> 
> I did not think too much about it and I don't understand what SPDK
> requires, but is there something wrong with this approach?

The stub driver idea is interesting. In the past we’ve needed access to more
than what a stub driver would provide - for example, to work around kernel
vfio + pci bugs when we probe 24 NVMe SSDs behind a PCIe switch that is
hot-inserted, or conversely handling hot removal of that same switch.  So
I’m worried that even if we do the stub driver, we end up running into a
case where we need these header file copies anyways.  Not ruling the stub
driver out, but I think it's a longer-term goal, not something we would have
ready for 22.11 obviously. 

So sticking with the internal header copies for now, this works fine
for LTS and non-LTS releases.  What about stable releases (i.e. 22.11.1)?
IIUC, these header files could change in a stable release since they are
no longer part of the ABI?

-Jim
  

On Thu, Sep 22, 2022 at 12:29 AM Harris, James R
<james.r.harris@intel.com> wrote:
> > On Aug 30, 2022, at 8:09 AM, David Marchand <david.marchand@redhat.com> wrote:
> >
> > On Mon, Aug 29, 2022 at 7:12 PM Walker, Benjamin
> > <benjamin.walker@intel.com> wrote:
> >>>> Can we keep rte_pci_register(), or a new variation of it that keeps
> >>>> the rte_pci_driver structure hidden?  Hiding rte_pci_register() would
> >>>> mean SPDK can no longer work with a packaged DPDK.  Or the DPDK
> >>>> packages would need to set enable_driver_sdk which I suspect is not the
> >>> intent.
> >>>
> >>> What do you think if SPDK maintains a copy of the internal headers?
> >>>
> >>> The internal API are not supposed to change that often, but we (DPDK) won't
> >>> guarantee it.
> >>> This would still put some maintenance burden on SPDK but I think it is a good
> >>> compromise.
> >>>
> >>
> >> Would these internal symbols be considered part of the public/official ABI? When
> >
> > What do you mean by "public/official"?
> > If you mean the "stable" ABI (as described in the ABI policy document
> > and for which compatibility is preserved across minor versions of the
> > ABI), the answer is no: internal symbols are not part of it.
> >
> >
> >> SPDK goes to dynamically load a shared DPDK library, how can we detect
> >> whether it's a version that we support linking against?
> >
> > The runtime version of a DPDK library is available via rte_version().
> >
> >
> > As for the PCI drivers that SPDK wants to register in DPDK, what do
> > you think if SPDK people added and maintained a "generic" PCI driver
> > in DPDK.
> > This driver would expose a new API (which can not re-expose internal
> > structures, like rte_pci_driver and consorts) and ensure its ABI is
> > maintained in the long term.
> > This makes me think of pci-stub, but in DPDK.
> >
> > I did not think too much about it and I don't understand what SPDK
> > requires, but is there something wrong with this approach?
>
> The stub driver idea is interesting. In the past we’ve needed access to more
> than what a stub driver would provide - for example, to work around kernel
> vfio + pci bugs when we probe 24 NVMe SSDs behind a PCIe switch that is
> hot-inserted, or conversely handling hot removal of that same switch.  So
> I’m worried that even if we do the stub driver, we end up running into a
> case where we need these header file copies anyways.  Not ruling the stub
> driver out, but I think it's a longer-term goal, not something we would have
> ready for 22.11 obviously.
>
> So sticking with the internal header copies for now, this works fine
> for LTS and non-LTS releases.  What about stable releases (i.e. 22.11.1)?
> IIUC, these header files could change in a stable release since they are
> no longer part of the ABI?

In theory, yes they can change.
But this is a price to pay, as no one seems willing to invest and
maintain a stable API for PCI drivers.


I just noticed that some parts of the cleanups I had proposed have
been merged in SPDK.
Next time, I prefer getting some feedback from SPDK community before
my SoB is applied (or stripped) on modified patches.


Thanks.
  

> On Sep 23, 2022, at 12:13 AM, David Marchand <david.marchand@redhat.com> wrote:
> 
> On Thu, Sep 22, 2022 at 12:29 AM Harris, James R
> <james.r.harris@intel.com> wrote:
>> 
>> 
>> So sticking with the internal header copies for now, this works fine
>> for LTS and non-LTS releases.  What about stable releases (i.e. 22.11.1)?
>> IIUC, these header files could change in a stable release since they are
>> no longer part of the ABI?
> 
> In theory, yes they can change.
> But this is a price to pay, as no one seems willing to invest and
> maintain a stable API for PCI drivers.

Understood.  SPDK is preparing for this possibility, just wanted to confirm
it was necessary.

> I just noticed that some parts of the cleanups I had proposed have
> been merged in SPDK.
> Next time, I prefer getting some feedback from SPDK community before
> my SoB is applied (or stripped) on modified patches.

My apologies David.  The RTE_DEV_FOREACH cleanup was a nice one and an obvious
improvement.  I believe it was applied without any modifications (except for
fuzz offsets) but still should have given you a heads-up.

-Jim