DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: Thomas Monjalon <thomas@monjalon.net>,
	dev@dpdk.org, Tyler Retzlaff <roretzla@linux.microsoft.com>,
	ocardona@microsoft.com, roretzla@microsoft.com,
	david.marchand@redhat.com, ferruh.yigit@intel.com
Subject: Re: out of tree driver builds broken with C++
Date: Mon, 14 Feb 2022 18:03:17 +0000	[thread overview]
Message-ID: <YgqZZaCXZ2vOrqyA@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <20220214092435.6fb6964e@hermes.local>

On Mon, Feb 14, 2022 at 09:24:35AM -0800, Stephen Hemminger wrote:
> On Mon, 14 Feb 2022 15:03:43 +0000
> Bruce Richardson <bruce.richardson@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.

The guards in the above files are all needed, I believe. The scan for
chkincs only runs on header files that are public, or part of the
driver_sdk set. Internal headers aren't scanned and don't need the guards.

/Bruce

  parent reply	other threads:[~2022-02-14 18:03 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-14  9:13 Tyler Retzlaff
2022-02-14  9:22 ` Thomas Monjalon
2022-02-14 10:45   ` Bruce Richardson
2022-02-14 11:19     ` Thomas Monjalon
2022-02-14 11:43       ` Bruce Richardson
2022-02-14 15:03         ` Bruce Richardson
2022-02-14 17:24           ` Stephen Hemminger
2022-02-14 17:49             ` Thomas Monjalon
2022-02-14 18:21               ` Stephen Hemminger
2022-02-14 18:32                 ` Tyler Retzlaff
2022-02-14 18:40                   ` Tyler Retzlaff
2022-02-14 18:03             ` Bruce Richardson [this message]
2022-02-15  6:10   ` Tyler Retzlaff
2022-02-15  8:55     ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YgqZZaCXZ2vOrqyA@bricha3-MOBL.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=ocardona@microsoft.com \
    --cc=roretzla@linux.microsoft.com \
    --cc=roretzla@microsoft.com \
    --cc=stephen@networkplumber.org \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).