From: Neil Horman <nhorman@tuxdriver.com>
To: David Marchand <david.marchand@6wind.com>
Cc: Thomas Monjalon <thomas.monjalon@6wind.com>,
"dev@dpdk.org" <dev@dpdk.org>,
Jan Viktorin <viktorin@rehivetech.com>
Subject: Re: [dpdk-dev] [PATCH 09/11] eal: move PCI table macro
Date: Fri, 8 Jul 2016 09:56:43 -0400 [thread overview]
Message-ID: <20160708135643.GC14917@hmsreliant.think-freely.org> (raw)
In-Reply-To: <CALwxeUv_davrQO+wkTr8Ciqyt3LjgYooRaxN5aRzFLmE8vY6bg@mail.gmail.com>
On Fri, Jul 08, 2016 at 10:49:25AM +0200, David Marchand wrote:
> Hello Thomas, Neil,
>
> (will be back in a couple of days, thanks Thomas for pointing this thread)
>
>
> On Thu, Jul 7, 2016 at 6:25 PM, Thomas Monjalon
> <thomas.monjalon@6wind.com> wrote:
> > 2016-07-07 12:11, Neil Horman:
> >> On Thu, Jul 07, 2016 at 05:36:28PM +0200, Thomas Monjalon wrote:
> >> > Remove include of rte_pci.h in the generic header rte_dev.h
> >> > and move the macro DRIVER_REGISTER_PCI_TABLE in rte_pci.h.
> > [...]
> >>
> >> This seems strange to me, in that its odd for the driver information export
> >> macros to be spread out in multiple locations. Specifically it enjoins the use
> >> of the DRV_EXP_TAG macro, which helps centralize tag naming. Perhaps the happy
> >> medium is to place all the export macros (includnig PMD_REGISTER_DRIVER) into
> >> its own pmd_register.h header?
> >
> > I don't know.
> > David, your opinion?
>
> - The suggestion I did offline to Thomas was to move pci stuff in pci headers.
> We are trying to move from the "all pci" code in eal to accomodate for
> other "buses" / architectures.
I get that, but I'm not sure that applies here. The macro in question is
specific to pci busses, and if there is additional bus information to export, it
will have its own macro (e.g. DRIVER_REGISTER_USB_TABLE or some such). While
I could see that being an argument for putting each macro in with its own bus
type, I think thats the wrong organization here, in that people writing drivers
will want to know what export macros are available and will expect to look in a
single place for it.
> Having a pci macro in a generic header like rte_dev.h is wrong to me.
> Moving this to a new header like pmd_register.h sounds like a new
> generic header with pci specific stuff in it.
Well, yes, but I see that as no different than rte_ethdev.c or rte_pdump.c.
both of those files will need to know about all the different types of busses
you support and have to include those corresponding header files (i.e. they will
have to include rte_pci.h, rte_usb.h, rte_i2c.h, etc). This is really no
different in my mind.
> So, I am not sure I follow you Neil.
>
> Can you elaborate ?
>
I suppose the best way to describe it is that while I understand and support the
desire to separate and abstract bus information away from device function, I
think theres a pragmatic descision here to prioritize functional domain over
header inclusion. That is to say, I think when people are writing a driver, it
will be helpful to have all the export macros in a single location so they know
what information they can export, and that includes registration of various bus
type identifiers. So a file like pmd_registration.h that includes rte_pci.h,
rte_usb.h, rte_i2c.h, etc is more useful to a developer, than spreading these
macros out to those various header files, for the sake of avoiding a potentially
unneeded include.
>
> - Why do you want to centralise the tag naming ?
> To avoid collisions ?
Yes, and to centralize that information. Since the pmdinfogen tool needs to
know what those tag names are as well, its useful to keep them in the same area
to maintain co-ordination. Its also useful because it means we can use one
macro to define tag naming convention, instead of having to re-implement or
dead-reckon it in multiple files.
> Well, adding those tags should not happen that often and I think we
> can maintain this with careful reviews.
I don't agree with that. This discussion is based on the fact that you expect
that we will be adding additional bus types in the future correct? Well, given
that we have a pci bus specific export macro, I would expect that to proliferate
to every other bus type as well, and so we can expect to at least have a new
tag added for every bus that is added, in addition to any other bus agnostic
information people wish to export (just off hand, looking at the linux modinfo
section, we might expect module author, module version, alias names, licensing
infomration, and others to be potential export candidates). So, depending on
how much this is adopted, I think we can potentially expect a great deal of
additional tagging to be needed.
Neil
next prev parent reply other threads:[~2016-07-08 13:57 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-07 15:36 [dpdk-dev] [PATCH 00/11] additions to pmdinfogen Thomas Monjalon
2016-07-07 15:36 ` [dpdk-dev] [PATCH 01/11] drivers: fix build with new register macro Thomas Monjalon
2016-07-07 15:50 ` Neil Horman
2016-07-07 16:20 ` Adrien Mazarguil
2016-07-07 15:36 ` [dpdk-dev] [PATCH 02/11] crypto: fix parameters registration Thomas Monjalon
2016-07-07 15:53 ` Neil Horman
2016-07-07 15:36 ` [dpdk-dev] [PATCH 03/11] mk: fix build dependency of drivers on pmdinfogen Thomas Monjalon
2016-07-07 15:56 ` Neil Horman
2016-07-07 16:21 ` Thomas Monjalon
2016-07-07 17:44 ` Neil Horman
2016-07-07 15:36 ` [dpdk-dev] [PATCH 04/11] mk: remove traces of hostapp build directory Thomas Monjalon
2016-07-07 16:02 ` Neil Horman
2016-07-07 15:36 ` [dpdk-dev] [PATCH 05/11] mk: fix driver build with installed SDK Thomas Monjalon
2016-07-07 15:36 ` [dpdk-dev] [PATCH 06/11] mk: fix verbose pmdinfogen run Thomas Monjalon
2016-07-07 16:04 ` Neil Horman
2016-07-07 15:36 ` [dpdk-dev] [PATCH 07/11] pmdinfogen: fix build warnings Thomas Monjalon
2016-07-07 17:55 ` Neil Horman
2016-07-07 21:25 ` Mcnamara, John
2016-07-08 14:51 ` Neil Horman
2016-07-07 15:36 ` [dpdk-dev] [PATCH 08/11] pmdinfogen: fix usage message Thomas Monjalon
2016-07-07 16:05 ` Neil Horman
2016-07-07 16:24 ` Thomas Monjalon
2016-07-07 17:46 ` Neil Horman
2016-07-07 15:36 ` [dpdk-dev] [PATCH 09/11] eal: move PCI table macro Thomas Monjalon
2016-07-07 15:41 ` Thomas Monjalon
2016-07-07 16:11 ` Neil Horman
2016-07-07 16:25 ` Thomas Monjalon
2016-07-08 8:49 ` David Marchand
2016-07-08 13:56 ` Neil Horman [this message]
2016-07-08 14:03 ` Thomas Monjalon
2016-07-08 14:13 ` Neil Horman
2016-07-07 15:36 ` [dpdk-dev] [PATCH 10/11] doc: fix syntax in pmdinfogen guide Thomas Monjalon
2016-07-07 15:42 ` Thomas Monjalon
2016-07-07 16:12 ` Neil Horman
2016-07-07 15:36 ` [dpdk-dev] [PATCH 11/11] maintainers: add section for pmdinfo Thomas Monjalon
2016-07-07 15:45 ` Thomas Monjalon
2016-07-07 16:14 ` Neil Horman
2016-07-07 16:26 ` Thomas Monjalon
2016-07-07 16:14 ` Neil Horman
2016-07-08 10:14 ` [dpdk-dev] [PATCH v2 00/10] additions to pmdinfogen Thomas Monjalon
2016-07-08 10:14 ` [dpdk-dev] [PATCH v2 01/10] drivers: fix build with new register macro Thomas Monjalon
2016-07-08 10:14 ` [dpdk-dev] [PATCH v2 02/10] mk: fix build dependency of drivers on pmdinfogen Thomas Monjalon
2016-07-08 10:14 ` [dpdk-dev] [PATCH v2 03/10] mk: remove traces of hostapp build directory Thomas Monjalon
2016-07-08 10:14 ` [dpdk-dev] [PATCH v2 04/10] mk: fix driver build with installed SDK Thomas Monjalon
2016-07-08 10:14 ` [dpdk-dev] [PATCH v2 05/10] mk: fix verbose pmdinfogen run Thomas Monjalon
2016-07-08 10:14 ` [dpdk-dev] [PATCH v2 06/10] pmdinfogen: fix build warnings Thomas Monjalon
2016-07-08 10:14 ` [dpdk-dev] [PATCH v2 07/10] pmdinfogen: fix usage message Thomas Monjalon
2016-07-08 10:14 ` [dpdk-dev] [PATCH v2 08/10] eal: move PCI table macro Thomas Monjalon
2016-07-08 10:14 ` [dpdk-dev] [PATCH v2 09/10] doc: fix syntax in pmdinfogen guide Thomas Monjalon
2016-07-08 10:14 ` [dpdk-dev] [PATCH v2 10/10] maintainers: add section for pmdinfo Thomas Monjalon
2016-07-08 14:42 ` [dpdk-dev] [PATCH v3 00/10] additions to pmdinfogen Thomas Monjalon
2016-07-08 14:42 ` [dpdk-dev] [PATCH v3 01/10] drivers: fix build with new register macro Thomas Monjalon
2016-07-08 14:42 ` [dpdk-dev] [PATCH v3 02/10] mk: fix build dependency of drivers on pmdinfogen Thomas Monjalon
2016-07-08 14:42 ` [dpdk-dev] [PATCH v3 03/10] mk: remove traces of hostapp build directory Thomas Monjalon
2016-07-08 14:42 ` [dpdk-dev] [PATCH v3 04/10] mk: fix driver build with installed SDK Thomas Monjalon
2016-07-08 14:53 ` Neil Horman
2016-07-08 14:42 ` [dpdk-dev] [PATCH v3 05/10] mk: fix verbose pmdinfogen run Thomas Monjalon
2016-07-08 14:42 ` [dpdk-dev] [PATCH v3 06/10] pmdinfogen: fix build warnings Thomas Monjalon
2016-07-08 15:23 ` Neil Horman
2016-07-08 14:42 ` [dpdk-dev] [PATCH v3 07/10] pmdinfogen: fix usage message Thomas Monjalon
2016-07-08 15:25 ` Neil Horman
2016-07-08 14:42 ` [dpdk-dev] [PATCH v3 08/10] eal: remove PCI include from generic driver header Thomas Monjalon
2016-07-08 15:26 ` Neil Horman
2016-07-08 14:42 ` [dpdk-dev] [PATCH v3 09/10] doc: fix syntax in pmdinfogen guide Thomas Monjalon
2016-07-08 14:42 ` [dpdk-dev] [PATCH v3 10/10] maintainers: add section for pmdinfo Thomas Monjalon
2016-07-08 15:54 ` [dpdk-dev] [PATCH v3 00/10] additions to pmdinfogen 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=20160708135643.GC14917@hmsreliant.think-freely.org \
--to=nhorman@tuxdriver.com \
--cc=david.marchand@6wind.com \
--cc=dev@dpdk.org \
--cc=thomas.monjalon@6wind.com \
--cc=viktorin@rehivetech.com \
/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).