[dpdk-dev,v2] ether: use a default for max Rx frame size in configure()
Checks
Commit Message
At the moment rte_eth_dev_configure() behaves inconsistent:
- for normal frames: out of range max_rx_pkt_len uses a default
- for jumbo frames: out of range max_rx_pkt_len gives an error
This patch fixes this inconsistency by using a default value
for max_rx_pkt_len both for normal and jumbo frames.
Signed-off-by: Andriy Berestovskyy <Andriy.Berestovskyy@caviumnetworks.com>
---
Notes:
v2 changes:
- reword the commit title according to the check-git-log.sh
lib/librte_ether/rte_ethdev.c | 20 +++++---------------
lib/librte_ether/rte_ethdev.h | 6 +++++-
2 files changed, 10 insertions(+), 16 deletions(-)
Comments
Hi, Andriy
I don't think this is a bug. Return errors when configure an invalid max_rx_pkt_len is suitable for this generic API.
rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_queue, uint16_t nb_tx_queue, const struct rte_eth_conf *eth_conf)
is used to configure an Ethernet device as eth_conf demand.
It's not suitable to give a default value in this function.
BRs,
Qiming
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Andriy
> Berestovskyy
> Sent: Friday, March 24, 2017 7:52 PM
> To: Thomas Monjalon <thomas.monjalon@6wind.com>
> Cc: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v2] ether: use a default for max Rx frame size in
> configure()
>
> At the moment behaves inconsistent:
> - for normal frames: out of range max_rx_pkt_len uses a default
> - for jumbo frames: out of range max_rx_pkt_len gives an error
>
> This patch fixes this inconsistency by using a default value for
> max_rx_pkt_len both for normal and jumbo frames.
>
> Signed-off-by: Andriy Berestovskyy
> <Andriy.Berestovskyy@caviumnetworks.com>
> ---
>
> Notes:
> v2 changes:
> - reword the commit title according to the check-git-log.sh
>
> lib/librte_ether/rte_ethdev.c | 20 +++++---------------
> lib/librte_ether/rte_ethdev.h | 6 +++++-
> 2 files changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index eb0a94a..f560051 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -856,21 +856,11 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t
> nb_rx_q, uint16_t nb_tx_q,
> * length is supported by the configured device.
> */
> if (dev_conf->rxmode.jumbo_frame == 1) {
> - if (dev_conf->rxmode.max_rx_pkt_len >
> - dev_info.max_rx_pktlen) {
> - RTE_PMD_DEBUG_TRACE("ethdev port_id=%d
> max_rx_pkt_len %u"
> - " > max valid value %u\n",
> - port_id,
> - (unsigned)dev_conf-
> >rxmode.max_rx_pkt_len,
> - (unsigned)dev_info.max_rx_pktlen);
> - return -EINVAL;
> - } else if (dev_conf->rxmode.max_rx_pkt_len <
> ETHER_MIN_LEN) {
> - RTE_PMD_DEBUG_TRACE("ethdev port_id=%d
> max_rx_pkt_len %u"
> - " < min valid value %u\n",
> - port_id,
> - (unsigned)dev_conf-
> >rxmode.max_rx_pkt_len,
> - (unsigned)ETHER_MIN_LEN);
> - return -EINVAL;
> + if (dev_conf->rxmode.max_rx_pkt_len >
> dev_info.max_rx_pktlen ||
> + dev_conf->rxmode.max_rx_pkt_len <
> ETHER_MIN_LEN) {
> + /* Use maximum frame size the NIC supports */
> + dev->data->dev_conf.rxmode.max_rx_pkt_len =
> +
> dev_info.max_rx_pktlen;
> }
> } else {
> if (dev_conf->rxmode.max_rx_pkt_len < ETHER_MIN_LEN
> || diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 4be217c..2adfd77 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -349,7 +349,11 @@ enum rte_eth_tx_mq_mode { struct
> rte_eth_rxmode {
> /** The multi-queue packet distribution mode to be used, e.g. RSS.
> */
> enum rte_eth_rx_mq_mode mq_mode;
> - uint32_t max_rx_pkt_len; /**< Only used if jumbo_frame enabled.
> */
> + /**
> + * Desired maximum RX frame size. Too short or too long size will be
> + * substituted by a default value.
> + */
> + uint32_t max_rx_pkt_len;
> uint16_t split_hdr_size; /**< hdr buf size (header_split enabled).*/
> __extension__
> uint16_t header_split : 1, /**< Header Split enable. */
> --
> 2.7.4
Hey Qiming,
On 27.03.2017 08:15, Yang, Qiming wrote:
> I don't think this is a bug. Return errors when configure an invalid max_rx_pkt_len is suitable for this generic API.
It is not a bug, it is an inconsistency. At the moment we can set
max_rx_pkt_len for normal frames and if it is out of range a default
value will be used instead.
IMO we should expect the same behavior from the same function for the
jumbo frames.
So at the moment we have:
jumbo == 0, max_rx_pkt_len == 0, result: max_rx_pkt_len = ETHER_MAX_LEN
jumbo == 0, max_rx_pkt_len == 1200, result: max_rx_pkt_len = 1200
jumbo == 1, max_rx_pkt_len == 0, result: error
jumbo == 1, max_rx_pkt_len == 9K, result: error or max_rx_pkt_len = 9K
> It's not suitable to give a default value in this function.
We use a default value for normal frames at the moment. The comment:
uint32_t max_rx_pkt_len; /**< Only used if jumbo_frame enabled. */
is obsolete and in fact we use max_rx_pkt_len both for jumbo and normal
frames. So the patch clarifies this as well.
Regards,
Andriy
2017-03-24 12:52, Andriy Berestovskyy:
> At the moment rte_eth_dev_configure() behaves inconsistent:
> - for normal frames: out of range max_rx_pkt_len uses a default
> - for jumbo frames: out of range max_rx_pkt_len gives an error
>
> This patch fixes this inconsistency by using a default value
> for max_rx_pkt_len both for normal and jumbo frames.
>
> Signed-off-by: Andriy Berestovskyy <Andriy.Berestovskyy@caviumnetworks.com>
It is a bit strange to use the max value when the input is below the minimum.
Anyway, why not fixing it in the reverse way: returning error for
out of range of non-jumbo frames?
I am not sure setting a default value in the back of the caller is really
a good behaviour.
Hi Thomas,
On 06.04.2017 22:48, Thomas Monjalon wrote:
> Anyway, why not fixing it in the reverse way: returning error for
> out of range of non-jumbo frames?
I guess we need to fix most of the examples then, since most of them
just pass 0 for normal frames. And there is no default for jumbo frames,
so an app must first get this info from the NIC...
> I am not sure setting a default value in the back of the caller is really
> a good behaviour.
From app perspective, any working default is better that a non-working
app, which you have to fix and recompile on each PMD/platform.
What if we use 0 for a default value both for normal and jumbo frames
(i.e. ETHER_MAX_LEN and dev_info.max_rx_pktlen) and an error if user
passed a non-zero max_rx_pkt_len?
It will make it consistent, we will not need to fix the existing apps
and we will have a default both for normal and jumbo frames. Win-win? ;)
Andriy
On Mon, Mar 27, 2017 at 10:38:13AM +0200, Andriy Berestovskyy wrote:
> Hey Qiming,
>
> On 27.03.2017 08:15, Yang, Qiming wrote:
> > I don't think this is a bug. Return errors when configure an invalid max_rx_pkt_len is suitable for this generic API.
>
> It is not a bug, it is an inconsistency. At the moment we can set
> max_rx_pkt_len for normal frames and if it is out of range a default value
> will be used instead.
>
> IMO we should expect the same behavior from the same function for the jumbo
> frames.
>
> So at the moment we have:
> jumbo == 0, max_rx_pkt_len == 0, result: max_rx_pkt_len = ETHER_MAX_LEN
> jumbo == 0, max_rx_pkt_len == 1200, result: max_rx_pkt_len = 1200
Is this really the case right now. My understanding was that when jumbo
== 0 max_rx_pkt_len is ignored. At least on Intel NICS, I believe the
max_rx_pkt_len is always 1518 unless jumbo frame support is explicitly
enabled. [Perhaps the driver maintainers can confirm this for me.]
Regards,
/Bruce
2017-04-07 10:09, Andriy Berestovskyy:
> Hi Thomas,
>
> On 06.04.2017 22:48, Thomas Monjalon wrote:
> > Anyway, why not fixing it in the reverse way: returning error for
> > out of range of non-jumbo frames?
>
> I guess we need to fix most of the examples then, since most of them
> just pass 0 for normal frames. And there is no default for jumbo frames,
> so an app must first get this info from the NIC...
Yes, I would vote to return the NIC capabilities to the app.
> > I am not sure setting a default value in the back of the caller is really
> > a good behaviour.
>
> From app perspective, any working default is better that a non-working
> app, which you have to fix and recompile on each PMD/platform.
Right
That's why there should be a capabilities API for this need.
> What if we use 0 for a default value both for normal and jumbo frames
> (i.e. ETHER_MAX_LEN and dev_info.max_rx_pktlen) and an error if user
> passed a non-zero max_rx_pkt_len?
We can set the right default value if the app input is 0,
as a special case.
For any other value, we must try to set it or return an error.
> It will make it consistent, we will not need to fix the existing apps
> and we will have a default both for normal and jumbo frames. Win-win? ;)
On 07.04.2017 10:34, Thomas Monjalon wrote:
> We can set the right default value if the app input is 0,
> as a special case.
> For any other value, we must try to set it or return an error.
Right, I will resend the patch.
Andriy
@@ -856,21 +856,11 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
* length is supported by the configured device.
*/
if (dev_conf->rxmode.jumbo_frame == 1) {
- if (dev_conf->rxmode.max_rx_pkt_len >
- dev_info.max_rx_pktlen) {
- RTE_PMD_DEBUG_TRACE("ethdev port_id=%d max_rx_pkt_len %u"
- " > max valid value %u\n",
- port_id,
- (unsigned)dev_conf->rxmode.max_rx_pkt_len,
- (unsigned)dev_info.max_rx_pktlen);
- return -EINVAL;
- } else if (dev_conf->rxmode.max_rx_pkt_len < ETHER_MIN_LEN) {
- RTE_PMD_DEBUG_TRACE("ethdev port_id=%d max_rx_pkt_len %u"
- " < min valid value %u\n",
- port_id,
- (unsigned)dev_conf->rxmode.max_rx_pkt_len,
- (unsigned)ETHER_MIN_LEN);
- return -EINVAL;
+ if (dev_conf->rxmode.max_rx_pkt_len > dev_info.max_rx_pktlen ||
+ dev_conf->rxmode.max_rx_pkt_len < ETHER_MIN_LEN) {
+ /* Use maximum frame size the NIC supports */
+ dev->data->dev_conf.rxmode.max_rx_pkt_len =
+ dev_info.max_rx_pktlen;
}
} else {
if (dev_conf->rxmode.max_rx_pkt_len < ETHER_MIN_LEN ||
@@ -349,7 +349,11 @@ enum rte_eth_tx_mq_mode {
struct rte_eth_rxmode {
/** The multi-queue packet distribution mode to be used, e.g. RSS. */
enum rte_eth_rx_mq_mode mq_mode;
- uint32_t max_rx_pkt_len; /**< Only used if jumbo_frame enabled. */
+ /**
+ * Desired maximum RX frame size. Too short or too long size will be
+ * substituted by a default value.
+ */
+ uint32_t max_rx_pkt_len;
uint16_t split_hdr_size; /**< hdr buf size (header_split enabled).*/
__extension__
uint16_t header_split : 1, /**< Header Split enable. */