mbox series

[v6,0/6] hide eth dev related structures

Message ID 20211013133704.31296-1-konstantin.ananyev@intel.com (mailing list archive)
Headers show
Series hide eth dev related structures | expand

Message

Ananyev, Konstantin Oct. 13, 2021, 1:36 p.m. UTC
v6 changes:
- Update comments (Andrew)
- Move callback related variables under corresponding ifdefs (Andrew)
- Few nits in rte_eth_macaddrs_get (Andrew)
- Rebased on top of next-net tree

v5 changes:
- Fix spelling (Thomas/David)
- Rename internal helper functions (David)
- Reorder patches and update commit messages (Thomas)
- Update comments (Thomas)
- Changed layout in rte_eth_fp_ops, to group functions and
   related data based on their functionality:
   first 64B line for Rx, second one for Tx.
   Didn't observe any real performance difference comparing to
   original layout. Though decided to keep a new one, as it seems
   a bit more plausible.

v4 changes:
 - Fix secondary process attach (Pavan)
 - Fix build failure (Ferruh)
 - Update lib/ethdev/verion.map (Ferruh)
   Note that moving newly added symbols from EXPERIMENTAL to DPDK_22
   section makes checkpatch.sh to complain.

v3 changes:
 - Changes in public struct naming (Jerin/Haiyue)
 - Split patches
 - Update docs
 - Shamelessly included Andrew's patch:
   https://patches.dpdk.org/project/dpdk/patch/20210928154856.1015020-1-andrew.rybchenko@oktetlabs.ru/
   into these series.
   I have to do similar thing here, so decided to avoid duplicated effort.

The aim of these patch series is to make rte_ethdev core data structures
(rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback, etc.) internal to
DPDK and not visible to the user.
That should allow future possible changes to core ethdev related structures
to be transparent to the user and help to improve ABI/API stability.
Note that current ethdev API is preserved, but it is a formal ABI break.

The work is based on previous discussions at:
https://www.mail-archive.com/dev@dpdk.org/msg211405.html
https://www.mail-archive.com/dev@dpdk.org/msg216685.html
and consists of the following main points:
1. Copy public 'fast' function pointers (rx_pkt_burst(), etc.) and
   related data pointer from rte_eth_dev into a separate flat array.
   We keep it public to still be able to use inline functions for these
   'fast' calls (like rte_eth_rx_burst(), etc.) to avoid/minimize slowdown.
   Note that apart from function pointers itself, each element of this
   flat array also contains two opaque pointers for each ethdev:
   1) a pointer to an array of internal queue data pointers
   2)  points to array of queue callback data pointers.
   Note that exposing this extra information allows us to avoid extra
   changes inside PMD level, plus should help to avoid possible
   performance degradation.
2. Change implementation of 'fast' inline ethdev functions
   (rte_eth_rx_burst(), etc.) to use new public flat array.
   While it is an ABI breakage, this change is intended to be transparent
   for both users (no changes in user app is required) and PMD developers
   (no changes in PMD is required).
   One extra note - with new implementation RX/TX callback invocation
   will cost one extra function call with this changes. That might cause
   some slowdown for code-path with RX/TX callbacks heavily involved.
   Hope such trade-off is acceptable for the community.
3. Move rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback and related
   things into internal header: <ethdev_driver.h>.

That approach was selected to:
  - Avoid(/minimize) possible performance losses.
  - Minimize required changes inside PMDs.

Performance testing results (ICX 2.0GHz, E810 (ice)):
 - testpmd macswap fwd mode, plus
   a) no RX/TX callbacks:
      no actual slowdown observed
   b) bpf-load rx 0 0 JM ./dpdk.org/examples/bpf/t3.o:
      ~2% slowdown
 - l3fwd: no actual slowdown observed

Would like to thank everyone who already reviewed and tested previous
versions of these series. All other interested parties please don't be shy
and provide your feedback.

Konstantin Ananyev (6):
  ethdev: allocate max space for internal queue array
  ethdev: change input parameters for rx_queue_count
  ethdev: copy fast-path API into separate structure
  ethdev: make fast-path functions to use new flat array
  ethdev: add API to retrieve multiple ethernet addresses
  ethdev: hide eth dev related structures

 app/test-pmd/config.c                         |  23 +-
 doc/guides/rel_notes/release_21_11.rst        |  17 +
 drivers/common/octeontx2/otx2_sec_idev.c      |   2 +-
 drivers/crypto/octeontx2/otx2_cryptodev_ops.c |   2 +-
 drivers/net/ark/ark_ethdev_rx.c               |   4 +-
 drivers/net/ark/ark_ethdev_rx.h               |   3 +-
 drivers/net/atlantic/atl_ethdev.h             |   2 +-
 drivers/net/atlantic/atl_rxtx.c               |   9 +-
 drivers/net/bnxt/bnxt_ethdev.c                |   8 +-
 drivers/net/cxgbe/base/adapter.h              |   2 +-
 drivers/net/dpaa/dpaa_ethdev.c                |   9 +-
 drivers/net/dpaa2/dpaa2_ethdev.c              |   9 +-
 drivers/net/dpaa2/dpaa2_ptp.c                 |   2 +-
 drivers/net/e1000/e1000_ethdev.h              |   6 +-
 drivers/net/e1000/em_rxtx.c                   |   4 +-
 drivers/net/e1000/igb_rxtx.c                  |   4 +-
 drivers/net/enic/enic_ethdev.c                |  12 +-
 drivers/net/fm10k/fm10k.h                     |   2 +-
 drivers/net/fm10k/fm10k_rxtx.c                |   4 +-
 drivers/net/hns3/hns3_rxtx.c                  |   7 +-
 drivers/net/hns3/hns3_rxtx.h                  |   2 +-
 drivers/net/i40e/i40e_rxtx.c                  |   4 +-
 drivers/net/i40e/i40e_rxtx.h                  |   3 +-
 drivers/net/iavf/iavf_rxtx.c                  |   4 +-
 drivers/net/iavf/iavf_rxtx.h                  |   2 +-
 drivers/net/ice/ice_rxtx.c                    |   4 +-
 drivers/net/ice/ice_rxtx.h                    |   2 +-
 drivers/net/igc/igc_txrx.c                    |   5 +-
 drivers/net/igc/igc_txrx.h                    |   3 +-
 drivers/net/ixgbe/ixgbe_ethdev.h              |   3 +-
 drivers/net/ixgbe/ixgbe_rxtx.c                |   4 +-
 drivers/net/mlx5/mlx5_rx.c                    |  26 +-
 drivers/net/mlx5/mlx5_rx.h                    |   2 +-
 drivers/net/netvsc/hn_rxtx.c                  |   4 +-
 drivers/net/netvsc/hn_var.h                   |   3 +-
 drivers/net/nfp/nfp_rxtx.c                    |   4 +-
 drivers/net/nfp/nfp_rxtx.h                    |   3 +-
 drivers/net/octeontx2/otx2_ethdev.h           |   2 +-
 drivers/net/octeontx2/otx2_ethdev_ops.c       |   8 +-
 drivers/net/sfc/sfc_ethdev.c                  |  12 +-
 drivers/net/thunderx/nicvf_ethdev.c           |   3 +-
 drivers/net/thunderx/nicvf_rxtx.c             |   4 +-
 drivers/net/thunderx/nicvf_rxtx.h             |   2 +-
 drivers/net/txgbe/txgbe_ethdev.h              |   3 +-
 drivers/net/txgbe/txgbe_rxtx.c                |   4 +-
 drivers/net/vhost/rte_eth_vhost.c             |   4 +-
 lib/ethdev/ethdev_driver.h                    | 154 +++++++++
 lib/ethdev/ethdev_private.c                   |  83 +++++
 lib/ethdev/ethdev_private.h                   |   7 +
 lib/ethdev/rte_ethdev.c                       |  90 ++++--
 lib/ethdev/rte_ethdev.h                       | 291 +++++++++++++-----
 lib/ethdev/rte_ethdev_core.h                  | 177 +++--------
 lib/ethdev/version.map                        |   6 +-
 lib/eventdev/rte_event_eth_rx_adapter.c       |   2 +-
 lib/eventdev/rte_event_eth_tx_adapter.c       |   2 +-
 lib/eventdev/rte_eventdev.c                   |   2 +-
 lib/metrics/rte_metrics_telemetry.c           |   2 +-
 57 files changed, 694 insertions(+), 373 deletions(-)

Comments

Ferruh Yigit Oct. 13, 2021, 8:16 p.m. UTC | #1
On 10/13/2021 2:36 PM, Konstantin Ananyev wrote:
> v6 changes:
> - Update comments (Andrew)
> - Move callback related variables under corresponding ifdefs (Andrew)
> - Few nits in rte_eth_macaddrs_get (Andrew)
> - Rebased on top of next-net tree
> 
> v5 changes:
> - Fix spelling (Thomas/David)
> - Rename internal helper functions (David)
> - Reorder patches and update commit messages (Thomas)
> - Update comments (Thomas)
> - Changed layout in rte_eth_fp_ops, to group functions and
>     related data based on their functionality:
>     first 64B line for Rx, second one for Tx.
>     Didn't observe any real performance difference comparing to
>     original layout. Though decided to keep a new one, as it seems
>     a bit more plausible.
> 
> v4 changes:
>   - Fix secondary process attach (Pavan)
>   - Fix build failure (Ferruh)
>   - Update lib/ethdev/verion.map (Ferruh)
>     Note that moving newly added symbols from EXPERIMENTAL to DPDK_22
>     section makes checkpatch.sh to complain.
> 
> v3 changes:
>   - Changes in public struct naming (Jerin/Haiyue)
>   - Split patches
>   - Update docs
>   - Shamelessly included Andrew's patch:
>     https://patches.dpdk.org/project/dpdk/patch/20210928154856.1015020-1-andrew.rybchenko@oktetlabs.ru/
>     into these series.
>     I have to do similar thing here, so decided to avoid duplicated effort.
> 
> The aim of these patch series is to make rte_ethdev core data structures
> (rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback, etc.) internal to
> DPDK and not visible to the user.
> That should allow future possible changes to core ethdev related structures
> to be transparent to the user and help to improve ABI/API stability.
> Note that current ethdev API is preserved, but it is a formal ABI break.
> 
> The work is based on previous discussions at:
> https://www.mail-archive.com/dev@dpdk.org/msg211405.html
> https://www.mail-archive.com/dev@dpdk.org/msg216685.html
> and consists of the following main points:
> 1. Copy public 'fast' function pointers (rx_pkt_burst(), etc.) and
>     related data pointer from rte_eth_dev into a separate flat array.
>     We keep it public to still be able to use inline functions for these
>     'fast' calls (like rte_eth_rx_burst(), etc.) to avoid/minimize slowdown.
>     Note that apart from function pointers itself, each element of this
>     flat array also contains two opaque pointers for each ethdev:
>     1) a pointer to an array of internal queue data pointers
>     2)  points to array of queue callback data pointers.
>     Note that exposing this extra information allows us to avoid extra
>     changes inside PMD level, plus should help to avoid possible
>     performance degradation.
> 2. Change implementation of 'fast' inline ethdev functions
>     (rte_eth_rx_burst(), etc.) to use new public flat array.
>     While it is an ABI breakage, this change is intended to be transparent
>     for both users (no changes in user app is required) and PMD developers
>     (no changes in PMD is required).
>     One extra note - with new implementation RX/TX callback invocation
>     will cost one extra function call with this changes. That might cause
>     some slowdown for code-path with RX/TX callbacks heavily involved.
>     Hope such trade-off is acceptable for the community.
> 3. Move rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback and related
>     things into internal header: <ethdev_driver.h>.
> 
> That approach was selected to:
>    - Avoid(/minimize) possible performance losses.
>    - Minimize required changes inside PMDs.
> 
> Performance testing results (ICX 2.0GHz, E810 (ice)):
>   - testpmd macswap fwd mode, plus
>     a) no RX/TX callbacks:
>        no actual slowdown observed
>     b) bpf-load rx 0 0 JM ./dpdk.org/examples/bpf/t3.o:
>        ~2% slowdown
>   - l3fwd: no actual slowdown observed
> 
> Would like to thank everyone who already reviewed and tested previous
> versions of these series. All other interested parties please don't be shy
> and provide your feedback.
> 
> Konstantin Ananyev (6):
>    ethdev: allocate max space for internal queue array
>    ethdev: change input parameters for rx_queue_count
>    ethdev: copy fast-path API into separate structure
>    ethdev: make fast-path functions to use new flat array
>    ethdev: add API to retrieve multiple ethernet addresses
>    ethdev: hide eth dev related structures
> 

For series,
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

No performance regression detected on my testing.

I am merging the series to next-net now which helps testing,
but before merging to main repo it will be good to get more
ack and test results (I can squash new tags later).

@Jerin, @Ajit, @Raslan, @Andrew, @Qi, @Honnappa,
Can you please test this set for any possible regression?

Series applied to dpdk-next-net/main, thanks.
Feifei Wang Oct. 14, 2021, 10:12 a.m. UTC | #2
> -----邮件原件-----
> 发件人: dev <dev-bounces@dpdk.org> 代表 Ferruh Yigit
> 发送时间: Thursday, October 14, 2021 4:16 AM
> 收件人: Konstantin Ananyev <konstantin.ananyev@intel.com>;
> dev@dpdk.org; jerinj@marvell.com; Ajit Khaparde
> (ajit.khaparde@broadcom.com) <ajit.khaparde@broadcom.com>; Raslan
> Darawsheh <rasland@nvidia.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Qi Zhang <qi.z.zhang@intel.com>;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> 抄送: 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
> (ajit.khaparde@broadcom.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;
> andrew.rybchenko@oktetlabs.ru; mczekaj@marvell.com;
> jiawenwu@trustnetic.com; jianwang@trustnetic.com;
> maxime.coquelin@redhat.com; chenbo.xia@intel.com;
> thomas@monjalon.net; mdr@ashroe.eu; jay.jayatheerthan@intel.com
> 主题: Re: [dpdk-dev] [PATCH v6 0/6] hide eth dev related structures
> 
> On 10/13/2021 2:36 PM, Konstantin Ananyev wrote:
> > v6 changes:
> > - Update comments (Andrew)
> > - Move callback related variables under corresponding ifdefs (Andrew)
> > - Few nits in rte_eth_macaddrs_get (Andrew)
> > - Rebased on top of next-net tree
> >
> > v5 changes:
> > - Fix spelling (Thomas/David)
> > - Rename internal helper functions (David)
> > - Reorder patches and update commit messages (Thomas)
> > - Update comments (Thomas)
> > - Changed layout in rte_eth_fp_ops, to group functions and
> >     related data based on their functionality:
> >     first 64B line for Rx, second one for Tx.
> >     Didn't observe any real performance difference comparing to
> >     original layout. Though decided to keep a new one, as it seems
> >     a bit more plausible.
> >
> > v4 changes:
> >   - Fix secondary process attach (Pavan)
> >   - Fix build failure (Ferruh)
> >   - Update lib/ethdev/verion.map (Ferruh)
> >     Note that moving newly added symbols from EXPERIMENTAL to DPDK_22
> >     section makes checkpatch.sh to complain.
> >
> > v3 changes:
> >   - Changes in public struct naming (Jerin/Haiyue)
> >   - Split patches
> >   - Update docs
> >   - Shamelessly included Andrew's patch:
> >
> https://patches.dpdk.org/project/dpdk/patch/20210928154856.1015020-1-
> andrew.rybchenko@oktetlabs.ru/
> >     into these series.
> >     I have to do similar thing here, so decided to avoid duplicated effort.
> >
> > The aim of these patch series is to make rte_ethdev core data
> > structures (rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback,
> > etc.) internal to DPDK and not visible to the user.
> > That should allow future possible changes to core ethdev related
> > structures to be transparent to the user and help to improve ABI/API
> stability.
> > Note that current ethdev API is preserved, but it is a formal ABI break.
> >
> > The work is based on previous discussions at:
> > https://www.mail-archive.com/dev@dpdk.org/msg211405.html
> > https://www.mail-archive.com/dev@dpdk.org/msg216685.html
> > and consists of the following main points:
> > 1. Copy public 'fast' function pointers (rx_pkt_burst(), etc.) and
> >     related data pointer from rte_eth_dev into a separate flat array.
> >     We keep it public to still be able to use inline functions for these
> >     'fast' calls (like rte_eth_rx_burst(), etc.) to avoid/minimize slowdown.
> >     Note that apart from function pointers itself, each element of this
> >     flat array also contains two opaque pointers for each ethdev:
> >     1) a pointer to an array of internal queue data pointers
> >     2)  points to array of queue callback data pointers.
> >     Note that exposing this extra information allows us to avoid extra
> >     changes inside PMD level, plus should help to avoid possible
> >     performance degradation.
> > 2. Change implementation of 'fast' inline ethdev functions
> >     (rte_eth_rx_burst(), etc.) to use new public flat array.
> >     While it is an ABI breakage, this change is intended to be transparent
> >     for both users (no changes in user app is required) and PMD developers
> >     (no changes in PMD is required).
> >     One extra note - with new implementation RX/TX callback invocation
> >     will cost one extra function call with this changes. That might cause
> >     some slowdown for code-path with RX/TX callbacks heavily involved.
> >     Hope such trade-off is acceptable for the community.
> > 3. Move rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback and related
> >     things into internal header: <ethdev_driver.h>.
> >
> > That approach was selected to:
> >    - Avoid(/minimize) possible performance losses.
> >    - Minimize required changes inside PMDs.
> >
> > Performance testing results (ICX 2.0GHz, E810 (ice)):
> >   - testpmd macswap fwd mode, plus
> >     a) no RX/TX callbacks:
> >        no actual slowdown observed
> >     b) bpf-load rx 0 0 JM ./dpdk.org/examples/bpf/t3.o:
> >        ~2% slowdown
> >   - l3fwd: no actual slowdown observed
> >
> > Would like to thank everyone who already reviewed and tested previous
> > versions of these series. All other interested parties please don't be
> > shy and provide your feedback.
> >
> > Konstantin Ananyev (6):
> >    ethdev: allocate max space for internal queue array
> >    ethdev: change input parameters for rx_queue_count
> >    ethdev: copy fast-path API into separate structure
> >    ethdev: make fast-path functions to use new flat array
> >    ethdev: add API to retrieve multiple ethernet addresses
> >    ethdev: hide eth dev related structures
> >
> 
> For series,
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> No performance regression detected on my testing.
> 
> I am merging the series to next-net now which helps testing, but before
> merging to main repo it will be good to get more ack and test results (I can
> squash new tags later).
> 
> @Jerin, @Ajit, @Raslan, @Andrew, @Qi, @Honnappa, Can you please test
> this set for any possible regression?
> 
> Series applied to dpdk-next-net/main, thanks.
> 

For series, there is no performance regression in n1sdp/thunderx2
with i40e and mlx5 40G NIC for l3fwd and testpmd.

Tested-by: Feifei Wang <feifei.wang2@arm.com>
Ali Alnubani Oct. 18, 2021, 4:04 p.m. UTC | #3
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
> Sent: Wednesday, October 13, 2021 11:16 PM
> To: Konstantin Ananyev <konstantin.ananyev@intel.com>; dev@dpdk.org;
> jerinj@marvell.com; Ajit Khaparde <ajit.khaparde@broadcom.com>; Raslan
> Darawsheh <rasland@nvidia.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Qi Zhang <qi.z.zhang@intel.com>;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> 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 Azrad <matan@nvidia.com>; Slava Ovsiienko
> <viacheslavo@nvidia.com>; sthemmin@microsoft.com; NBU-Contact-longli
> <longli@microsoft.com>; heinrich.kuhn@corigine.com;
> kirankumark@marvell.com; andrew.rybchenko@oktetlabs.ru;
> mczekaj@marvell.com; jiawenwu@trustnetic.com;
> jianwang@trustnetic.com; maxime.coquelin@redhat.com;
> chenbo.xia@intel.com; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; mdr@ashroe.eu; jay.jayatheerthan@intel.com
> Subject: Re: [dpdk-dev] [PATCH v6 0/6] hide eth dev related structures
> 
> On 10/13/2021 2:36 PM, Konstantin Ananyev wrote:
> > v6 changes:
> > - Update comments (Andrew)
> > - Move callback related variables under corresponding ifdefs (Andrew)
> > - Few nits in rte_eth_macaddrs_get (Andrew)
> > - Rebased on top of next-net tree
> >
> > v5 changes:
> > - Fix spelling (Thomas/David)
> > - Rename internal helper functions (David)
> > - Reorder patches and update commit messages (Thomas)
> > - Update comments (Thomas)
> > - Changed layout in rte_eth_fp_ops, to group functions and
> >     related data based on their functionality:
> >     first 64B line for Rx, second one for Tx.
> >     Didn't observe any real performance difference comparing to
> >     original layout. Though decided to keep a new one, as it seems
> >     a bit more plausible.
> >
> > v4 changes:
> >   - Fix secondary process attach (Pavan)
> >   - Fix build failure (Ferruh)
> >   - Update lib/ethdev/verion.map (Ferruh)
> >     Note that moving newly added symbols from EXPERIMENTAL to DPDK_22
> >     section makes checkpatch.sh to complain.
> >
> > v3 changes:
> >   - Changes in public struct naming (Jerin/Haiyue)
> >   - Split patches
> >   - Update docs
> >   - Shamelessly included Andrew's patch:
> >     https://patches.dpdk.org/project/dpdk/patch/20210928154856.1015020-
> 1-andrew.rybchenko@oktetlabs.ru/
> >     into these series.
> >     I have to do similar thing here, so decided to avoid duplicated effort.
> >
> > The aim of these patch series is to make rte_ethdev core data structures
> > (rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback, etc.) internal to
> > DPDK and not visible to the user.
> > That should allow future possible changes to core ethdev related structures
> > to be transparent to the user and help to improve ABI/API stability.
> > Note that current ethdev API is preserved, but it is a formal ABI break.
> >
> > The work is based on previous discussions at:
> > https://www.mail-archive.com/dev@dpdk.org/msg211405.html
> > https://www.mail-archive.com/dev@dpdk.org/msg216685.html
> > and consists of the following main points:
> > 1. Copy public 'fast' function pointers (rx_pkt_burst(), etc.) and
> >     related data pointer from rte_eth_dev into a separate flat array.
> >     We keep it public to still be able to use inline functions for these
> >     'fast' calls (like rte_eth_rx_burst(), etc.) to avoid/minimize slowdown.
> >     Note that apart from function pointers itself, each element of this
> >     flat array also contains two opaque pointers for each ethdev:
> >     1) a pointer to an array of internal queue data pointers
> >     2)  points to array of queue callback data pointers.
> >     Note that exposing this extra information allows us to avoid extra
> >     changes inside PMD level, plus should help to avoid possible
> >     performance degradation.
> > 2. Change implementation of 'fast' inline ethdev functions
> >     (rte_eth_rx_burst(), etc.) to use new public flat array.
> >     While it is an ABI breakage, this change is intended to be transparent
> >     for both users (no changes in user app is required) and PMD developers
> >     (no changes in PMD is required).
> >     One extra note - with new implementation RX/TX callback invocation
> >     will cost one extra function call with this changes. That might cause
> >     some slowdown for code-path with RX/TX callbacks heavily involved.
> >     Hope such trade-off is acceptable for the community.
> > 3. Move rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback and
> related
> >     things into internal header: <ethdev_driver.h>.
> >
> > That approach was selected to:
> >    - Avoid(/minimize) possible performance losses.
> >    - Minimize required changes inside PMDs.
> >
> > Performance testing results (ICX 2.0GHz, E810 (ice)):
> >   - testpmd macswap fwd mode, plus
> >     a) no RX/TX callbacks:
> >        no actual slowdown observed
> >     b) bpf-load rx 0 0 JM ./dpdk.org/examples/bpf/t3.o:
> >        ~2% slowdown
> >   - l3fwd: no actual slowdown observed
> >
> > Would like to thank everyone who already reviewed and tested previous
> > versions of these series. All other interested parties please don't be shy
> > and provide your feedback.
> >
> > Konstantin Ananyev (6):
> >    ethdev: allocate max space for internal queue array
> >    ethdev: change input parameters for rx_queue_count
> >    ethdev: copy fast-path API into separate structure
> >    ethdev: make fast-path functions to use new flat array
> >    ethdev: add API to retrieve multiple ethernet addresses
> >    ethdev: hide eth dev related structures
> >
> 
> For series,
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> No performance regression detected on my testing.
> 
> I am merging the series to next-net now which helps testing,
> but before merging to main repo it will be good to get more
> ack and test results (I can squash new tags later).
> 
> @Jerin, @Ajit, @Raslan, @Andrew, @Qi, @Honnappa,
> Can you please test this set for any possible regression?
> 
> Series applied to dpdk-next-net/main, thanks.
> 

Tested (on dpdk-next-net/main) single and multi-core packet forwarding performance with testpmd on both ConnectX-5 and ConnectX-6 Dx. I didn't see any noticeable regressions.

Thanks,
Ali
Ferruh Yigit Oct. 18, 2021, 4:47 p.m. UTC | #4
On 10/18/2021 5:04 PM, Ali Alnubani wrote:
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
>> Sent: Wednesday, October 13, 2021 11:16 PM
>> To: Konstantin Ananyev <konstantin.ananyev@intel.com>; dev@dpdk.org;
>> jerinj@marvell.com; Ajit Khaparde <ajit.khaparde@broadcom.com>; Raslan
>> Darawsheh <rasland@nvidia.com>; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; Qi Zhang <qi.z.zhang@intel.com>;
>> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
>> 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 Azrad <matan@nvidia.com>; Slava Ovsiienko
>> <viacheslavo@nvidia.com>; sthemmin@microsoft.com; NBU-Contact-longli
>> <longli@microsoft.com>; heinrich.kuhn@corigine.com;
>> kirankumark@marvell.com; andrew.rybchenko@oktetlabs.ru;
>> mczekaj@marvell.com; jiawenwu@trustnetic.com;
>> jianwang@trustnetic.com; maxime.coquelin@redhat.com;
>> chenbo.xia@intel.com; NBU-Contact-Thomas Monjalon
>> <thomas@monjalon.net>; mdr@ashroe.eu; jay.jayatheerthan@intel.com
>> Subject: Re: [dpdk-dev] [PATCH v6 0/6] hide eth dev related structures
>>
>> On 10/13/2021 2:36 PM, Konstantin Ananyev wrote:
>>> v6 changes:
>>> - Update comments (Andrew)
>>> - Move callback related variables under corresponding ifdefs (Andrew)
>>> - Few nits in rte_eth_macaddrs_get (Andrew)
>>> - Rebased on top of next-net tree
>>>
>>> v5 changes:
>>> - Fix spelling (Thomas/David)
>>> - Rename internal helper functions (David)
>>> - Reorder patches and update commit messages (Thomas)
>>> - Update comments (Thomas)
>>> - Changed layout in rte_eth_fp_ops, to group functions and
>>>      related data based on their functionality:
>>>      first 64B line for Rx, second one for Tx.
>>>      Didn't observe any real performance difference comparing to
>>>      original layout. Though decided to keep a new one, as it seems
>>>      a bit more plausible.
>>>
>>> v4 changes:
>>>    - Fix secondary process attach (Pavan)
>>>    - Fix build failure (Ferruh)
>>>    - Update lib/ethdev/verion.map (Ferruh)
>>>      Note that moving newly added symbols from EXPERIMENTAL to DPDK_22
>>>      section makes checkpatch.sh to complain.
>>>
>>> v3 changes:
>>>    - Changes in public struct naming (Jerin/Haiyue)
>>>    - Split patches
>>>    - Update docs
>>>    - Shamelessly included Andrew's patch:
>>>      https://patches.dpdk.org/project/dpdk/patch/20210928154856.1015020-
>> 1-andrew.rybchenko@oktetlabs.ru/
>>>      into these series.
>>>      I have to do similar thing here, so decided to avoid duplicated effort.
>>>
>>> The aim of these patch series is to make rte_ethdev core data structures
>>> (rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback, etc.) internal to
>>> DPDK and not visible to the user.
>>> That should allow future possible changes to core ethdev related structures
>>> to be transparent to the user and help to improve ABI/API stability.
>>> Note that current ethdev API is preserved, but it is a formal ABI break.
>>>
>>> The work is based on previous discussions at:
>>> https://www.mail-archive.com/dev@dpdk.org/msg211405.html
>>> https://www.mail-archive.com/dev@dpdk.org/msg216685.html
>>> and consists of the following main points:
>>> 1. Copy public 'fast' function pointers (rx_pkt_burst(), etc.) and
>>>      related data pointer from rte_eth_dev into a separate flat array.
>>>      We keep it public to still be able to use inline functions for these
>>>      'fast' calls (like rte_eth_rx_burst(), etc.) to avoid/minimize slowdown.
>>>      Note that apart from function pointers itself, each element of this
>>>      flat array also contains two opaque pointers for each ethdev:
>>>      1) a pointer to an array of internal queue data pointers
>>>      2)  points to array of queue callback data pointers.
>>>      Note that exposing this extra information allows us to avoid extra
>>>      changes inside PMD level, plus should help to avoid possible
>>>      performance degradation.
>>> 2. Change implementation of 'fast' inline ethdev functions
>>>      (rte_eth_rx_burst(), etc.) to use new public flat array.
>>>      While it is an ABI breakage, this change is intended to be transparent
>>>      for both users (no changes in user app is required) and PMD developers
>>>      (no changes in PMD is required).
>>>      One extra note - with new implementation RX/TX callback invocation
>>>      will cost one extra function call with this changes. That might cause
>>>      some slowdown for code-path with RX/TX callbacks heavily involved.
>>>      Hope such trade-off is acceptable for the community.
>>> 3. Move rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback and
>> related
>>>      things into internal header: <ethdev_driver.h>.
>>>
>>> That approach was selected to:
>>>     - Avoid(/minimize) possible performance losses.
>>>     - Minimize required changes inside PMDs.
>>>
>>> Performance testing results (ICX 2.0GHz, E810 (ice)):
>>>    - testpmd macswap fwd mode, plus
>>>      a) no RX/TX callbacks:
>>>         no actual slowdown observed
>>>      b) bpf-load rx 0 0 JM ./dpdk.org/examples/bpf/t3.o:
>>>         ~2% slowdown
>>>    - l3fwd: no actual slowdown observed
>>>
>>> Would like to thank everyone who already reviewed and tested previous
>>> versions of these series. All other interested parties please don't be shy
>>> and provide your feedback.
>>>
>>> Konstantin Ananyev (6):
>>>     ethdev: allocate max space for internal queue array
>>>     ethdev: change input parameters for rx_queue_count
>>>     ethdev: copy fast-path API into separate structure
>>>     ethdev: make fast-path functions to use new flat array
>>>     ethdev: add API to retrieve multiple ethernet addresses
>>>     ethdev: hide eth dev related structures
>>>
>>
>> For series,
>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>
>> No performance regression detected on my testing.
>>
>> I am merging the series to next-net now which helps testing,
>> but before merging to main repo it will be good to get more
>> ack and test results (I can squash new tags later).
>>
>> @Jerin, @Ajit, @Raslan, @Andrew, @Qi, @Honnappa,
>> Can you please test this set for any possible regression?
>>
>> Series applied to dpdk-next-net/main, thanks.
>>
> 
> Tested (on dpdk-next-net/main) single and multi-core packet forwarding performance with testpmd on both ConnectX-5 and ConnectX-6 Dx. I didn't see any noticeable regressions.
> 

Thanks!

At this stage I am putting set to pull request for main repo.
Last day for anyone who wants to test the set.
Ajit Khaparde Oct. 18, 2021, 11:47 p.m. UTC | #5
On Mon, Oct 18, 2021 at 9:47 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 10/18/2021 5:04 PM, Ali Alnubani wrote:
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Ferruh Yigit
> >> Sent: Wednesday, October 13, 2021 11:16 PM
> >> To: Konstantin Ananyev <konstantin.ananyev@intel.com>; dev@dpdk.org;
> >> jerinj@marvell.com; Ajit Khaparde <ajit.khaparde@broadcom.com>; Raslan
> >> Darawsheh <rasland@nvidia.com>; Andrew Rybchenko
> >> <andrew.rybchenko@oktetlabs.ru>; Qi Zhang <qi.z.zhang@intel.com>;
> >> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> >> 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 Azrad <matan@nvidia.com>; Slava Ovsiienko
> >> <viacheslavo@nvidia.com>; sthemmin@microsoft.com; NBU-Contact-longli
> >> <longli@microsoft.com>; heinrich.kuhn@corigine.com;
> >> kirankumark@marvell.com; andrew.rybchenko@oktetlabs.ru;
> >> mczekaj@marvell.com; jiawenwu@trustnetic.com;
> >> jianwang@trustnetic.com; maxime.coquelin@redhat.com;
> >> chenbo.xia@intel.com; NBU-Contact-Thomas Monjalon
> >> <thomas@monjalon.net>; mdr@ashroe.eu; jay.jayatheerthan@intel.com
> >> Subject: Re: [dpdk-dev] [PATCH v6 0/6] hide eth dev related structures
> >>
> >> On 10/13/2021 2:36 PM, Konstantin Ananyev wrote:
> >>> v6 changes:
> >>> - Update comments (Andrew)
> >>> - Move callback related variables under corresponding ifdefs (Andrew)
> >>> - Few nits in rte_eth_macaddrs_get (Andrew)
> >>> - Rebased on top of next-net tree
> >>>
> >>> v5 changes:
> >>> - Fix spelling (Thomas/David)
> >>> - Rename internal helper functions (David)
> >>> - Reorder patches and update commit messages (Thomas)
> >>> - Update comments (Thomas)
> >>> - Changed layout in rte_eth_fp_ops, to group functions and
> >>>      related data based on their functionality:
> >>>      first 64B line for Rx, second one for Tx.
> >>>      Didn't observe any real performance difference comparing to
> >>>      original layout. Though decided to keep a new one, as it seems
> >>>      a bit more plausible.
> >>>
> >>> v4 changes:
> >>>    - Fix secondary process attach (Pavan)
> >>>    - Fix build failure (Ferruh)
> >>>    - Update lib/ethdev/verion.map (Ferruh)
> >>>      Note that moving newly added symbols from EXPERIMENTAL to DPDK_22
> >>>      section makes checkpatch.sh to complain.
> >>>
> >>> v3 changes:
> >>>    - Changes in public struct naming (Jerin/Haiyue)
> >>>    - Split patches
> >>>    - Update docs
> >>>    - Shamelessly included Andrew's patch:
> >>>      https://patches.dpdk.org/project/dpdk/patch/20210928154856.1015020-
> >> 1-andrew.rybchenko@oktetlabs.ru/
> >>>      into these series.
> >>>      I have to do similar thing here, so decided to avoid duplicated effort.
> >>>
> >>> The aim of these patch series is to make rte_ethdev core data structures
> >>> (rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback, etc.) internal to
> >>> DPDK and not visible to the user.
> >>> That should allow future possible changes to core ethdev related structures
> >>> to be transparent to the user and help to improve ABI/API stability.
> >>> Note that current ethdev API is preserved, but it is a formal ABI break.
> >>>
> >>> The work is based on previous discussions at:
> >>> https://www.mail-archive.com/dev@dpdk.org/msg211405.html
> >>> https://www.mail-archive.com/dev@dpdk.org/msg216685.html
> >>> and consists of the following main points:
> >>> 1. Copy public 'fast' function pointers (rx_pkt_burst(), etc.) and
> >>>      related data pointer from rte_eth_dev into a separate flat array.
> >>>      We keep it public to still be able to use inline functions for these
> >>>      'fast' calls (like rte_eth_rx_burst(), etc.) to avoid/minimize slowdown.
> >>>      Note that apart from function pointers itself, each element of this
> >>>      flat array also contains two opaque pointers for each ethdev:
> >>>      1) a pointer to an array of internal queue data pointers
> >>>      2)  points to array of queue callback data pointers.
> >>>      Note that exposing this extra information allows us to avoid extra
> >>>      changes inside PMD level, plus should help to avoid possible
> >>>      performance degradation.
> >>> 2. Change implementation of 'fast' inline ethdev functions
> >>>      (rte_eth_rx_burst(), etc.) to use new public flat array.
> >>>      While it is an ABI breakage, this change is intended to be transparent
> >>>      for both users (no changes in user app is required) and PMD developers
> >>>      (no changes in PMD is required).
> >>>      One extra note - with new implementation RX/TX callback invocation
> >>>      will cost one extra function call with this changes. That might cause
> >>>      some slowdown for code-path with RX/TX callbacks heavily involved.
> >>>      Hope such trade-off is acceptable for the community.
> >>> 3. Move rte_eth_dev, rte_eth_dev_data, rte_eth_rxtx_callback and
> >> related
> >>>      things into internal header: <ethdev_driver.h>.
> >>>
> >>> That approach was selected to:
> >>>     - Avoid(/minimize) possible performance losses.
> >>>     - Minimize required changes inside PMDs.
> >>>
> >>> Performance testing results (ICX 2.0GHz, E810 (ice)):
> >>>    - testpmd macswap fwd mode, plus
> >>>      a) no RX/TX callbacks:
> >>>         no actual slowdown observed
> >>>      b) bpf-load rx 0 0 JM ./dpdk.org/examples/bpf/t3.o:
> >>>         ~2% slowdown
> >>>    - l3fwd: no actual slowdown observed
> >>>
> >>> Would like to thank everyone who already reviewed and tested previous
> >>> versions of these series. All other interested parties please don't be shy
> >>> and provide your feedback.
> >>>
> >>> Konstantin Ananyev (6):
> >>>     ethdev: allocate max space for internal queue array
> >>>     ethdev: change input parameters for rx_queue_count
> >>>     ethdev: copy fast-path API into separate structure
> >>>     ethdev: make fast-path functions to use new flat array
> >>>     ethdev: add API to retrieve multiple ethernet addresses
> >>>     ethdev: hide eth dev related structures
> >>>
> >>
> >> For series,
> >> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>
> >> No performance regression detected on my testing.
> >>
> >> I am merging the series to next-net now which helps testing,
> >> but before merging to main repo it will be good to get more
> >> ack and test results (I can squash new tags later).
> >>
> >> @Jerin, @Ajit, @Raslan, @Andrew, @Qi, @Honnappa,
> >> Can you please test this set for any possible regression?
> >>
> >> Series applied to dpdk-next-net/main, thanks.
> >>
> >
> > Tested (on dpdk-next-net/main) single and multi-core packet forwarding performance with testpmd on both ConnectX-5 and ConnectX-6 Dx. I didn't see any noticeable regressions.
> >
>
> Thanks!
>
> At this stage I am putting set to pull request for main repo.
> Last day for anyone who wants to test the set.
I tested dpdk-next-net/main. My testing was limited and did not reveal
any issues.