From: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
To: Shreyansh Jain <shreyansh.jain@nxp.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v1 2/8] bus: introduce opaque control framework
Date: Mon, 11 Dec 2017 13:43:59 +0100 [thread overview]
Message-ID: <20171211124359.zhyeaveywobmobef@bidouze.vm.6wind.com> (raw)
In-Reply-To: <088de7d2-9bd4-4a49-44ba-9df9c52b72d1@nxp.com>
On Mon, Dec 11, 2017 at 05:30:16PM +0530, Shreyansh Jain wrote:
> On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:
> > New configuration elements are added to the buses. They make the ABI
> > unstable and will continue to do so.
> >
> > This new control scheme allows to add new bus operators without
> > breaking the ABI and by only expanding the API.
> >
> > This helps having more stability in core EAL subsystems, while allowing
> > flexibility for future evolutions.
> >
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> > lib/librte_eal/common/eal_common_bus.c | 9 +++++++
> > lib/librte_eal/common/include/rte_bus.h | 46 +++++++++++++++++++++++++++++++++
> > 2 files changed, 55 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> > index 3c66a02..65d7229 100644
> > --- a/lib/librte_eal/common/eal_common_bus.c
> > +++ b/lib/librte_eal/common/eal_common_bus.c
> > @@ -42,6 +42,13 @@
> > struct rte_bus_list rte_bus_list =
> > TAILQ_HEAD_INITIALIZER(rte_bus_list);
> > +static rte_bus_ctrl_t
> > +rte_bus_default_ctrl(enum rte_bus_ctrl_op op __rte_unused,
> > + enum rte_bus_ctrl_item item __rte_unused)
> > +{
> > + return NULL;
> > +}
> > +
> > void
> > rte_bus_register(struct rte_bus *bus)
> > {
> > @@ -53,6 +60,8 @@ rte_bus_register(struct rte_bus *bus)
> > RTE_VERIFY(bus->find_device);
> > /* Buses supporting driver plug also require unplug. */
> > RTE_VERIFY(!bus->plug || bus->unplug);
> > + if (bus->ctrl == NULL)
> > + bus->ctrl = &rte_bus_default_ctrl;
> > TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
> > RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);
> > diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
> > index 331d954..bd3c28e 100644
> > --- a/lib/librte_eal/common/include/rte_bus.h
> > +++ b/lib/librte_eal/common/include/rte_bus.h
> > @@ -183,6 +183,51 @@ struct rte_bus_conf {
> > enum rte_bus_probe_mode probe_mode; /**< Probe policy. */
> > };
> > +/**
> > + * Bus configuration items.
> > + */
> > +enum rte_bus_ctrl_item {
> > + RTE_BUS_CTRL_PROBE_MODE = 0,
> > + RTE_BUS_CTRL_ITEM_MAX,
> > +};
>
> I am assuming that a driver implementation can take more than ITEM_MAX
> control knobs. It is opaque to the library. Are we on same page?
>
> For example, a bus driver can implement:
>
> rte_bus_XXX_ctrl_item {
> <Leaving space for allowing rte_bus.h implementations>
> RTE_BUS_XYZ_KNOB_1 = 100,
> RTE_BUS_XYZ_KNOB_2,
> RTE_BUS_XYZ_KNOB_3,
> };
>
> without the library knowing or restricting the API to RTE_BUS_CTRL_ITEM_MAX.
>
> I see that in your code for PCI (Patch 5/8: pci_ctrl) you have restricted
> the control knob to RTE_BUS_CTRL_ITEM_MAX.
> I hope that such restrictions would not float to library layer.
>
> If we are on same page, should this be documented as a code comment
> somewhere?
> if not, do you think what I am stating makes sense?
>
I see what you mean, but I'm not sure it would be a good thing.
Actually, I think proposing this ITEM_MAX was a mistake.
Regarding the specific bus knobs:
- If a single bus needs this knob, then it would be better for the dev
to add it as part of the bus' public API, following the correct
library versioning processes. This would not break this bus control
structure ABI.
- If more than one bus implement this knob, then it should be proposed
as part of the library API. Buses adding this new knob would break
their ABI, other buses would be left untouched.
This makes me realize that proposing this ITEM_MAX value is not good to
the intended purpose of this patchset:
- If a bus implementation use a reference to ITEM_MAX, then the control
structure ABI would be broken by any new control knob added, even if the
bus does not implement it. Granted, it would not break the driver
structure itself, but still. My PCI implementation is thus incorrect.
Therefore I think that it would be best to remove this ITEM_MAX altogether,
forcing bus developpers to use other ways that would not break their
ABIs every other release.
--
Gaëtan Rivet
6WIND
next prev parent reply other threads:[~2017-12-11 12:44 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-12 8:18 [dpdk-dev] [PATCH v1 0/8] Bus " Gaetan Rivet
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 1/8] bus: rename scan policy as probe policy Gaetan Rivet
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 2/8] bus: introduce opaque control framework Gaetan Rivet
2017-12-11 12:00 ` Shreyansh Jain
2017-12-11 12:43 ` Gaëtan Rivet [this message]
2017-12-11 13:36 ` Shreyansh Jain
2017-12-11 14:38 ` Gaëtan Rivet
2017-12-12 7:21 ` Shreyansh Jain
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 3/8] bus: remove probe mode configuration structure Gaetan Rivet
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 4/8] bus: add probe mode setter Gaetan Rivet
2017-12-11 12:39 ` Shreyansh Jain
2017-12-11 12:43 ` Shreyansh Jain
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 5/8] bus/pci: implement ctrl operator Gaetan Rivet
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 6/8] bus: add IOVA mode as a ctrl operation Gaetan Rivet
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 7/8] bus/pci: implement IOVA mode getter Gaetan Rivet
2017-10-12 8:18 ` [dpdk-dev] [PATCH v1 8/8] bus: remove redundant " Gaetan Rivet
2017-12-11 11:53 ` [dpdk-dev] [PATCH v1 0/8] Bus control framework Shreyansh Jain
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=20171211124359.zhyeaveywobmobef@bidouze.vm.6wind.com \
--to=gaetan.rivet@6wind.com \
--cc=dev@dpdk.org \
--cc=shreyansh.jain@nxp.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).