From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
To: Konstantin Ananyev <konstantin.ananyev@intel.com>, dev@dpdk.org
Cc: xiaoyun.li@intel.com, anoobj@marvell.com, jerinj@marvell.com,
ndabilpuram@marvell.com, adwivedi@marvell.com,
shepard.siegel@atomicrules.com, ed.czeck@atomicrules.com,
john.miller@atomicrules.com, irusskikh@marvell.com,
ajit.khaparde@broadcom.com, somnath.kotur@broadcom.com,
rahul.lakkireddy@chelsio.com, hemant.agrawal@nxp.com,
sachin.saxena@oss.nxp.com, haiyue.wang@intel.com,
johndale@cisco.com, hyonkim@cisco.com, qi.z.zhang@intel.com,
xiao.w.wang@intel.com, humin29@huawei.com,
yisen.zhuang@huawei.com, oulijun@huawei.com,
beilei.xing@intel.com, jingjing.wu@intel.com,
qiming.yang@intel.com, matan@nvidia.com, viacheslavo@nvidia.com,
sthemmin@microsoft.com, longli@microsoft.com,
heinrich.kuhn@corigine.com, kirankumark@marvell.com,
mczekaj@marvell.com, jiawenwu@trustnetic.com,
jianwang@trustnetic.com, maxime.coquelin@redhat.com,
chenbo.xia@intel.com, thomas@monjalon.net,
ferruh.yigit@intel.com, mdr@ashroe.eu,
jay.jayatheerthan@intel.com
Subject: Re: [dpdk-dev] [PATCH v5 4/7] ethdev: copy fast-path API into separate structure
Date: Mon, 11 Oct 2021 11:25:53 +0300 [thread overview]
Message-ID: <37c19f16-ce0d-533f-2861-176ddf416df7@oktetlabs.ru> (raw)
In-Reply-To: <20211007112750.25526-5-konstantin.ananyev@intel.com>
On 10/7/21 2:27 PM, Konstantin Ananyev wrote:
> Copy public function pointers (rx_pkt_burst(), etc.) and related
> pointers to internal data from rte_eth_dev structure into a
> separate flat array. That array will remain in a public header.
> The intention here is to make rte_eth_dev and related structures internal.
> That should allow future possible changes to core eth_dev structures
> to be transparent to the user and help to avoid ABI/API breakages.
> The plan is to keep minimal part of data from rte_eth_dev public,
> so we still can use inline functions for fast-path calls
> (like rte_eth_rx_burst(), etc.) to avoid/minimize slowdown.
> The whole idea beyond this new schema:
> 1. PMDs keep to setup fast-path function pointers and related data
> inside rte_eth_dev struct in the same way they did it before.
> 2. Inside rte_eth_dev_start() and inside rte_eth_dev_probing_finish()
> (for secondary process) we call eth_dev_fp_ops_setup, which
> copies these function and data pointers into rte_eth_fp_ops[port_id].
> 3. Inside rte_eth_dev_stop() and inside rte_eth_dev_release_port()
> we call eth_dev_fp_ops_reset(), which resets rte_eth_fp_ops[port_id]
> into some dummy values.
> 4. fast-path ethdev API (rte_eth_rx_burst(), etc.) will use that new
> flat array to call PMD specific functions.
> That approach should allow us to make rte_eth_devices[] private
> without introducing regression and help to avoid changes in drivers code.
>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Overall LGTM, few nits below.
> ---
> lib/ethdev/ethdev_private.c | 52 ++++++++++++++++++++++++++++++++++
> lib/ethdev/ethdev_private.h | 7 +++++
> lib/ethdev/rte_ethdev.c | 27 ++++++++++++++++++
> lib/ethdev/rte_ethdev_core.h | 55 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 141 insertions(+)
>
> diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
> index 012cf73ca2..3eeda6e9f9 100644
> --- a/lib/ethdev/ethdev_private.c
> +++ b/lib/ethdev/ethdev_private.c
> @@ -174,3 +174,55 @@ rte_eth_devargs_parse_representor_ports(char *str, void *data)
> RTE_LOG(ERR, EAL, "wrong representor format: %s\n", str);
> return str == NULL ? -1 : 0;
> }
> +
> +static uint16_t
> +dummy_eth_rx_burst(__rte_unused void *rxq,
> + __rte_unused struct rte_mbuf **rx_pkts,
> + __rte_unused uint16_t nb_pkts)
> +{
> + RTE_ETHDEV_LOG(ERR, "rx_pkt_burst for unconfigured port\n");
May be "unconfigured" -> "stopped" ? Or "non-started" ?
> + rte_errno = ENOTSUP;
> + return 0;
> +}
> +
> +static uint16_t
> +dummy_eth_tx_burst(__rte_unused void *txq,
> + __rte_unused struct rte_mbuf **tx_pkts,
> + __rte_unused uint16_t nb_pkts)
> +{
> + RTE_ETHDEV_LOG(ERR, "tx_pkt_burst for unconfigured port\n");
May be "unconfigured" -> "stopped" ?
> + rte_errno = ENOTSUP;
> + return 0;
> +}
> +
> +void
> +eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo)
> +{
> + static void *dummy_data[RTE_MAX_QUEUES_PER_PORT];
> + static const struct rte_eth_fp_ops dummy_ops = {
> + .rx_pkt_burst = dummy_eth_rx_burst,
> + .tx_pkt_burst = dummy_eth_tx_burst,
> + .rxq = {.data = dummy_data, .clbk = dummy_data,},
> + .txq = {.data = dummy_data, .clbk = dummy_data,},
> + };
> +
> + *fpo = dummy_ops;
> +}
> +
> +void
> +eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
> + const struct rte_eth_dev *dev)
> +{
> + fpo->rx_pkt_burst = dev->rx_pkt_burst;
> + fpo->tx_pkt_burst = dev->tx_pkt_burst;
> + fpo->tx_pkt_prepare = dev->tx_pkt_prepare;
> + fpo->rx_queue_count = dev->rx_queue_count;
> + fpo->rx_descriptor_status = dev->rx_descriptor_status;
> + fpo->tx_descriptor_status = dev->tx_descriptor_status;
> +
> + fpo->rxq.data = dev->data->rx_queues;
> + fpo->rxq.clbk = (void **)(uintptr_t)dev->post_rx_burst_cbs;
> +
> + fpo->txq.data = dev->data->tx_queues;
> + fpo->txq.clbk = (void **)(uintptr_t)dev->pre_tx_burst_cbs;
> +}
> diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
> index 3724429577..5721be7bdc 100644
> --- a/lib/ethdev/ethdev_private.h
> +++ b/lib/ethdev/ethdev_private.h
> @@ -26,4 +26,11 @@ eth_find_device(const struct rte_eth_dev *_start, rte_eth_cmp_t cmp,
> /* Parse devargs value for representor parameter. */
> int rte_eth_devargs_parse_representor_ports(char *str, void *data);
>
> +/* reset eth fast-path API to dummy values */
> +void eth_dev_fp_ops_reset(struct rte_eth_fp_ops *fpo);
> +
> +/* setup eth fast-path API to ethdev values */
> +void eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
> + const struct rte_eth_dev *dev);
> +
> #endif /* _ETH_PRIVATE_H_ */
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index c8abda6dd7..9f7a0cbb8c 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -44,6 +44,9 @@
> static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
> struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
>
> +/* public fast-path API */
Shoudn't it be a doxygen style comment?
> +struct rte_eth_fp_ops rte_eth_fp_ops[RTE_MAX_ETHPORTS];
> +
> /* spinlock for eth device callbacks */
> static rte_spinlock_t eth_dev_cb_lock = RTE_SPINLOCK_INITIALIZER;
>
[snip]
> diff --git a/lib/ethdev/rte_ethdev_core.h b/lib/ethdev/rte_ethdev_core.h
> index 51cd68de94..d5853dff86 100644
> --- a/lib/ethdev/rte_ethdev_core.h
> +++ b/lib/ethdev/rte_ethdev_core.h
> @@ -50,6 +50,61 @@ typedef int (*eth_rx_descriptor_status_t)(void *rxq, uint16_t offset);
> typedef int (*eth_tx_descriptor_status_t)(void *txq, uint16_t offset);
> /**< @internal Check the status of a Tx descriptor */
>
> +/**
> + * @internal
> + * Structure used to hold opaque pointers to internal ethdev Rx/Tx
> + * queues data.
> + * The main purpose to expose these pointers at all - allow compiler
> + * to fetch this data for fast-path ethdev inline functions in advance.
> + */
> +struct rte_ethdev_qdata {
> + void **data;
> + /**< points to array of internal queue data pointers */
Please, put the documentation on the same like or just
put documentation before the documented member.
> + void **clbk;
> + /**< points to array of queue callback data pointers */
> +};
> +
> +/**
> + * @internal
> + * fast-path ethdev functions and related data are hold in a flat array.
> + * One entry per ethdev.
> + * On 64-bit systems contents of this structure occupy exactly two 64B lines.
> + * On 32-bit systems contents of this structure fits into one 64B line.
> + */
> +struct rte_eth_fp_ops {
> +
> + /**
> + * Rx fast-path functions and related data.
> + * 64-bit systems: occupies first 64B line
> + */
As I understand the above comment is for a group of below
fields. If so, Doxygen annocation for member groups should
be used.
> + eth_rx_burst_t rx_pkt_burst;
> + /**< PMD receive function. */
May I ask to avoid usage of documentation after member in a
separate line. It makes sense to if it is located in the same
line, but otherwise, it should be simply put before the
documented member.
[snip]
next prev parent reply other threads:[~2021-10-11 8:25 UTC|newest]
Thread overview: 112+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-20 16:28 [dpdk-dev] [RFC 0/7] hide eth dev related structures Konstantin Ananyev
2021-08-20 16:28 ` [dpdk-dev] [RFC 1/7] eth: move ethdev 'burst' API into separate structure Konstantin Ananyev
2021-08-20 16:28 ` [dpdk-dev] [RFC 2/7] eth: make drivers to use new API for Rx Konstantin Ananyev
2021-09-06 18:41 ` Ferruh Yigit
2021-09-14 14:28 ` Ananyev, Konstantin
2021-08-20 16:28 ` [dpdk-dev] [RFC 3/7] eth: make drivers to use new API for Tx Konstantin Ananyev
2021-08-20 16:28 ` [dpdk-dev] [RFC 4/7] eth: make drivers to use new API for Tx prepare Konstantin Ananyev
2021-08-20 16:28 ` [dpdk-dev] [RFC 5/7] eth: make drivers to use new API to obtain descriptor status Konstantin Ananyev
2021-08-20 16:28 ` [dpdk-dev] [RFC 6/7] eth: make drivers to use new API for Rx queue count Konstantin Ananyev
2021-08-20 16:28 ` [dpdk-dev] [RFC 7/7] eth: hide eth dev related structures Konstantin Ananyev
2021-08-26 12:37 ` [dpdk-dev] [RFC 0/7] " Jerin Jacob
2021-09-06 18:09 ` Ferruh Yigit
2021-09-14 13:33 ` Ananyev, Konstantin
2021-09-15 9:45 ` Jerin Jacob
2021-09-22 15:08 ` Ananyev, Konstantin
2021-09-27 16:14 ` Jerin Jacob
2021-09-28 9:37 ` Ananyev, Konstantin
2021-09-22 14:09 ` [dpdk-dev] [RFC v2 0/5] " Konstantin Ananyev
2021-09-22 14:09 ` [dpdk-dev] [RFC v2 1/5] ethdev: allocate max space for internal queue array Konstantin Ananyev
2021-09-22 14:09 ` [dpdk-dev] [RFC v2 2/5] ethdev: change input parameters for rx_queue_count Konstantin Ananyev
2021-09-23 5:51 ` Wang, Haiyue
2021-09-22 14:09 ` [dpdk-dev] [RFC v2 3/5] ethdev: copy ethdev 'burst' API into separate structure Konstantin Ananyev
2021-09-23 5:58 ` Wang, Haiyue
2021-09-27 18:01 ` Jerin Jacob
2021-09-28 9:42 ` Ananyev, Konstantin
2021-09-22 14:09 ` [dpdk-dev] [RFC v2 4/5] ethdev: make burst functions to use new flat array Konstantin Ananyev
2021-09-22 14:09 ` [dpdk-dev] [RFC v2 5/5] ethdev: hide eth dev related structures Konstantin Ananyev
2021-10-01 14:02 ` [dpdk-dev] [PATCH v3 0/7] " Konstantin Ananyev
2021-10-01 14:02 ` [dpdk-dev] [PATCH v3 1/7] ethdev: allocate max space for internal queue array Konstantin Ananyev
2021-10-01 16:48 ` Ferruh Yigit
2021-10-01 14:02 ` [dpdk-dev] [PATCH v3 2/7] ethdev: change input parameters for rx_queue_count Konstantin Ananyev
2021-10-01 14:02 ` [dpdk-dev] [PATCH v3 3/7] ethdev: copy ethdev 'fast' API into separate structure Konstantin Ananyev
2021-10-01 14:02 ` [dpdk-dev] [PATCH v3 4/7] ethdev: make burst functions to use new flat array Konstantin Ananyev
2021-10-01 16:46 ` Ferruh Yigit
2021-10-01 17:40 ` Ananyev, Konstantin
2021-10-04 8:46 ` Ferruh Yigit
2021-10-04 9:20 ` Ananyev, Konstantin
2021-10-04 10:13 ` Ferruh Yigit
2021-10-04 11:17 ` Ananyev, Konstantin
2021-10-01 14:02 ` [dpdk-dev] [PATCH v3 5/7] ethdev: add API to retrieve multiple ethernet addresses Konstantin Ananyev
2021-10-01 14:02 ` [dpdk-dev] [PATCH v3 6/7] ethdev: remove legacy Rx descriptor done API Konstantin Ananyev
2021-10-01 14:02 ` [dpdk-dev] [PATCH v3 7/7] ethdev: hide eth dev related structures Konstantin Ananyev
2021-10-01 16:53 ` Ferruh Yigit
2021-10-01 17:04 ` Ferruh Yigit
2021-10-01 17:02 ` [dpdk-dev] [PATCH v3 0/7] " Ferruh Yigit
2021-10-04 13:55 ` [dpdk-dev] [PATCH v4 " Konstantin Ananyev
2021-10-04 13:55 ` [dpdk-dev] [PATCH v4 1/7] ethdev: allocate max space for internal queue array Konstantin Ananyev
2021-10-05 12:09 ` Thomas Monjalon
2021-10-05 16:45 ` Ananyev, Konstantin
2021-10-05 16:49 ` Thomas Monjalon
2021-10-05 12:21 ` Thomas Monjalon
2021-10-04 13:55 ` [dpdk-dev] [PATCH v4 2/7] ethdev: change input parameters for rx_queue_count Konstantin Ananyev
2021-10-04 13:55 ` [dpdk-dev] [PATCH v4 3/7] ethdev: copy ethdev 'fast' API into separate structure Konstantin Ananyev
2021-10-05 13:09 ` Thomas Monjalon
2021-10-05 16:41 ` Ananyev, Konstantin
2021-10-05 16:48 ` Thomas Monjalon
2021-10-05 17:04 ` Ananyev, Konstantin
2021-10-04 13:56 ` [dpdk-dev] [PATCH v4 4/7] ethdev: make burst functions to use new flat array Konstantin Ananyev
2021-10-05 9:54 ` David Marchand
2021-10-05 10:13 ` Ananyev, Konstantin
2021-10-04 13:56 ` [dpdk-dev] [PATCH v4 5/7] ethdev: add API to retrieve multiple ethernet addresses Konstantin Ananyev
2021-10-05 13:13 ` Thomas Monjalon
2021-10-05 16:35 ` Ananyev, Konstantin
2021-10-05 16:45 ` Thomas Monjalon
2021-10-05 17:12 ` Ananyev, Konstantin
2021-10-05 17:41 ` Thomas Monjalon
2021-10-04 13:56 ` [dpdk-dev] [PATCH v4 6/7] ethdev: remove legacy Rx descriptor done API Konstantin Ananyev
2021-10-05 13:14 ` Thomas Monjalon
2021-10-05 16:21 ` Ananyev, Konstantin
2021-10-04 13:56 ` [dpdk-dev] [PATCH v4 7/7] ethdev: hide eth dev related structures Konstantin Ananyev
2021-10-05 10:04 ` David Marchand
2021-10-05 10:43 ` Ferruh Yigit
2021-10-05 11:37 ` David Marchand
2021-10-05 15:57 ` Ananyev, Konstantin
2021-10-05 13:24 ` Thomas Monjalon
2021-10-05 16:19 ` Ananyev, Konstantin
2021-10-05 16:25 ` Thomas Monjalon
2021-10-06 16:42 ` [dpdk-dev] [PATCH v4 0/7] " Ali Alnubani
2021-10-06 17:26 ` Ali Alnubani
2021-10-07 11:27 ` [dpdk-dev] [PATCH v5 " Konstantin Ananyev
2021-10-07 11:27 ` [dpdk-dev] [PATCH v5 1/7] ethdev: remove legacy Rx descriptor done API Konstantin Ananyev
2021-10-07 11:27 ` [dpdk-dev] [PATCH v5 2/7] ethdev: allocate max space for internal queue array Konstantin Ananyev
2021-10-11 9:20 ` Andrew Rybchenko
2021-10-11 16:25 ` Ananyev, Konstantin
2021-10-11 17:15 ` Andrew Rybchenko
2021-10-11 23:06 ` Ananyev, Konstantin
2021-10-12 5:47 ` Andrew Rybchenko
2021-10-07 11:27 ` [dpdk-dev] [PATCH v5 3/7] ethdev: change input parameters for rx_queue_count Konstantin Ananyev
2021-10-11 8:06 ` Andrew Rybchenko
2021-10-12 17:59 ` Hyong Youb Kim (hyonkim)
2021-10-07 11:27 ` [dpdk-dev] [PATCH v5 4/7] ethdev: copy fast-path API into separate structure Konstantin Ananyev
2021-10-09 12:05 ` fengchengwen
2021-10-11 1:18 ` fengchengwen
2021-10-11 8:39 ` Andrew Rybchenko
2021-10-11 15:24 ` Ananyev, Konstantin
2021-10-11 8:35 ` Andrew Rybchenko
2021-10-11 15:15 ` Ananyev, Konstantin
2021-10-11 8:25 ` Andrew Rybchenko [this message]
2021-10-11 16:52 ` Ananyev, Konstantin
2021-10-11 17:22 ` Andrew Rybchenko
2021-10-07 11:27 ` [dpdk-dev] [PATCH v5 5/7] ethdev: make fast-path functions to use new flat array Konstantin Ananyev
2021-10-11 9:02 ` Andrew Rybchenko
2021-10-11 15:47 ` Ananyev, Konstantin
2021-10-11 17:03 ` Andrew Rybchenko
2021-10-07 11:27 ` [dpdk-dev] [PATCH v5 6/7] ethdev: add API to retrieve multiple ethernet addresses Konstantin Ananyev
2021-10-11 9:09 ` Andrew Rybchenko
2021-10-07 11:27 ` [dpdk-dev] [PATCH v5 7/7] ethdev: hide eth dev related structures Konstantin Ananyev
2021-10-11 9:20 ` Andrew Rybchenko
2021-10-11 15:54 ` Ananyev, Konstantin
2021-10-11 17:04 ` Andrew Rybchenko
2021-10-08 18:13 ` [dpdk-dev] [PATCH v5 0/7] " Slava Ovsiienko
2021-10-11 9:22 ` Andrew Rybchenko
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=37c19f16-ce0d-533f-2861-176ddf416df7@oktetlabs.ru \
--to=andrew.rybchenko@oktetlabs.ru \
--cc=adwivedi@marvell.com \
--cc=ajit.khaparde@broadcom.com \
--cc=anoobj@marvell.com \
--cc=beilei.xing@intel.com \
--cc=chenbo.xia@intel.com \
--cc=dev@dpdk.org \
--cc=ed.czeck@atomicrules.com \
--cc=ferruh.yigit@intel.com \
--cc=haiyue.wang@intel.com \
--cc=heinrich.kuhn@corigine.com \
--cc=hemant.agrawal@nxp.com \
--cc=humin29@huawei.com \
--cc=hyonkim@cisco.com \
--cc=irusskikh@marvell.com \
--cc=jay.jayatheerthan@intel.com \
--cc=jerinj@marvell.com \
--cc=jianwang@trustnetic.com \
--cc=jiawenwu@trustnetic.com \
--cc=jingjing.wu@intel.com \
--cc=john.miller@atomicrules.com \
--cc=johndale@cisco.com \
--cc=kirankumark@marvell.com \
--cc=konstantin.ananyev@intel.com \
--cc=longli@microsoft.com \
--cc=matan@nvidia.com \
--cc=maxime.coquelin@redhat.com \
--cc=mczekaj@marvell.com \
--cc=mdr@ashroe.eu \
--cc=ndabilpuram@marvell.com \
--cc=oulijun@huawei.com \
--cc=qi.z.zhang@intel.com \
--cc=qiming.yang@intel.com \
--cc=rahul.lakkireddy@chelsio.com \
--cc=sachin.saxena@oss.nxp.com \
--cc=shepard.siegel@atomicrules.com \
--cc=somnath.kotur@broadcom.com \
--cc=sthemmin@microsoft.com \
--cc=thomas@monjalon.net \
--cc=viacheslavo@nvidia.com \
--cc=xiao.w.wang@intel.com \
--cc=xiaoyun.li@intel.com \
--cc=yisen.zhuang@huawei.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).