[dpdk-dev] [PATCH] doc: announce ABI change on ethdev

[dpdk-dev] [PATCH] doc: announce ABI change on ethdev

[Shahaf Shuler]

This is an ABI change notice for DPDK 17.08 in librte_ether
about changes in rte_eth_txmode structure.

Currently Tx offloads are enabled by default, and can be disabled
using ETH_TXQ_FLAGS_NO* flags. This behaviour is not consistent with
the Rx side where the Rx offloads are disabled by default and enabled
according to bit field in rte_eth_rxmode structure.

The proposal is to disable the Tx offloads by default, and provide
a way for the application to enable them in rte_eth_txmode structure.
Besides of making the Tx configuration API more consistent for
applications, PMDs will be able to provide a better out of the
box performance.
Finally, as part of the work, the ETH_TXQ_FLAGS_NO* will
be superseded as well.

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
looks like this patch has arrived to everyone     
besides dev@dpdk.org resending it again. sorry for
the noise.                                        
---
 doc/guides/rel_notes/deprecation.rst | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index a3e7c720c..0920b4766 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -81,3 +81,11 @@ Deprecation Notices
 
   - ``rte_crpytodev_scheduler_mode_get``, replaced by ``rte_cryptodev_scheduler_mode_get``
   - ``rte_crpytodev_scheduler_mode_set``, replaced by ``rte_cryptodev_scheduler_mode_set``
+
+* ethdev: in 17.08 ABI changes are planned:
+  Tx offloads will no longer be enabled by default.
+  Instead, the ``rte_eth_txmode`` structure will be extended with bit field to enable
+  each Tx offload.
+  Besides of making the Rx/Tx configuration API more consistent for the
+  application, PMDs will be able to provide a better out of the box performance.
+  as part of the work, ``ETH_TXQ_FLAGS_NO*`` will be superseded as well.
-- 
2.12.0

Re: [dpdk-dev] [PATCH] doc: announce ABI change on ethdev

[Shahaf Shuler]

Monday, May 1, 2017 9:58 AM, Shahaf Shuler:
> 
> This is an ABI change notice for DPDK 17.08 in librte_ether about changes in
> rte_eth_txmode structure.
> 
> Currently Tx offloads are enabled by default, and can be disabled using
> ETH_TXQ_FLAGS_NO* flags. This behaviour is not consistent with the Rx side
> where the Rx offloads are disabled by default and enabled according to bit
> field in rte_eth_rxmode structure.
> 
> The proposal is to disable the Tx offloads by default, and provide a way for
> the application to enable them in rte_eth_txmode structure.
> Besides of making the Tx configuration API more consistent for applications,
> PMDs will be able to provide a better out of the box performance.
> Finally, as part of the work, the ETH_TXQ_FLAGS_NO* will be superseded as
> well.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>

Hi, 
Any comments on this announcement? 

Please have a min to read and comment. 
This work can improve all PMDs data path code, which will be according to application needs.

> ---
> looks like this patch has arrived to everyone
> besides dev@dpdk.org resending it again. sorry for
> the noise.
> ---
>  doc/guides/rel_notes/deprecation.rst | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index a3e7c720c..0920b4766 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -81,3 +81,11 @@ Deprecation Notices
> 
>    - ``rte_crpytodev_scheduler_mode_get``, replaced by
> ``rte_cryptodev_scheduler_mode_get``
>    - ``rte_crpytodev_scheduler_mode_set``, replaced by
> ``rte_cryptodev_scheduler_mode_set``
> +
> +* ethdev: in 17.08 ABI changes are planned:
> +  Tx offloads will no longer be enabled by default.
> +  Instead, the ``rte_eth_txmode`` structure will be extended with bit
> +field to enable
> +  each Tx offload.
> +  Besides of making the Rx/Tx configuration API more consistent for the
> +  application, PMDs will be able to provide a better out of the box
> performance.
> +  as part of the work, ``ETH_TXQ_FLAGS_NO*`` will be superseded as well.
> --
> 2.12.0

Re: [dpdk-dev] [PATCH] doc: announce ABI change on ethdev

[Adrien Mazarguil]

On Mon, May 01, 2017 at 09:58:12AM +0300, Shahaf Shuler wrote:
> This is an ABI change notice for DPDK 17.08 in librte_ether
> about changes in rte_eth_txmode structure.
> 
> Currently Tx offloads are enabled by default, and can be disabled
> using ETH_TXQ_FLAGS_NO* flags. This behaviour is not consistent with
> the Rx side where the Rx offloads are disabled by default and enabled
> according to bit field in rte_eth_rxmode structure.
> 
> The proposal is to disable the Tx offloads by default, and provide
> a way for the application to enable them in rte_eth_txmode structure.
> Besides of making the Tx configuration API more consistent for
> applications, PMDs will be able to provide a better out of the
> box performance.
> Finally, as part of the work, the ETH_TXQ_FLAGS_NO* will
> be superseded as well.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>

Basically, TX mbuf flags like TSO and checksum offloads won't have to be
honored by PMDs unless applications request them first while configuring the
device, just like RX offloads.

Considering more and more TX offloads will be added over time, I do not
think expecting them all to be enabled by default is sane. There will always
be an associated software cost in PMDs, and this solution allows
applications to selectively enable them as needed for maximum performance.

Konstantin/Olivier/Tomasz, I do not want to resume the thread about
tx_prepare(), however this could provide an alternative means to benefit
from improved performance when applications do not need TSO (or any other
offload for that matter), while adding consistency to device configuration.

What's your opinion?

In any case I'm fine with this change:

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

> ---
>  doc/guides/rel_notes/deprecation.rst | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index a3e7c720c..0920b4766 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -81,3 +81,11 @@ Deprecation Notices
>  
>    - ``rte_crpytodev_scheduler_mode_get``, replaced by ``rte_cryptodev_scheduler_mode_get``
>    - ``rte_crpytodev_scheduler_mode_set``, replaced by ``rte_cryptodev_scheduler_mode_set``
> +
> +* ethdev: in 17.08 ABI changes are planned:
> +  Tx offloads will no longer be enabled by default.
> +  Instead, the ``rte_eth_txmode`` structure will be extended with bit field to enable
> +  each Tx offload.
> +  Besides of making the Rx/Tx configuration API more consistent for the
> +  application, PMDs will be able to provide a better out of the box performance.
> +  as part of the work, ``ETH_TXQ_FLAGS_NO*`` will be superseded as well.
> -- 
> 2.12.0
> 

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH] doc: announce ABI change on ethdev

[Ferruh Yigit]

On 5/1/2017 7:58 AM, Shahaf Shuler wrote:
> This is an ABI change notice for DPDK 17.08 in librte_ether
> about changes in rte_eth_txmode structure.
> 
> Currently Tx offloads are enabled by default, and can be disabled
> using ETH_TXQ_FLAGS_NO* flags. This behaviour is not consistent with
> the Rx side where the Rx offloads are disabled by default and enabled
> according to bit field in rte_eth_rxmode structure.
> 
> The proposal is to disable the Tx offloads by default, and provide
> a way for the application to enable them in rte_eth_txmode structure.
> Besides of making the Tx configuration API more consistent for
> applications, PMDs will be able to provide a better out of the
> box performance.
> Finally, as part of the work, the ETH_TXQ_FLAGS_NO* will
> be superseded as well.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
> looks like this patch has arrived to everyone     
> besides dev@dpdk.org resending it again. sorry for
> the noise.                                        
> ---
>  doc/guides/rel_notes/deprecation.rst | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index a3e7c720c..0920b4766 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -81,3 +81,11 @@ Deprecation Notices
>  
>    - ``rte_crpytodev_scheduler_mode_get``, replaced by ``rte_cryptodev_scheduler_mode_get``
>    - ``rte_crpytodev_scheduler_mode_set``, replaced by ``rte_cryptodev_scheduler_mode_set``
> +
> +* ethdev: in 17.08 ABI changes are planned:
> +  Tx offloads will no longer be enabled by default.
> +  Instead, the ``rte_eth_txmode`` structure will be extended with bit field to enable
> +  each Tx offload.
> +  Besides of making the Rx/Tx configuration API more consistent for the
> +  application, PMDs will be able to provide a better out of the box performance.

I understand the consistency part, but why PMD performs better when Tx
offload disabled?

> +  as part of the work, ``ETH_TXQ_FLAGS_NO*`` will be superseded as well.
> 

Re: [dpdk-dev] [PATCH] doc: announce ABI change on ethdev

[Shahaf Shuler]

Tuesday, May 9, 2017 4:49 PM, Ferruh Yigit:
> On 5/1/2017 7:58 AM, Shahaf Shuler wrote:
> 
> I understand the consistency part, but why PMD performs better when Tx
> offload disabled?
> 

Well Adrien pretty much summarized it [1].

Tx offload consumes cycles, for examples checksum or TSO.
Since those offloads are enabled by default, the application will need to pay for them, even if they are not used.
True, it is possible to disable them, however when new offloads are introduced the application will need to be constantly modified in order to disable the new ones. 

A better approach will be to disable all the Tx offload by default, and enable them according to the application needs. 
This will enable to the PMD to provide the Tx burst function which suits the most to the application specific needs, with no extra overhead.

[1] http://dpdk.org/ml/archives/dev/2017-May/065509.html

Re: [dpdk-dev] [PATCH] doc: announce ABI change on ethdev

[Jerin Jacob]

-----Original Message-----
> Date: Tue, 9 May 2017 15:40:04 +0200
> From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> To: Shahaf Shuler <shahafs@mellanox.com>, Konstantin Ananyev
>  <konstantin.ananyev@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>  Tomasz Kulasek <tomaszx.kulasek@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] doc: announce ABI change on ethdev
> 
> On Mon, May 01, 2017 at 09:58:12AM +0300, Shahaf Shuler wrote:
> > This is an ABI change notice for DPDK 17.08 in librte_ether
> > about changes in rte_eth_txmode structure.
> > 
> > Currently Tx offloads are enabled by default, and can be disabled
> > using ETH_TXQ_FLAGS_NO* flags. This behaviour is not consistent with
> > the Rx side where the Rx offloads are disabled by default and enabled
> > according to bit field in rte_eth_rxmode structure.
> > 
> > The proposal is to disable the Tx offloads by default, and provide
> > a way for the application to enable them in rte_eth_txmode structure.
> > Besides of making the Tx configuration API more consistent for
> > applications, PMDs will be able to provide a better out of the
> > box performance.
> > Finally, as part of the work, the ETH_TXQ_FLAGS_NO* will
> > be superseded as well.
> > 
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> 
> Basically, TX mbuf flags like TSO and checksum offloads won't have to be
> honored by PMDs unless applications request them first while configuring the
> device, just like RX offloads.
> 
> Considering more and more TX offloads will be added over time, I do not
> think expecting them all to be enabled by default is sane. There will always
> be an associated software cost in PMDs, and this solution allows
> applications to selectively enable them as needed for maximum performance.
> 
> Konstantin/Olivier/Tomasz, I do not want to resume the thread about
> tx_prepare(), however this could provide an alternative means to benefit
> from improved performance when applications do not need TSO (or any other
> offload for that matter), while adding consistency to device configuration.
> 
> What's your opinion?
> 
> In any case I'm fine with this change:
> 
> Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Re: [dpdk-dev] [PATCH] doc: announce ABI change on ethdev

[Ananyev, Konstantin]

> 
> This is an ABI change notice for DPDK 17.08 in librte_ether
> about changes in rte_eth_txmode structure.
> 
> Currently Tx offloads are enabled by default, and can be disabled
> using ETH_TXQ_FLAGS_NO* flags. This behaviour is not consistent with
> the Rx side where the Rx offloads are disabled by default and enabled
> according to bit field in rte_eth_rxmode structure.
> 
> The proposal is to disable the Tx offloads by default, and provide
> a way for the application to enable them in rte_eth_txmode structure.
> Besides of making the Tx configuration API more consistent for
> applications, PMDs will be able to provide a better out of the
> box performance.
> Finally, as part of the work, the ETH_TXQ_FLAGS_NO* will
> be superseded as well.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
> looks like this patch has arrived to everyone
> besides dev@dpdk.org resending it again. sorry for
> the noise.
> ---
>  doc/guides/rel_notes/deprecation.rst | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index a3e7c720c..0920b4766 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -81,3 +81,11 @@ Deprecation Notices
> 
>    - ``rte_crpytodev_scheduler_mode_get``, replaced by ``rte_cryptodev_scheduler_mode_get``
>    - ``rte_crpytodev_scheduler_mode_set``, replaced by ``rte_cryptodev_scheduler_mode_set``
> +
> +* ethdev: in 17.08 ABI changes are planned:
> +  Tx offloads will no longer be enabled by default.
> +  Instead, the ``rte_eth_txmode`` structure will be extended with bit field to enable
> +  each Tx offload.
> +  Besides of making the Rx/Tx configuration API more consistent for the
> +  application, PMDs will be able to provide a better out of the box performance.
> +  as part of the work, ``ETH_TXQ_FLAGS_NO*`` will be superseded as well.

Seems ok to me, the only extra suggestion I have:
instead of introducing new bit-fields can we make
txmode (and rxmode) to use DEV_TX_OFFLOAD_(DEV_RX_OFFLOAD_)* values
to specify desired tx(/rx) offloads.
Konstantin

> --
> 2.12.0

Re: [dpdk-dev] [PATCH] doc: announce ABI change on ethdev

[Bruce Richardson]

On Mon, May 01, 2017 at 09:58:12AM +0300, Shahaf Shuler wrote:
> This is an ABI change notice for DPDK 17.08 in librte_ether
> about changes in rte_eth_txmode structure.
> 
> Currently Tx offloads are enabled by default, and can be disabled
> using ETH_TXQ_FLAGS_NO* flags. This behaviour is not consistent with
> the Rx side where the Rx offloads are disabled by default and enabled
> according to bit field in rte_eth_rxmode structure.
> 
> The proposal is to disable the Tx offloads by default, and provide
> a way for the application to enable them in rte_eth_txmode structure.
> Besides of making the Tx configuration API more consistent for
> applications, PMDs will be able to provide a better out of the
> box performance.
> Finally, as part of the work, the ETH_TXQ_FLAGS_NO* will
> be superseded as well.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---

Sounds a great idea to me. I never liked the fact that offloads were on
by default and needed to be explicitly disabled.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH] doc: announce ABI change on ethdev

[Thomas Monjalon]

> > > This is an ABI change notice for DPDK 17.08 in librte_ether
> > > about changes in rte_eth_txmode structure.
> > > 
> > > Currently Tx offloads are enabled by default, and can be disabled
> > > using ETH_TXQ_FLAGS_NO* flags. This behaviour is not consistent with
> > > the Rx side where the Rx offloads are disabled by default and enabled
> > > according to bit field in rte_eth_rxmode structure.
> > > 
> > > The proposal is to disable the Tx offloads by default, and provide
> > > a way for the application to enable them in rte_eth_txmode structure.
> > > Besides of making the Tx configuration API more consistent for
> > > applications, PMDs will be able to provide a better out of the
> > > box performance.
> > > Finally, as part of the work, the ETH_TXQ_FLAGS_NO* will
> > > be superseded as well.
> > > 
> > > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > 
> > Basically, TX mbuf flags like TSO and checksum offloads won't have to be
> > honored by PMDs unless applications request them first while configuring the
> > device, just like RX offloads.
> > 
> > Considering more and more TX offloads will be added over time, I do not
> > think expecting them all to be enabled by default is sane. There will always
> > be an associated software cost in PMDs, and this solution allows
> > applications to selectively enable them as needed for maximum performance.
> > 
> > Konstantin/Olivier/Tomasz, I do not want to resume the thread about
> > tx_prepare(), however this could provide an alternative means to benefit
> > from improved performance when applications do not need TSO (or any other
> > offload for that matter), while adding consistency to device configuration.
> > 
> > What's your opinion?
> > 
> > In any case I'm fine with this change:
> > 
> > Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>

Applied, thanks