[v1,1/6] ethdev: add min/max MTU to device info
Checks
Commit Message
From: Stephen Hemminger <stephen@networkplumber.org>
This addresses the usability issue raised by OVS at DPDK Userspace
summit. It adds general min/max mtu into device info. For compatiablity,
and to save space, it fits in a hole in existing structure.
The initial version sets max mtu to normal Ethernet, it is up to
PMD to set larger value if it supports Jumbo frames.
Also remove the deprecation notice introduced in 18.11 regarding this
change and bump ethdev ABI version.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Ian Stokes <ian.stokes@intel.com>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
RFC -> v1
* Removed RFC status.
* dev_info->max_mtu set to UINT16_MAX initially instead of ETHER_MTU to
avoid breaking jumbo frame support. ETHER_MTU to be re-introduced
when all PMD drivers modified to support min/max mtu support.
* Bump ethdev ABI version.
* Added ACK from Andrew Rybchenko.
---
doc/guides/rel_notes/deprecation.rst | 12 ------------
doc/guides/rel_notes/release_19_05.rst | 8 +++++++-
lib/librte_ethdev/Makefile | 2 +-
lib/librte_ethdev/meson.build | 2 +-
lib/librte_ethdev/rte_ethdev.c | 7 +++++++
lib/librte_ethdev/rte_ethdev.h | 2 ++
6 files changed, 18 insertions(+), 15 deletions(-)
Comments
On 2/27/2019 9:45 PM, Ian Stokes wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
>
> This addresses the usability issue raised by OVS at DPDK Userspace
> summit. It adds general min/max mtu into device info. For compatiablity,
> and to save space, it fits in a hole in existing structure.
>
> The initial version sets max mtu to normal Ethernet, it is up to
> PMD to set larger value if it supports Jumbo frames.
>
> Also remove the deprecation notice introduced in 18.11 regarding this
> change and bump ethdev ABI version.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Ian Stokes <ian.stokes@intel.com>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> @@ -2524,6 +2524,8 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
> dev_info->rx_desc_lim = lim;
> dev_info->tx_desc_lim = lim;
> dev_info->device = dev->device;
> + dev_info->min_mtu = ETHER_MIN_MTU;
> + dev_info->max_mtu = UINT16_MAX;
Not only mtu but do you think should we document in 'rte_eth_dev_info_get()'
doxygen documentation, the default values that API sets?
>
> RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
> (*dev->dev_ops->dev_infos_get)(dev, dev_info);
> @@ -2587,12 +2589,17 @@ int
> rte_eth_dev_set_mtu(uint16_t port_id, uint16_t mtu)
> {
> int ret;
> + struct rte_eth_dev_info dev_info;
> struct rte_eth_dev *dev;
>
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> dev = &rte_eth_devices[port_id];
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mtu_set, -ENOTSUP);
>
> + rte_eth_dev_info_get(port_id, &dev_info);
If we rely on 'rte_eth_dev_info_get()', we should add a check if "dev_infos_get"
is supported before calling it, like [1]. Unfortunately 'rte_eth_dev_info_get()'
return type is 'void', so we can't know if the struct has valid values or not
otherwise.
Or perhaps if port doesn't support "dev_infos_get", we can skip the mtu check
instead of returning error.
[1]
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
> + if (mtu < dev_info.min_mtu || mtu > dev_info.max_mtu)
> + return -EINVAL;
> +
Should we document this behavior change in the function comment in header file?
Update when -EINVAL returned, etc..
On 3/19/2019 4:15 PM, Ferruh Yigit wrote:
> On 2/27/2019 9:45 PM, Ian Stokes wrote:
>> From: Stephen Hemminger <stephen@networkplumber.org>
>>
>> This addresses the usability issue raised by OVS at DPDK Userspace
>> summit. It adds general min/max mtu into device info. For compatiablity,
>> and to save space, it fits in a hole in existing structure.
>>
>> The initial version sets max mtu to normal Ethernet, it is up to
>> PMD to set larger value if it supports Jumbo frames.
>>
>> Also remove the deprecation notice introduced in 18.11 regarding this
>> change and bump ethdev ABI version.
>>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> Signed-off-by: Ian Stokes <ian.stokes@intel.com>
>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>
>> @@ -2524,6 +2524,8 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>> dev_info->rx_desc_lim = lim;
>> dev_info->tx_desc_lim = lim;
>> dev_info->device = dev->device;
>> + dev_info->min_mtu = ETHER_MIN_MTU;
>> + dev_info->max_mtu = UINT16_MAX;
>
> Not only mtu but do you think should we document in 'rte_eth_dev_info_get()'
> doxygen documentation, the default values that API sets?
>
Sure, that would be useful, I can include it in the v2 of this patch.
What would you document the values under? They are not @params and not
@return, is there a particular label/format that should be used for
values set within a function?
>>
>> RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
>> (*dev->dev_ops->dev_infos_get)(dev, dev_info);
>> @@ -2587,12 +2589,17 @@ int
>> rte_eth_dev_set_mtu(uint16_t port_id, uint16_t mtu)
>> {
>> int ret;
>> + struct rte_eth_dev_info dev_info;
>> struct rte_eth_dev *dev;
>>
>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> dev = &rte_eth_devices[port_id];
>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mtu_set, -ENOTSUP);
>>
>> + rte_eth_dev_info_get(port_id, &dev_info);
>
> If we rely on 'rte_eth_dev_info_get()', we should add a check if "dev_infos_get"
> is supported before calling it, like [1]. Unfortunately 'rte_eth_dev_info_get()'
> return type is 'void', so we can't know if the struct has valid values or not
> otherwise.
> Or perhaps if port doesn't support "dev_infos_get", we can skip the mtu check
> instead of returning error.
>
Hmmm,good point, I assumed rte_eth_dev_info_get() would be implemented
for nearly all devices but again, only assumption, could be wrong.
I'd prefer to err on the side of caution, so for the moment we can skip
the check if dev infos is not available as you suggest. That way it
should still work non supported devices.
> [1]
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
>
>> + if (mtu < dev_info.min_mtu || mtu > dev_info.max_mtu)
>> + return -EINVAL;
>> +
>
> Should we document this behavior change in the function comment in header file?
> Update when -EINVAL returned, etc..
Sure I can take care of that in the v2.
Ian
>
On 3/21/2019 12:50 PM, Ian Stokes wrote:
> On 3/19/2019 4:15 PM, Ferruh Yigit wrote:
>> On 2/27/2019 9:45 PM, Ian Stokes wrote:
>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>>
>>> This addresses the usability issue raised by OVS at DPDK Userspace
>>> summit. It adds general min/max mtu into device info. For compatiablity,
>>> and to save space, it fits in a hole in existing structure.
>>>
>>> The initial version sets max mtu to normal Ethernet, it is up to
>>> PMD to set larger value if it supports Jumbo frames.
>>>
>>> Also remove the deprecation notice introduced in 18.11 regarding this
>>> change and bump ethdev ABI version.
>>>
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>> Signed-off-by: Ian Stokes <ian.stokes@intel.com>
>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>
>>> @@ -2524,6 +2524,8 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>>> dev_info->rx_desc_lim = lim;
>>> dev_info->tx_desc_lim = lim;
>>> dev_info->device = dev->device;
>>> + dev_info->min_mtu = ETHER_MIN_MTU;
>>> + dev_info->max_mtu = UINT16_MAX;
>>
>> Not only mtu but do you think should we document in 'rte_eth_dev_info_get()'
>> doxygen documentation, the default values that API sets?
>>
>
> Sure, that would be useful, I can include it in the v2 of this patch.
> What would you document the values under? They are not @params and not
> @return, is there a particular label/format that should be used for
> values set within a function?
In the description paragraph of the function perhaps? I am not aware of a label
for this.
On 3/21/2019 2:06 PM, Ferruh Yigit wrote:
> On 3/21/2019 12:50 PM, Ian Stokes wrote:
>> On 3/19/2019 4:15 PM, Ferruh Yigit wrote:
>>> On 2/27/2019 9:45 PM, Ian Stokes wrote:
>>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>>>
>>>> This addresses the usability issue raised by OVS at DPDK Userspace
>>>> summit. It adds general min/max mtu into device info. For compatiablity,
>>>> and to save space, it fits in a hole in existing structure.
>>>>
>>>> The initial version sets max mtu to normal Ethernet, it is up to
>>>> PMD to set larger value if it supports Jumbo frames.
>>>>
>>>> Also remove the deprecation notice introduced in 18.11 regarding this
>>>> change and bump ethdev ABI version.
>>>>
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>>>> Signed-off-by: Ian Stokes <ian.stokes@intel.com>
>>>> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>
>>>> @@ -2524,6 +2524,8 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>>>> dev_info->rx_desc_lim = lim;
>>>> dev_info->tx_desc_lim = lim;
>>>> dev_info->device = dev->device;
>>>> + dev_info->min_mtu = ETHER_MIN_MTU;
>>>> + dev_info->max_mtu = UINT16_MAX;
>>>
>>> Not only mtu but do you think should we document in 'rte_eth_dev_info_get()'
>>> doxygen documentation, the default values that API sets?
>>>
>>
>> Sure, that would be useful, I can include it in the v2 of this patch.
>> What would you document the values under? They are not @params and not
>> @return, is there a particular label/format that should be used for
>> values set within a function?
>
> In the description paragraph of the function perhaps? I am not aware of a label
> for this.
>
Perfect, thanks for the clarification.
Ian
@@ -56,18 +56,6 @@ Deprecation Notices
Target release for removal of the legacy API will be defined once most
PMDs have switched to rte_flow.
-* ethdev: Maximum and minimum MTU values vary between hardware devices. In
- hardware agnostic DPDK applications access to such information would allow
- a more accurate way of validating and setting supported MTU values on a per
- device basis rather than using a defined default for all devices. To
- resolve this, the following members will be added to ``rte_eth_dev_info``.
- Note: these can be added to fit a hole in the existing structure for amd64
- but not for 32-bit, as such ABI change will occur as size of the structure
- will increase.
-
- - Member ``uint16_t min_mtu`` the minimum MTU allowed.
- - Member ``uint16_t max_mtu`` the maximum MTU allowed.
-
* meter: New ``rte_color`` definition will be added in 19.02 and that will
replace ``enum rte_meter_color`` in meter library in 19.05. This will help
to consolidate color definition, which is currently replicated in many places,
@@ -116,6 +116,12 @@ ABI Changes
Also, make sure to start the actual text at the margin.
=========================================================
+* ethdev: Additional fields in rte_eth_dev_info.
+
+ The ``rte_eth_dev_info`` structure has had two extra fields
+ added: ``min_mtu`` and ``max_mtu``. Each of these are of type ``uint16_t``.
+ The values of these fields can be set specifically by the PMD drivers as
+ supported values can vary from device to device.
Shared Library Versions
-----------------------
@@ -151,7 +157,7 @@ The libraries prepended with a plus sign were incremented in this version.
librte_distributor.so.1
librte_eal.so.9
librte_efd.so.1
- librte_ethdev.so.11
+ + librte_ethdev.so.12
librte_eventdev.so.6
librte_flow_classify.so.1
librte_gro.so.1
@@ -16,7 +16,7 @@ LDLIBS += -lrte_mbuf -lrte_kvargs -lrte_cmdline -lrte_meter
EXPORT_MAP := rte_ethdev_version.map
-LIBABIVER := 11
+LIBABIVER := 12
SRCS-y += ethdev_private.c
SRCS-y += rte_ethdev.c
@@ -2,7 +2,7 @@
# Copyright(c) 2017 Intel Corporation
name = 'ethdev'
-version = 11
+version = 12
allow_experimental_apis = true
sources = files('ethdev_private.c',
'ethdev_profile.c',
@@ -2524,6 +2524,8 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
dev_info->rx_desc_lim = lim;
dev_info->tx_desc_lim = lim;
dev_info->device = dev->device;
+ dev_info->min_mtu = ETHER_MIN_MTU;
+ dev_info->max_mtu = UINT16_MAX;
RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
(*dev->dev_ops->dev_infos_get)(dev, dev_info);
@@ -2587,12 +2589,17 @@ int
rte_eth_dev_set_mtu(uint16_t port_id, uint16_t mtu)
{
int ret;
+ struct rte_eth_dev_info dev_info;
struct rte_eth_dev *dev;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
dev = &rte_eth_devices[port_id];
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mtu_set, -ENOTSUP);
+ rte_eth_dev_info_get(port_id, &dev_info);
+ if (mtu < dev_info.min_mtu || mtu > dev_info.max_mtu)
+ return -EINVAL;
+
ret = (*dev->dev_ops->mtu_set)(dev, mtu);
if (!ret)
dev->data->mtu = mtu;
@@ -1086,6 +1086,8 @@ struct rte_eth_dev_info {
const char *driver_name; /**< Device Driver name. */
unsigned int if_index; /**< Index to bound host interface, or 0 if none.
Use if_indextoname() to translate into an interface name. */
+ uint16_t min_mtu; /**< Minimum MTU allowed */
+ uint16_t max_mtu; /**< Maximum MTU allowed */
const uint32_t *dev_flags; /**< Device flags */
uint32_t min_rx_bufsize; /**< Minimum size of RX buffer. */
uint32_t max_rx_pktlen; /**< Maximum configurable length of RX pkt. */