On Mon, Feb 14, 2022 at 10:21:20AM -0800, Stephen Hemminger wrote:
> On Mon, 14 Feb 2022 18:49:48 +0100
> Thomas Monjalon <tho...@monjalon.net> wrote:
>
> > 14/02/2022 18:t24, Stephen Hemminger:
> > > On Mon, 14 Feb 2022 15:03:43 +0000
> > > Bruce Richardson <bruce.richard...@intel.com> wrote:
> > >
> > > > On Mon, Feb 14, 2022 at 11:43:35AM +0000, Bruce Richardson wrote:
> > > > > On Mon, Feb 14, 2022 at 12:19:24PM +0100, Thomas Monjalon wrote:
> > > > > > 14/02/2022 11:45, Bruce Richardson:
> > > > > > > On Mon, Feb 14, 2022 at 10:22:08AM +0100, Thomas Monjalon wrote:
> > > > > > >
> > > > > > > > 14/02/2022 10:13, Tyler Retzlaff:
> > > > > > > > > while the driver api is "internal" we agreed some time ago
> > > > > > > > > that drivers
> > > > > > > > > could be built external to the dpdk tree. by enabling the
> > > > > > > > > meson setup
> > > > > > > > > option -Denable_driver_sdk=true.
> > > > > > > > >
> > > > > > > > > it was agreed that the driver api was internal and would
> > > > > > > > > attract no
> > > > > > > > > binary compatibility support which was fine. this change has
> > > > > > > > > now
> > > > > > > > > imposed a further restriction that out of tree drivers have
> > > > > > > > > to be
> > > > > > > > > authored in C only as non-C++ compatible code will invariably
> > > > > > > > > leak into
> > > > > > > > > the internal structures.
> > > > > > > > >
> > > > > > > > > you won't allow us to build C++ drivers in the dpdk tree and
> > > > > > > > > it seems
> > > > > > > > > now you are preventing building of C++ drivers outside of the
> > > > > > > > > tree too.
> > > > > > > >
> > > > > > > > That's the problem of non-written assumptions, they are unknown
> > > > > > > > or forgotten.
> > > > > > > > Did we agree to support out-of-tree drivers in C++?
> > > > > > > >
> > > > > > > > We really need to make things clear and written in
> > > > > > > > documentation.
> > > > > > > >
> > > > > > > > > could we please re-evaluate this.
> > > > > > > >
> > > > > > > > Yes we can re-evaluate.
> > > > > > > > What is the list of impacted files?
> > > > > > > >
> > > > > > > Hacking meson files a bit, the list of SDK header files is
> > > > > > > reported as below.
> > > > > > >
> > > > > > > /Bruce
> > > > > > >
> > > > > > > Message: SDK headers:
> > > > > > > Message: ethdev_driver.h
> > > > > > > Message: ethdev_pci.h
> > > > > > > Message: ethdev_vdev.h
> > > > > > > Message: cryptodev_pmd.h
> > > > > > > Message: eventdev_pmd.h
> > > > > > > Message: eventdev_pmd_pci.h
> > > > > > > Message: eventdev_pmd_vdev.h
> > > > > > > Message: eventdev_trace.h
> > > > > > > Message: event_timer_adapter_pmd.h
> > > > > > > Message: rte_dmadev_pmd.h
> > > > > > > Message: vdpa_driver.h
> > > > > >
> > > > > > I see no harm in supporting C++ include of these headers.
> > > > > > Any objection?
> > > > > >
> > > > > > Could we have a test in chkincs for the SDK headers?
> > > > > >
> > > > > Yes. It may make things a little more complicated, though, as it seems
> > > > > these headers also have a tendency to rely on some driver headers -
> > > > > specifically bus driver headers.
> > > > >
> > > >
> > > > Working on a patch to add these to checks. However, just running a c++
> > > > compile does not check for valid 'extern "C"' blocks. Adding the
> > > > following
> > > > to chkincs/meson.build as a basic sanity check throws up a larger list
> > > > of
> > > > files to be looked at.
> > > >
> > > > Diff:
> > > >
> > > > +# check for extern C in files, since this is not detected as an error
> > > > by the compiler
> > > > +grep = find_program('grep', required: false)
> > > > +if grep.found()
> > > > + errlist = run_command([grep, '--files-without-match', '^extern
> > > > "C"', dpdk_chkinc_headers],
> > > > + check: true, capture: true).stdout().split()
> > > > + if errlist != []
> > > > + error('Files missing C++ \'extern "C"\' guards:\n- ' + '\n-
> > > > '.join(errlist))
> > > > + endif
> > > > +endif
> > > > +
> > > >
> > > > Output:
> > > > ../buildtools/chkincs/meson.build:45:8: ERROR: Problem encountered:
> > > > Files missing C++ 'extern "C"' guards:
> > > > - /home/bruce/dpdk.org/lib/telemetry/rte_telemetry.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_bitops.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_branch_prediction.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_compat.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_hypervisor.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_keepalive.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_feature_defs.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_pci_dev_features.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_time.h
> > > > - /home/bruce/dpdk.org/lib/eal/include/rte_trace_point_register.h
> > > > - /home/bruce/dpdk.org/lib/eal/linux/include/rte_os.h
> > > > - /home/bruce/dpdk.org/lib/ethdev/rte_dev_info.h
> > > > - /home/bruce/dpdk.org/lib/ethdev/ethdev_driver.h
> > > > - /home/bruce/dpdk.org/lib/ethdev/ethdev_pci.h
> > > > - /home/bruce/dpdk.org/lib/ethdev/ethdev_vdev.h
> > > > - /home/bruce/dpdk.org/lib/metrics/rte_metrics_telemetry.h
> > > > - /home/bruce/dpdk.org/lib/acl/rte_acl_osdep.h
> > > > - /home/bruce/dpdk.org/lib/bpf/bpf_def.h
> > > > - /home/bruce/dpdk.org/lib/compressdev/rte_compressdev_internal.h
> > > > - /home/bruce/dpdk.org/lib/cryptodev/cryptodev_pmd.h
> > > > - /home/bruce/dpdk.org/lib/eventdev/rte_event_ring.h
> > > > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd.h
> > > > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_pci.h
> > > > - /home/bruce/dpdk.org/lib/eventdev/eventdev_pmd_vdev.h
> > > > - /home/bruce/dpdk.org/lib/kni/rte_kni_common.h
> > > > - /home/bruce/dpdk.org/lib/vhost/rte_vdpa.h
> > > > - /home/bruce/dpdk.org/lib/vhost/rte_vhost_async.h
> > > > - /home/bruce/dpdk.org/lib/vhost/rte_vhost_crypto.h
> > > > - /home/bruce/dpdk.org/lib/vhost/vdpa_driver.h
> > > >
> > > > Regards,
> > > > /Bruce
> > >
> > > The actual C++ guards are small and have no impact therefore we
> > > should accept patches as needed. Having more guards than needed is
> > > just noise.
> > >
> > > But the responsibility of telling DPDK project where they are required
> > > should fall to any out-of-tree users who care.
> >
> > Sorry Stephen I don't understand.
> > Please could you rephrase your comment?
>
> Since there are no in-tree DPDK drivers that use C++ it is an extra
> effort to add infrastructure to test if headers are broken. As new
> features get added to the DPDK, it doesn't make sense for the upstream
> DPDK to guarantee that no driver API changed in a way that breaks C++.
> But if a breakage happens in C++ driver, the upstream DPDK is willing
> to accept patches to fix it.
that is completely untenable. to have a policy where the consumers using
c++ have to detect the non-compatible changes and then react by
submitting patches to fix upstream is overly burdensome.
it is far less overhead to prevent the non-compatible code creaping into
the headers in the first place rather than pushing it down to the users
of dpdk.
