[RFC,v2] doc: announce max Rx packet len field deprecation
Checks
Commit Message
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
Cc: Thomas Monjalon <thomas@monjalon.net>
Cc: Andrew Rybchenko <arybchenko@solarflare.com>
Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
Cc: Matan Azrad <matan@nvidia.com>
Cc: Olivier Matz <olivier.matz@6wind.com>
Cc: Jerin Jacob <jerinj@marvell.com>
v2:
* ``uint32_t mtu`` moved to ``struct rte_eth_conf``
* The "Driver is responsible from updating ``(struct
rte_eth_dev)->data->mtu``" updated because ethdev layer also can do
this. The intention there was both APIs should update the variable.
Another open question is from Andrew, if we can remove the ``uint32_t
max_rx_pkt_len`` completely from the ``rte_eth_dev_configure()``.
This may force applications to have one more additional
``rte_eth_dev_set_mtu()`` call for device initialization, but if
applications are OK with the default values most of times, agree that
removing is easier solution, please comment.
---
doc/guides/rel_notes/deprecation.rst | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
Comments
On Tue, Nov 24, 2020 at 9:36 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Cc: Matan Azrad <matan@nvidia.com>
> Cc: Olivier Matz <olivier.matz@6wind.com>
> Cc: Jerin Jacob <jerinj@marvell.com>
>
> v2:
> * ``uint32_t mtu`` moved to ``struct rte_eth_conf``
> * The "Driver is responsible from updating ``(struct
> rte_eth_dev)->data->mtu``" updated because ethdev layer also can do
> this. The intention there was both APIs should update the variable.
>
> Another open question is from Andrew, if we can remove the ``uint32_t
> max_rx_pkt_len`` completely from the ``rte_eth_dev_configure()``.
> This may force applications to have one more additional
> ``rte_eth_dev_set_mtu()`` call for device initialization, but if
> applications are OK with the default values most of times, agree that
> removing is easier solution, please comment.
> ---
> doc/guides/rel_notes/deprecation.rst | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 96986fabd598..73e43581d615 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -88,6 +88,31 @@ Deprecation Notices
> will be limited to maximum 256 queues.
> Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be removed.
>
> +* ethdev: ``uint32_t max_rx_pkt_len`` field of ``struct rte_eth_rxmode``, will be
> + replaced by a new ``uint32_t mtu`` field of ``struct rte_eth_conf`` in v21.11.
> + The new ``mtu`` field will be used to configure the initial device MTU via
> + ``rte_eth_dev_configure()`` API.
> + Later MTU can be changed by ``rte_eth_dev_set_mtu()`` API as done now.
> + The existing ``(struct rte_eth_dev)->data->mtu`` variable will be used to store
> + the configured ``mtu`` value,
> + and this new ``(struct rte_eth_dev)->data->dev_conf.mtu`` variable will
> + be used to store the user configuration request.
> + Unlike ``max_rx_pkt_len``, which was valid only when ``JUMBO_FRAME`` enabled,
> + ``mtu`` field will be always valid.
> + When ``mtu`` config is not provided by the application, default ``RTE_ETHER_MTU``
> + value will be used.
> + ``(struct rte_eth_dev)->data->mtu`` should be updated after MTU set successfully,
> + either by ``rte_eth_dev_configure()`` or ``rte_eth_dev_set_mtu()``.
> +
> + An application may need to configure device for a specific Rx packet size, like for
> + cases ``DEV_RX_OFFLOAD_SCATTER`` is not supported and device received packet size
> + can't be bigger than Rx buffer size.
> + To cover these cases an application needs to know the device packet overhead to be
> + able to calculate the ``mtu`` corresponding to a Rx buffer size, for this
> + ``(struct rte_eth_dev_info).max_rx_pktlen`` will be kept,
> + the device packet overhead can be calculated as:
> + ``(struct rte_eth_dev_info).max_rx_pktlen - (struct rte_eth_dev_info).max_mtu``
> +
> * sched: To allow more traffic classes, flexible mapping of pipe queues to
> traffic classes, and subport level configuration of pipes and queues
> changes will be made to macros, data structures and API functions defined
> --
> 2.26.2
>
On 11/24/20 8:36 PM, Ferruh Yigit wrote:
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
A couple of questions below, but anyway:
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Cc: Matan Azrad <matan@nvidia.com>
> Cc: Olivier Matz <olivier.matz@6wind.com>
> Cc: Jerin Jacob <jerinj@marvell.com>
>
> v2:
> * ``uint32_t mtu`` moved to ``struct rte_eth_conf``
> * The "Driver is responsible from updating ``(struct
> rte_eth_dev)->data->mtu``" updated because ethdev layer also can do
> this. The intention there was both APIs should update the variable.
>
> Another open question is from Andrew, if we can remove the ``uint32_t
> max_rx_pkt_len`` completely from the ``rte_eth_dev_configure()``.
> This may force applications to have one more additional
> ``rte_eth_dev_set_mtu()`` call for device initialization, but if
> applications are OK with the default values most of times, agree that
> removing is easier solution, please comment.
Still valid plus I'd remove JUMBO_FRAME offload since
it is redundant. We have max_mtu and max_rx_pktlen in dev_info.
On 11/26/2020 11:28 AM, Andrew Rybchenko wrote:
> On 11/24/20 8:36 PM, Ferruh Yigit wrote:
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>
> A couple of questions below, but anyway:
>
> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>
>> ---
>> Cc: Thomas Monjalon <thomas@monjalon.net>
>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
>> Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
>> Cc: Matan Azrad <matan@nvidia.com>
>> Cc: Olivier Matz <olivier.matz@6wind.com>
>> Cc: Jerin Jacob <jerinj@marvell.com>
>>
>> v2:
>> * ``uint32_t mtu`` moved to ``struct rte_eth_conf``
>> * The "Driver is responsible from updating ``(struct
>> rte_eth_dev)->data->mtu``" updated because ethdev layer also can do
>> this. The intention there was both APIs should update the variable.
>>
>> Another open question is from Andrew, if we can remove the ``uint32_t
>> max_rx_pkt_len`` completely from the ``rte_eth_dev_configure()``.
>> This may force applications to have one more additional
>> ``rte_eth_dev_set_mtu()`` call for device initialization, but if
>> applications are OK with the default values most of times, agree that
>> removing is easier solution, please comment.
>
> Still valid
Yep, waiting for more comments for it.
> plus I'd remove JUMBO_FRAME offload since
> it is redundant. We have max_mtu and max_rx_pktlen in dev_info.
>
Right, I missed that 'max_mtu' & 'max_rx_pktlen' can be used to detect jumbo
frame capability. +1 to remove JUMBO_FRAME offload.
I don't know if should it be part of this deprecation notice, or a separate one.
It is related, but logically not exactly part of this deprecation notice.
Hi
Looks very good direction - it will do some order here.
Will be also good to define what is MTU exactly:
Maximum Rx packet size \ Maximum Tx packet size \ both?
Is it include L2\L3 headers? Vlan?
How is it going together with ``uint32_t max_lro_pkt_size``?
Also when should the user configure DEV_RX_OFFLOAD_SCATTER?
Matan
From: Ferruh Yigit
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Cc: Matan Azrad <matan@nvidia.com>
> Cc: Olivier Matz <olivier.matz@6wind.com>
> Cc: Jerin Jacob <jerinj@marvell.com>
>
> v2:
> * ``uint32_t mtu`` moved to ``struct rte_eth_conf``
> * The "Driver is responsible from updating ``(struct
> rte_eth_dev)->data->mtu``" updated because ethdev layer also can do
> this. The intention there was both APIs should update the variable.
>
> Another open question is from Andrew, if we can remove the ``uint32_t
> max_rx_pkt_len`` completely from the ``rte_eth_dev_configure()``.
> This may force applications to have one more additional
> ``rte_eth_dev_set_mtu()`` call for device initialization, but if applications are
> OK with the default values most of times, agree that removing is easier
> solution, please comment.
> ---
> doc/guides/rel_notes/deprecation.rst | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 96986fabd598..73e43581d615 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -88,6 +88,31 @@ Deprecation Notices
> will be limited to maximum 256 queues.
> Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be
> removed.
>
> +* ethdev: ``uint32_t max_rx_pkt_len`` field of ``struct
> +rte_eth_rxmode``, will be
> + replaced by a new ``uint32_t mtu`` field of ``struct rte_eth_conf`` in v21.11.
> + The new ``mtu`` field will be used to configure the initial device
> +MTU via
> + ``rte_eth_dev_configure()`` API.
> + Later MTU can be changed by ``rte_eth_dev_set_mtu()`` API as done now.
> + The existing ``(struct rte_eth_dev)->data->mtu`` variable will be
> +used to store
> + the configured ``mtu`` value,
> + and this new ``(struct rte_eth_dev)->data->dev_conf.mtu`` variable
> +will
> + be used to store the user configuration request.
> + Unlike ``max_rx_pkt_len``, which was valid only when ``JUMBO_FRAME``
> +enabled,
> + ``mtu`` field will be always valid.
> + When ``mtu`` config is not provided by the application, default
> +``RTE_ETHER_MTU``
> + value will be used.
> + ``(struct rte_eth_dev)->data->mtu`` should be updated after MTU set
> +successfully,
> + either by ``rte_eth_dev_configure()`` or ``rte_eth_dev_set_mtu()``.
> +
> + An application may need to configure device for a specific Rx packet
> + size, like for cases ``DEV_RX_OFFLOAD_SCATTER`` is not supported and
> + device received packet size can't be bigger than Rx buffer size.
> + To cover these cases an application needs to know the device packet
> + overhead to be able to calculate the ``mtu`` corresponding to a Rx
> + buffer size, for this ``(struct rte_eth_dev_info).max_rx_pktlen``
> + will be kept, the device packet overhead can be calculated as:
> + ``(struct rte_eth_dev_info).max_rx_pktlen - (struct
> + rte_eth_dev_info).max_mtu``
> +
> * sched: To allow more traffic classes, flexible mapping of pipe queues to
> traffic classes, and subport level configuration of pipes and queues
> changes will be made to macros, data structures and API functions defined
> --
> 2.26.2
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Matan Azrad
> Sent: Thursday, November 26, 2020 7:31 PM
>
> Hi
>
> Looks very good direction - it will do some order here.
>
> Will be also good to define what is MTU exactly:
> Maximum Rx packet size \ Maximum Tx packet size \ both?
If we cannot find a use case requiring different RX and TX packet size limits, I prefer combining them into one. And in this case, "mtu" (Max TRANSMISSION Unit) is not the optimal name.
Furthermore, the conventional use of "MTU" does not count the L2 headers.
> Is it include L2\L3 headers? Vlan?
I once proposed that it should count what goes on the wire, i.e. including L2 and any VLAN headers. But instead, it was decided to mimic what Linux does: Do not count single VLAN headers, but do count double VLAN headers. I don't know if double VLAN headers only count one of the VLAN headers or both. And I don't know if single VLAN headers are counted if VLAN stripping/appending is disabled.
In other words: Matan has a good point here... It should be well defined (and well documented)!
> How is it going together with ``uint32_t max_lro_pkt_size``?
>
> Also when should the user configure DEV_RX_OFFLOAD_SCATTER?
>
> Matan
>
> From: Ferruh Yigit
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> > Cc: Thomas Monjalon <thomas@monjalon.net>
> > Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> > Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > Cc: Matan Azrad <matan@nvidia.com>
> > Cc: Olivier Matz <olivier.matz@6wind.com>
> > Cc: Jerin Jacob <jerinj@marvell.com>
> >
> > v2:
> > * ``uint32_t mtu`` moved to ``struct rte_eth_conf``
> > * The "Driver is responsible from updating ``(struct
> > rte_eth_dev)->data->mtu``" updated because ethdev layer also can do
> > this. The intention there was both APIs should update the variable.
> >
> > Another open question is from Andrew, if we can remove the ``uint32_t
> > max_rx_pkt_len`` completely from the ``rte_eth_dev_configure()``.
> > This may force applications to have one more additional
> > ``rte_eth_dev_set_mtu()`` call for device initialization, but if
> applications are
> > OK with the default values most of times, agree that removing is
> easier
> > solution, please comment.
> > ---
> > doc/guides/rel_notes/deprecation.rst | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> > b/doc/guides/rel_notes/deprecation.rst
> > index 96986fabd598..73e43581d615 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -88,6 +88,31 @@ Deprecation Notices
> > will be limited to maximum 256 queues.
> > Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be
> > removed.
> >
> > +* ethdev: ``uint32_t max_rx_pkt_len`` field of ``struct
> > +rte_eth_rxmode``, will be
> > + replaced by a new ``uint32_t mtu`` field of ``struct
> rte_eth_conf`` in v21.11.
> > + The new ``mtu`` field will be used to configure the initial device
> > +MTU via
> > + ``rte_eth_dev_configure()`` API.
> > + Later MTU can be changed by ``rte_eth_dev_set_mtu()`` API as done
> now.
> > + The existing ``(struct rte_eth_dev)->data->mtu`` variable will be
> > +used to store
> > + the configured ``mtu`` value,
> > + and this new ``(struct rte_eth_dev)->data->dev_conf.mtu`` variable
> > +will
> > + be used to store the user configuration request.
> > + Unlike ``max_rx_pkt_len``, which was valid only when
> ``JUMBO_FRAME``
> > +enabled,
> > + ``mtu`` field will be always valid.
> > + When ``mtu`` config is not provided by the application, default
> > +``RTE_ETHER_MTU``
> > + value will be used.
> > + ``(struct rte_eth_dev)->data->mtu`` should be updated after MTU
> set
> > +successfully,
> > + either by ``rte_eth_dev_configure()`` or
> ``rte_eth_dev_set_mtu()``.
> > +
> > + An application may need to configure device for a specific Rx
> packet
> > + size, like for cases ``DEV_RX_OFFLOAD_SCATTER`` is not supported
> and
> > + device received packet size can't be bigger than Rx buffer size.
> > + To cover these cases an application needs to know the device
> packet
> > + overhead to be able to calculate the ``mtu`` corresponding to a Rx
> > + buffer size, for this ``(struct rte_eth_dev_info).max_rx_pktlen``
> > + will be kept, the device packet overhead can be calculated as:
> > + ``(struct rte_eth_dev_info).max_rx_pktlen - (struct
> > + rte_eth_dev_info).max_mtu``
> > +
Please note that the vmxnet3 driver behaves different that other NIC drivers regarding when it can be configured in Linux. The interface needs to be running before it accepts many of the configuration parameters. I don't know if it behaves similarly in DPDK.
I have CC'ed the vmxnet3 maintainer, Yong Wang, in case VMware has an opinion regarding this RFC.
> > * sched: To allow more traffic classes, flexible mapping of pipe
> queues to
> > traffic classes, and subport level configuration of pipes and
> queues
> > changes will be made to macros, data structures and API functions
> defined
> > --
> > 2.26.2
>
26/11/2020 13:34, Ferruh Yigit:
> On 11/26/2020 11:28 AM, Andrew Rybchenko wrote:
> > On 11/24/20 8:36 PM, Ferruh Yigit wrote:
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> >
> > A couple of questions below, but anyway:
> >
> > Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >
> >> Another open question is from Andrew, if we can remove the ``uint32_t
> >> max_rx_pkt_len`` completely from the ``rte_eth_dev_configure()``.
> >> This may force applications to have one more additional
> >> ``rte_eth_dev_set_mtu()`` call for device initialization, but if
> >> applications are OK with the default values most of times, agree that
> >> removing is easier solution, please comment.
> >
> > Still valid
>
> Yep, waiting for more comments for it.
In general, I am in favor of
- avoiding redundancy in API
- have more specific functions in API
So yes, no need to keep a field for rte_eth_dev_configure()
if the same can be done with rte_eth_dev_set_mtu().
> > plus I'd remove JUMBO_FRAME offload since
> > it is redundant. We have max_mtu and max_rx_pktlen in dev_info.
>
> Right, I missed that 'max_mtu' & 'max_rx_pktlen' can be used to detect jumbo
> frame capability. +1 to remove JUMBO_FRAME offload.
If we can manage without this (strange) offload flag,
I am for dropping it.
> I don't know if should it be part of this deprecation notice, or a separate one.
Let's keep this first notice in 20.11 to show the direction.
> It is related, but logically not exactly part of this deprecation notice.
We can update or add more notices during next year.
Acked-by: Thomas Monjalon <thomas@monjalon.net>
Applied, thanks
@@ -88,6 +88,31 @@ Deprecation Notices
will be limited to maximum 256 queues.
Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS`` will be removed.
+* ethdev: ``uint32_t max_rx_pkt_len`` field of ``struct rte_eth_rxmode``, will be
+ replaced by a new ``uint32_t mtu`` field of ``struct rte_eth_conf`` in v21.11.
+ The new ``mtu`` field will be used to configure the initial device MTU via
+ ``rte_eth_dev_configure()`` API.
+ Later MTU can be changed by ``rte_eth_dev_set_mtu()`` API as done now.
+ The existing ``(struct rte_eth_dev)->data->mtu`` variable will be used to store
+ the configured ``mtu`` value,
+ and this new ``(struct rte_eth_dev)->data->dev_conf.mtu`` variable will
+ be used to store the user configuration request.
+ Unlike ``max_rx_pkt_len``, which was valid only when ``JUMBO_FRAME`` enabled,
+ ``mtu`` field will be always valid.
+ When ``mtu`` config is not provided by the application, default ``RTE_ETHER_MTU``
+ value will be used.
+ ``(struct rte_eth_dev)->data->mtu`` should be updated after MTU set successfully,
+ either by ``rte_eth_dev_configure()`` or ``rte_eth_dev_set_mtu()``.
+
+ An application may need to configure device for a specific Rx packet size, like for
+ cases ``DEV_RX_OFFLOAD_SCATTER`` is not supported and device received packet size
+ can't be bigger than Rx buffer size.
+ To cover these cases an application needs to know the device packet overhead to be
+ able to calculate the ``mtu`` corresponding to a Rx buffer size, for this
+ ``(struct rte_eth_dev_info).max_rx_pktlen`` will be kept,
+ the device packet overhead can be calculated as:
+ ``(struct rte_eth_dev_info).max_rx_pktlen - (struct rte_eth_dev_info).max_mtu``
+
* sched: To allow more traffic classes, flexible mapping of pipe queues to
traffic classes, and subport level configuration of pipes and queues
changes will be made to macros, data structures and API functions defined