mbox series

[v5,00/16] remove mbuf timestamp

Message ID 20201103140931.488700-1-thomas@monjalon.net (mailing list archive)
Headers
Series remove mbuf timestamp |

Message

Thomas Monjalon Nov. 3, 2020, 2:09 p.m. UTC 
  The mbuf field timestamp was announced to be removed for three reasons:
  - a dynamic field already exist, used for Tx only
  - this field always used 8 bytes even if unneeded
  - this field is in the first half (cacheline) of mbuf

After this series, the dynamic field timestamp is used for both Rx and Tx
with separate dynamic flags to distinguish when the value is meaningful
without resetting the field during forwarding.

As a consequence, 8 bytes can be re-allocated to dynamic fields
in the first half of mbuf structure.
It is still open to change more the mbuf layout.

This mbuf layout change is important to allow adding more features
(consuming more dynamic fields) during the next year,
and can allow performance improvements with new usages in the first half.


v5:
- add a blank line between different kind of ARK variables
- move registration after octeontx2 VF config
- register also in otx2_nix_timesync_enable

v4:
- use local variable in nfb
- fix flag initialization
- remove useless blank line

v3:
- move ark variables declaration in a .h file 
- improve cache locality for octeontx2
- add comments about cache locality in commit logs
- add comment for unused flag offset 17
- add timestamp register functions
- replace lookup with register in drivers and apps
- remove register in ethdev

v2:
- remove optimization to register only once in ethdev
- fix error message in latencystats
- convert rxtx_callbacks macro to inline function
- increase dynamic fields space
- do not move pool field


Thomas Monjalon (16):
  eventdev: remove software Rx timestamp
  mbuf: add Rx timestamp flag and helpers
  latency: switch Rx timestamp to dynamic mbuf field
  net/ark: switch Rx timestamp to dynamic mbuf field
  net/dpaa2: switch Rx timestamp to dynamic mbuf field
  net/mlx5: fix dynamic mbuf offset lookup check
  net/mlx5: switch Rx timestamp to dynamic mbuf field
  net/nfb: switch Rx timestamp to dynamic mbuf field
  net/octeontx2: switch Rx timestamp to dynamic mbuf field
  net/pcap: switch Rx timestamp to dynamic mbuf field
  app/testpmd: switch Rx timestamp to dynamic mbuf field
  examples/rxtx_callbacks: switch timestamp to dynamic field
  ethdev: add doxygen comment for Rx timestamp API
  mbuf: remove deprecated timestamp field
  mbuf: add Tx timestamp registration helper
  ethdev: include mbuf registration in Tx timestamp API

 app/test-pmd/config.c                         | 38 -------------
 app/test-pmd/util.c                           | 38 ++++++++++++-
 app/test/test_mbuf.c                          |  1 -
 doc/guides/nics/mlx5.rst                      |  5 +-
 .../prog_guide/event_ethernet_rx_adapter.rst  |  6 +-
 doc/guides/rel_notes/deprecation.rst          |  4 --
 doc/guides/rel_notes/release_20_11.rst        |  4 ++
 drivers/net/ark/ark_ethdev.c                  | 17 ++++++
 drivers/net/ark/ark_ethdev_rx.c               |  7 ++-
 drivers/net/ark/ark_ethdev_rx.h               |  2 +
 drivers/net/dpaa2/dpaa2_ethdev.c              | 11 ++++
 drivers/net/dpaa2/dpaa2_ethdev.h              |  2 +
 drivers/net/dpaa2/dpaa2_rxtx.c                | 25 ++++++---
 drivers/net/mlx5/mlx5_ethdev.c                |  8 ++-
 drivers/net/mlx5/mlx5_rxq.c                   |  8 +++
 drivers/net/mlx5/mlx5_rxtx.c                  |  8 +--
 drivers/net/mlx5/mlx5_rxtx.h                  | 19 +++++++
 drivers/net/mlx5/mlx5_rxtx_vec_altivec.h      | 41 +++++++-------
 drivers/net/mlx5/mlx5_rxtx_vec_neon.h         | 43 ++++++++-------
 drivers/net/mlx5/mlx5_rxtx_vec_sse.h          | 35 ++++++------
 drivers/net/mlx5/mlx5_trigger.c               |  2 +-
 drivers/net/mlx5/mlx5_txq.c                   |  2 +-
 drivers/net/nfb/nfb_rx.c                      | 15 ++++-
 drivers/net/nfb/nfb_rx.h                      | 21 +++++--
 drivers/net/octeontx2/otx2_ethdev.c           | 10 ++++
 drivers/net/octeontx2/otx2_ptp.c              |  8 +++
 drivers/net/octeontx2/otx2_rx.h               | 19 ++++++-
 drivers/net/pcap/rte_eth_pcap.c               | 20 ++++++-
 examples/rxtx_callbacks/main.c                | 16 +++++-
 lib/librte_ethdev/rte_ethdev.h                | 14 ++++-
 .../rte_event_eth_rx_adapter.c                | 11 ----
 .../rte_event_eth_rx_adapter.h                |  6 +-
 lib/librte_latencystats/rte_latencystats.c    | 30 ++++++++--
 lib/librte_mbuf/rte_mbuf.c                    |  2 -
 lib/librte_mbuf/rte_mbuf.h                    |  2 +-
 lib/librte_mbuf/rte_mbuf_core.h               | 12 +---
 lib/librte_mbuf/rte_mbuf_dyn.c                | 51 +++++++++++++++++
 lib/librte_mbuf/rte_mbuf_dyn.h                | 55 +++++++++++++++++--
 lib/librte_mbuf/version.map                   |  2 +
 39 files changed, 440 insertions(+), 180 deletions(-)

Comments

Olivier Matz Nov. 3, 2020, 2:17 p.m. UTC | #1 
On Tue, Nov 03, 2020 at 03:09:15PM +0100, Thomas Monjalon wrote:
> The mbuf field timestamp was announced to be removed for three reasons:
>   - a dynamic field already exist, used for Tx only
>   - this field always used 8 bytes even if unneeded
>   - this field is in the first half (cacheline) of mbuf
> 
> After this series, the dynamic field timestamp is used for both Rx and Tx
> with separate dynamic flags to distinguish when the value is meaningful
> without resetting the field during forwarding.
> 
> As a consequence, 8 bytes can be re-allocated to dynamic fields
> in the first half of mbuf structure.
> It is still open to change more the mbuf layout.
> 
> This mbuf layout change is important to allow adding more features
> (consuming more dynamic fields) during the next year,
> and can allow performance improvements with new usages in the first half.
> 
> 
> v5:
> - add a blank line between different kind of ARK variables
> - move registration after octeontx2 VF config
> - register also in otx2_nix_timesync_enable
> 
> v4:
> - use local variable in nfb
> - fix flag initialization
> - remove useless blank line
> 
> v3:
> - move ark variables declaration in a .h file 
> - improve cache locality for octeontx2
> - add comments about cache locality in commit logs
> - add comment for unused flag offset 17
> - add timestamp register functions
> - replace lookup with register in drivers and apps
> - remove register in ethdev
> 
> v2:
> - remove optimization to register only once in ethdev
> - fix error message in latencystats
> - convert rxtx_callbacks macro to inline function
> - increase dynamic fields space
> - do not move pool field
> 
> 
> Thomas Monjalon (16):
>   eventdev: remove software Rx timestamp
>   mbuf: add Rx timestamp flag and helpers
>   latency: switch Rx timestamp to dynamic mbuf field
>   net/ark: switch Rx timestamp to dynamic mbuf field
>   net/dpaa2: switch Rx timestamp to dynamic mbuf field
>   net/mlx5: fix dynamic mbuf offset lookup check
>   net/mlx5: switch Rx timestamp to dynamic mbuf field
>   net/nfb: switch Rx timestamp to dynamic mbuf field
>   net/octeontx2: switch Rx timestamp to dynamic mbuf field
>   net/pcap: switch Rx timestamp to dynamic mbuf field
>   app/testpmd: switch Rx timestamp to dynamic mbuf field
>   examples/rxtx_callbacks: switch timestamp to dynamic field
>   ethdev: add doxygen comment for Rx timestamp API
>   mbuf: remove deprecated timestamp field
>   mbuf: add Tx timestamp registration helper
>   ethdev: include mbuf registration in Tx timestamp API
> 
>  app/test-pmd/config.c                         | 38 -------------
>  app/test-pmd/util.c                           | 38 ++++++++++++-
>  app/test/test_mbuf.c                          |  1 -
>  doc/guides/nics/mlx5.rst                      |  5 +-
>  .../prog_guide/event_ethernet_rx_adapter.rst  |  6 +-
>  doc/guides/rel_notes/deprecation.rst          |  4 --
>  doc/guides/rel_notes/release_20_11.rst        |  4 ++
>  drivers/net/ark/ark_ethdev.c                  | 17 ++++++
>  drivers/net/ark/ark_ethdev_rx.c               |  7 ++-
>  drivers/net/ark/ark_ethdev_rx.h               |  2 +
>  drivers/net/dpaa2/dpaa2_ethdev.c              | 11 ++++
>  drivers/net/dpaa2/dpaa2_ethdev.h              |  2 +
>  drivers/net/dpaa2/dpaa2_rxtx.c                | 25 ++++++---
>  drivers/net/mlx5/mlx5_ethdev.c                |  8 ++-
>  drivers/net/mlx5/mlx5_rxq.c                   |  8 +++
>  drivers/net/mlx5/mlx5_rxtx.c                  |  8 +--
>  drivers/net/mlx5/mlx5_rxtx.h                  | 19 +++++++
>  drivers/net/mlx5/mlx5_rxtx_vec_altivec.h      | 41 +++++++-------
>  drivers/net/mlx5/mlx5_rxtx_vec_neon.h         | 43 ++++++++-------
>  drivers/net/mlx5/mlx5_rxtx_vec_sse.h          | 35 ++++++------
>  drivers/net/mlx5/mlx5_trigger.c               |  2 +-
>  drivers/net/mlx5/mlx5_txq.c                   |  2 +-
>  drivers/net/nfb/nfb_rx.c                      | 15 ++++-
>  drivers/net/nfb/nfb_rx.h                      | 21 +++++--
>  drivers/net/octeontx2/otx2_ethdev.c           | 10 ++++
>  drivers/net/octeontx2/otx2_ptp.c              |  8 +++
>  drivers/net/octeontx2/otx2_rx.h               | 19 ++++++-
>  drivers/net/pcap/rte_eth_pcap.c               | 20 ++++++-
>  examples/rxtx_callbacks/main.c                | 16 +++++-
>  lib/librte_ethdev/rte_ethdev.h                | 14 ++++-
>  .../rte_event_eth_rx_adapter.c                | 11 ----
>  .../rte_event_eth_rx_adapter.h                |  6 +-
>  lib/librte_latencystats/rte_latencystats.c    | 30 ++++++++--
>  lib/librte_mbuf/rte_mbuf.c                    |  2 -
>  lib/librte_mbuf/rte_mbuf.h                    |  2 +-
>  lib/librte_mbuf/rte_mbuf_core.h               | 12 +---
>  lib/librte_mbuf/rte_mbuf_dyn.c                | 51 +++++++++++++++++
>  lib/librte_mbuf/rte_mbuf_dyn.h                | 55 +++++++++++++++++--
>  lib/librte_mbuf/version.map                   |  2 +
>  39 files changed, 440 insertions(+), 180 deletions(-)
> 
> -- 
> 2.28.0
> 

For the series:
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Thomas Monjalon Nov. 3, 2020, 2:44 p.m. UTC | #2 
03/11/2020 15:17, Olivier Matz:
> On Tue, Nov 03, 2020 at 03:09:15PM +0100, Thomas Monjalon wrote:
> > The mbuf field timestamp was announced to be removed for three reasons:
> >   - a dynamic field already exist, used for Tx only
> >   - this field always used 8 bytes even if unneeded
> >   - this field is in the first half (cacheline) of mbuf
> > 
> > After this series, the dynamic field timestamp is used for both Rx and Tx
> > with separate dynamic flags to distinguish when the value is meaningful
> > without resetting the field during forwarding.
> > 
> > As a consequence, 8 bytes can be re-allocated to dynamic fields
> > in the first half of mbuf structure.
> > It is still open to change more the mbuf layout.
> > 
> > This mbuf layout change is important to allow adding more features
> > (consuming more dynamic fields) during the next year,
> > and can allow performance improvements with new usages in the first half.
[...]
> >  39 files changed, 440 insertions(+), 180 deletions(-)
> 
> For the series:
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks for the help Olivier, David and Andrew.

Next step: decide whether we keep "free space" in the first half
for dynamic fields or move another field from the second half.
Stephen Hemminger Nov. 3, 2020, 4:08 p.m. UTC | #3 
On Tue,  3 Nov 2020 15:09:15 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> The mbuf field timestamp was announced to be removed for three reasons:
>   - a dynamic field already exist, used for Tx only
>   - this field always used 8 bytes even if unneeded
>   - this field is in the first half (cacheline) of mbuf
> 
> After this series, the dynamic field timestamp is used for both Rx and Tx
> with separate dynamic flags to distinguish when the value is meaningful
> without resetting the field during forwarding.

There should be a place in documentation which describes all the
dynamic fields and their meaning.  For example, which drivers/features
set the field and the exact meaning.  Is the timestamp in HW units,
UTC units, or TSC ticks?
Thomas Monjalon Nov. 3, 2020, 4:20 p.m. UTC | #4 
03/11/2020 17:08, Stephen Hemminger:
> On Tue,  3 Nov 2020 15:09:15 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > The mbuf field timestamp was announced to be removed for three reasons:
> >   - a dynamic field already exist, used for Tx only
> >   - this field always used 8 bytes even if unneeded
> >   - this field is in the first half (cacheline) of mbuf
> > 
> > After this series, the dynamic field timestamp is used for both Rx and Tx
> > with separate dynamic flags to distinguish when the value is meaningful
> > without resetting the field during forwarding.
> 
> There should be a place in documentation which describes all the
> dynamic fields and their meaning.  For example, which drivers/features
> set the field and the exact meaning.

A dynamic field can be registered by anyone, including the apps.
So you will never get a full list.
The meaning of each field should be defined in its context
(driver, lib or app).

> Is the timestamp in HW units, UTC units, or TSC ticks?

The timestamp unit is driver-specific.
It is explained in ethdev API:
http://doc.dpdk.org/api/rte__ethdev_8h.html#a4346bf07a0d302c9ba4fe06baffd3196
Stephen Hemminger Nov. 3, 2020, 5:42 p.m. UTC | #5 
On Tue, 03 Nov 2020 17:20:20 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:

> 03/11/2020 17:08, Stephen Hemminger:
> > On Tue,  3 Nov 2020 15:09:15 +0100
> > Thomas Monjalon <thomas@monjalon.net> wrote:
> >   
> > > The mbuf field timestamp was announced to be removed for three reasons:
> > >   - a dynamic field already exist, used for Tx only
> > >   - this field always used 8 bytes even if unneeded
> > >   - this field is in the first half (cacheline) of mbuf
> > > 
> > > After this series, the dynamic field timestamp is used for both Rx and Tx
> > > with separate dynamic flags to distinguish when the value is meaningful
> > > without resetting the field during forwarding.  
> > 
> > There should be a place in documentation which describes all the
> > dynamic fields and their meaning.  For example, which drivers/features
> > set the field and the exact meaning.  
> 
> A dynamic field can be registered by anyone, including the apps.
> So you will never get a full list.
> The meaning of each field should be defined in its context
> (driver, lib or app).
> 
> > Is the timestamp in HW units, UTC units, or TSC ticks?  
> 
> The timestamp unit is driver-specific.
> It is explained in ethdev API:
> http://doc.dpdk.org/api/rte__ethdev_8h.html#a4346bf07a0d302c9ba4fe06baffd3196


Are there are any conventions we should use in this area?
There could be overlapping usage between subsystems?
Thomas Monjalon Nov. 3, 2020, 5:55 p.m. UTC | #6 
03/11/2020 18:42, Stephen Hemminger:
> On Tue, 03 Nov 2020 17:20:20 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
> > 03/11/2020 17:08, Stephen Hemminger:
> > > On Tue,  3 Nov 2020 15:09:15 +0100
> > > Thomas Monjalon <thomas@monjalon.net> wrote:
> > >   
> > > > The mbuf field timestamp was announced to be removed for three reasons:
> > > >   - a dynamic field already exist, used for Tx only
> > > >   - this field always used 8 bytes even if unneeded
> > > >   - this field is in the first half (cacheline) of mbuf
> > > > 
> > > > After this series, the dynamic field timestamp is used for both Rx and Tx
> > > > with separate dynamic flags to distinguish when the value is meaningful
> > > > without resetting the field during forwarding.  
> > > 
> > > There should be a place in documentation which describes all the
> > > dynamic fields and their meaning.  For example, which drivers/features
> > > set the field and the exact meaning.  
> > 
> > A dynamic field can be registered by anyone, including the apps.
> > So you will never get a full list.
> > The meaning of each field should be defined in its context
> > (driver, lib or app).
> > 
> > > Is the timestamp in HW units, UTC units, or TSC ticks?  
> > 
> > The timestamp unit is driver-specific.
> > It is explained in ethdev API:
> > http://doc.dpdk.org/api/rte__ethdev_8h.html#a4346bf07a0d302c9ba4fe06baffd3196
> 
> 
> Are there are any conventions we should use in this area?
> There could be overlapping usage between subsystems?

The name of the field should be prefixed with the right context
to avoid overlapping of different usages.
It is documented here:
	http://doc.dpdk.org/api/rte__mbuf__dyn_8h.html