From: Ian Stokes <ian.stokes@intel.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>, dev@dpdk.org
Cc: stephen@networkplumber.org
Subject: Re: [dpdk-dev] [PATCH v1 1/6] ethdev: add min/max MTU to device info
Date: Thu, 21 Mar 2019 12:50:28 +0000 [thread overview]
Message-ID: <ae7570af-9ee5-a696-1eba-9b293f801b83@intel.com> (raw)
Message-ID: <20190321125028._MCSGBMnI6scVqR4yPC5B3ktWwXkDcvM6JUEajIsosY@z> (raw)
In-Reply-To: <b8707c85-1c20-6ba3-ca87-c778869fbfc4@intel.com>
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
>
next prev parent reply other threads:[~2019-03-21 12:50 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-27 21:45 [dpdk-dev] [PATCH v1 0/6] " Ian Stokes
2019-02-27 21:45 ` [dpdk-dev] [PATCH v1 1/6] " Ian Stokes
2019-03-19 16:15 ` Ferruh Yigit
2019-03-19 16:15 ` Ferruh Yigit
2019-03-21 12:50 ` Ian Stokes [this message]
2019-03-21 12:50 ` Ian Stokes
2019-03-21 14:06 ` Ferruh Yigit
2019-03-21 14:06 ` Ferruh Yigit
2019-03-21 14:59 ` Ian Stokes
2019-03-21 14:59 ` Ian Stokes
2019-02-27 21:45 ` [dpdk-dev] [PATCH v1 2/6] net/i40e: set min and max MTU for i40e devices Ian Stokes
2019-03-19 16:18 ` Ferruh Yigit
2019-03-19 16:18 ` Ferruh Yigit
2019-03-21 12:57 ` Ian Stokes
2019-03-21 12:57 ` Ian Stokes
2019-02-27 21:45 ` [dpdk-dev] [PATCH v1 3/6] net/i40e: set min and max MTU for i40e VF devices Ian Stokes
2019-02-27 21:45 ` [dpdk-dev] [PATCH v1 4/6] net/ixgbe: set min and max MTU for ixgbe devices Ian Stokes
2019-02-27 21:45 ` [dpdk-dev] [PATCH v1 5/6] net/ixgbe: set min and max MTU for ixgbe VF devices Ian Stokes
2019-02-27 21:45 ` [dpdk-dev] [PATCH v1 6/6] net/e1000: set min and max MTU for igb devices Ian Stokes
2019-03-19 16:30 ` [dpdk-dev] [PATCH v1 0/6] ethdev: add min/max MTU to device info Ferruh Yigit
2019-03-19 16:30 ` Ferruh Yigit
2019-03-21 13:03 ` Ian Stokes
2019-03-21 13:03 ` Ian Stokes
2019-03-22 13:05 ` Ian Stokes
2019-03-22 13:05 ` Ian Stokes
2019-03-22 14:08 ` Ferruh Yigit
2019-03-22 14:08 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ae7570af-9ee5-a696-1eba-9b293f801b83@intel.com \
--to=ian.stokes@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).