net/i40e: enable max frame size at port level
Checks
Commit Message
From: Dapeng Yu <dapengx.yu@intel.com>
Currently max frame size is set at queue level, which makes the values
of the following counters wrong when a jumbo frame is received.
The expected value:
rx_good_bytes: 0
rx_errors: 1
rx_oversize_errors: 1
The actual value:
rx_good_bytes: 1626
rx_errors: 0
rx_oversize_errors: 0
This patch enables setting max frame size at port level, and makes the
values above right.
Cc: stable@dpdk.org
Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
---
drivers/net/i40e/i40e_ethdev.c | 41 ++++++++++++++++++++++++++++------
1 file changed, 34 insertions(+), 7 deletions(-)
Comments
> -----Original Message-----
> From: dapengx.yu@intel.com <dapengx.yu@intel.com>
> Sent: Tuesday, December 7, 2021 5:00 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
> Subject: [PATCH] net/i40e: enable max frame size at port level
>
> From: Dapeng Yu <dapengx.yu@intel.com>
>
> Currently max frame size is set at queue level, which makes the values of the
> following counters wrong when a jumbo frame is received.
>
> The expected value:
> rx_good_bytes: 0
> rx_errors: 1
> rx_oversize_errors: 1
>
> The actual value:
> rx_good_bytes: 1626
> rx_errors: 0
> rx_oversize_errors: 0
>
> This patch enables setting max frame size at port level, and makes the values
> above right.
>
> Cc: stable@dpdk.org
>
> Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
> ---
Tested-by: Peng Zhang <peng1x.zhang@intel.com>
> -----Original Message-----
> From: dapengx.yu@intel.com <dapengx.yu@intel.com>
> Sent: Tuesday, December 7, 2021 5:00 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
> Subject: [PATCH] net/i40e: enable max frame size at port level
>
> From: Dapeng Yu <dapengx.yu@intel.com>
>
> Currently max frame size is set at queue level, which makes the values of the
> following counters wrong when a jumbo frame is received.
>
> The expected value:
> rx_good_bytes: 0
> rx_errors: 1
> rx_oversize_errors: 1
>
> The actual value:
> rx_good_bytes: 1626
> rx_errors: 0
> rx_oversize_errors: 0
>
> This patch enables setting max frame size at port level, and makes the values
> above right.
>
> Cc: stable@dpdk.org
>
> Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
> --
Tested-by: Peng Zhang <peng1x.zhang@intel.com>
> -----Original Message-----
> From: Zhang, Peng1X <peng1x.zhang@intel.com>
> Sent: Monday, February 21, 2022 6:43 PM
> To: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [PATCH] net/i40e: enable max frame size at port level
>
> > -----Original Message-----
> > From: dapengx.yu@intel.com <dapengx.yu@intel.com>
> > Sent: Tuesday, December 7, 2021 5:00 PM
> > To: Xing, Beilei <beilei.xing@intel.com>
> > Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
> > Subject: [PATCH] net/i40e: enable max frame size at port level
> >
> > From: Dapeng Yu <dapengx.yu@intel.com>
> >
> > Currently max frame size is set at queue level, which makes the values
> > of the following counters wrong when a jumbo frame is received.
> >
> > The expected value:
> > rx_good_bytes: 0
> > rx_errors: 1
> > rx_oversize_errors: 1
> >
> > The actual value:
> > rx_good_bytes: 1626
> > rx_errors: 0
> > rx_oversize_errors: 0
> >
> > This patch enables setting max frame size at port level, and makes the
> > values above right.
> >
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
> > --
> Tested-by: Peng Zhang <peng1x.zhang@intel.com>
Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
Applied to dpdk-next-net-intel.
Thanks
Qi
On 21/02/2022 11:22, Zhang, Qi Z wrote:
>
>
>> -----Original Message-----
>> From: Zhang, Peng1X <peng1x.zhang@intel.com>
>> Sent: Monday, February 21, 2022 6:43 PM
>> To: dev@dpdk.org; stable@dpdk.org
>> Subject: RE: [PATCH] net/i40e: enable max frame size at port level
>>
>>> -----Original Message-----
>>> From: dapengx.yu@intel.com <dapengx.yu@intel.com>
>>> Sent: Tuesday, December 7, 2021 5:00 PM
>>> To: Xing, Beilei <beilei.xing@intel.com>
>>> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org
>>> Subject: [PATCH] net/i40e: enable max frame size at port level
>>>
>>> From: Dapeng Yu <dapengx.yu@intel.com>
>>>
>>> Currently max frame size is set at queue level, which makes the values
>>> of the following counters wrong when a jumbo frame is received.
>>>
>>> The expected value:
>>> rx_good_bytes: 0
>>> rx_errors: 1
>>> rx_oversize_errors: 1
>>>
>>> The actual value:
>>> rx_good_bytes: 1626
>>> rx_errors: 0
>>> rx_oversize_errors: 0
>>>
>>> This patch enables setting max frame size at port level, and makes the
>>> values above right.
>>>
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
>>> --
>> Tested-by: Peng Zhang <peng1x.zhang@intel.com>
>
> Reviewed-by: Qi Zhang <qi.z.zhang@intel.com>
>
> Applied to dpdk-next-net-intel.
>
Please add a Fixes: tag so we can know which LTS it is relevant for.
> Thanks
> Qi
>
On Tue, Dec 7, 2021 at 10:02 AM <dapengx.yu@intel.com> wrote:
>
> From: Dapeng Yu <dapengx.yu@intel.com>
>
> Currently max frame size is set at queue level, which makes the values
> of the following counters wrong when a jumbo frame is received.
>
> The expected value:
> rx_good_bytes: 0
> rx_errors: 1
> rx_oversize_errors: 1
>
> The actual value:
> rx_good_bytes: 1626
> rx_errors: 0
> rx_oversize_errors: 0
>
> This patch enables setting max frame size at port level, and makes the
> values above right.
>
> Cc: stable@dpdk.org
>
> Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
I am looking again at this patch that triggered regressions in stable
branches, when used with OVS.
> ---
> drivers/net/i40e/i40e_ethdev.c | 41 ++++++++++++++++++++++++++++------
> 1 file changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index c0bfff43ee..ef9b2b2414 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -386,6 +386,7 @@ static int i40e_set_default_mac_addr(struct rte_eth_dev *dev,
> struct rte_ether_addr *mac_addr);
>
> static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
> +static void i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size);
>
> static int i40e_ethertype_filter_convert(
> const struct rte_eth_ethertype_filter *input,
> @@ -1709,11 +1710,6 @@ eth_i40e_dev_init(struct rte_eth_dev *dev, void *init_params __rte_unused)
> */
> i40e_add_tx_flow_control_drop_filter(pf);
>
> - /* Set the max frame size to 0x2600 by default,
> - * in case other drivers changed the default value.
> - */
> - i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, TRUE, false, 0, NULL);
> -
If I understand correctly, the max framesize (9728) was always applied
in dev_init, before this change.
> /* initialize RSS rule list */
> TAILQ_INIT(&pf->rss_config_list);
>
> @@ -2364,6 +2360,7 @@ i40e_dev_start(struct rte_eth_dev *dev)
> uint32_t intr_vector = 0;
> struct i40e_vsi *vsi;
> uint16_t nb_rxq, nb_txq;
> + uint16_t max_frame_size;
>
> hw->adapter_stopped = 0;
>
> @@ -2502,6 +2499,9 @@ i40e_dev_start(struct rte_eth_dev *dev)
> "please call hierarchy_commit() "
> "before starting the port");
>
> + max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD;
> + i40e_set_mac_max_frame(dev, max_frame_size);
> +
And now this change tries to configure, at dev_start step, the max
frame size that the application actually requested.
Which seems ok, afaiu.
> return I40E_SUCCESS;
>
> tx_err:
> @@ -2848,6 +2848,9 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev)
> return i40e_phy_conf_link(hw, abilities, speed, false);
> }
>
> +#define CHECK_INTERVAL 100 /* 100ms */
> +#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */
> +
> static __rte_always_inline void
> update_link_reg(struct i40e_hw *hw, struct rte_eth_link *link)
> {
> @@ -2914,8 +2917,6 @@ static __rte_always_inline void
> update_link_aq(struct i40e_hw *hw, struct rte_eth_link *link,
> bool enable_lse, int wait_to_complete)
> {
> -#define CHECK_INTERVAL 100 /* 100ms */
> -#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */
> uint32_t rep_cnt = MAX_REPEAT_TIME;
> struct i40e_link_status link_status;
> int status;
> @@ -6719,6 +6720,7 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev)
> if (!ret)
> rte_eth_dev_callback_process(dev,
> RTE_ETH_EVENT_INTR_LSC, NULL);
> +
> break;
> default:
> PMD_DRV_LOG(DEBUG, "Request %u is not supported yet",
> @@ -12103,6 +12105,31 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
> return ret;
> }
>
> +static void
> +i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size)
> +{
> + struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> + uint32_t rep_cnt = MAX_REPEAT_TIME;
> + struct rte_eth_link link;
> + enum i40e_status_code status;
> +
> + do {
> + update_link_reg(hw, &link);
> + if (link.link_status)
> + break;
> +
> + rte_delay_ms(CHECK_INTERVAL);
> + } while (--rep_cnt);
> +
> + if (link.link_status) {
> + status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false, NULL);
> + if (status != I40E_SUCCESS)
> + PMD_DRV_LOG(ERR, "Failed to set max frame size at port level");
> + } else {
> + PMD_DRV_LOG(ERR, "Set max frame size at port level not applicable on link down");
> + }
But here, applying the configuration when the link is up only, looks
very wrong to me.
If the link is down, then this configuration will never get applied.
And what is worse, the application has no indication of it since no
error code is propagated.
So the application thinks the port started, but it will never get "big" packets.
Like for example:
- unplug cable,
- start testpmd and configure mtu to 9000,
# dpdk-testpmd -l 0,2,3 --in-memory -a 0000:05:00.1 -- -i --disable-link-check
...
Configuring Port 0 (socket 0)
i40e_set_mac_max_frame(): Set max frame size at port level not
applicable on link down
Port 0: 3C:FD:FE:A0:A4:C5
Done
testpmd> port stop 0
Stopping ports...
Done
testpmd> port config mtu 0 9000
testpmd> port start 0
i40e_set_mac_max_frame(): Set max frame size at port level not
applicable on link down
Port 0: 3C:FD:FE:A0:A4:C5
Done
- plug back cable, and send "big" packets, like for example ping with
-s 8000 from other side of the cable,
- no "big" packet is received on the dpdk port, and its rx_errors
xstats keeps increasing,
Looking at the original change, why did you not ignore link status state?
I would just move "i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX,
TRUE, false, 0, NULL);" in dev_start.
On Mon, Dec 12, 2022 at 2:53 PM David Marchand
<david.marchand@redhat.com> wrote:
> On Tue, Dec 7, 2021 at 10:02 AM <dapengx.yu@intel.com> wrote:
> >
> > From: Dapeng Yu <dapengx.yu@intel.com>
I got a bounce on his mail address, so adding people who
submitted/reviewed later fixes for this change.
Please guys, have a look.
> >
> > Currently max frame size is set at queue level, which makes the values
> > of the following counters wrong when a jumbo frame is received.
> >
> > The expected value:
> > rx_good_bytes: 0
> > rx_errors: 1
> > rx_oversize_errors: 1
> >
> > The actual value:
> > rx_good_bytes: 1626
> > rx_errors: 0
> > rx_oversize_errors: 0
> >
> > This patch enables setting max frame size at port level, and makes the
> > values above right.
> >
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Dapeng Yu <dapengx.yu@intel.com>
>
> I am looking again at this patch that triggered regressions in stable
> branches, when used with OVS.
>
>
> > ---
> > drivers/net/i40e/i40e_ethdev.c | 41 ++++++++++++++++++++++++++++------
> > 1 file changed, 34 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> > index c0bfff43ee..ef9b2b2414 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -386,6 +386,7 @@ static int i40e_set_default_mac_addr(struct rte_eth_dev *dev,
> > struct rte_ether_addr *mac_addr);
> >
> > static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
> > +static void i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size);
> >
> > static int i40e_ethertype_filter_convert(
> > const struct rte_eth_ethertype_filter *input,
> > @@ -1709,11 +1710,6 @@ eth_i40e_dev_init(struct rte_eth_dev *dev, void *init_params __rte_unused)
> > */
> > i40e_add_tx_flow_control_drop_filter(pf);
> >
> > - /* Set the max frame size to 0x2600 by default,
> > - * in case other drivers changed the default value.
> > - */
> > - i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, TRUE, false, 0, NULL);
> > -
>
> If I understand correctly, the max framesize (9728) was always applied
> in dev_init, before this change.
>
>
> > /* initialize RSS rule list */
> > TAILQ_INIT(&pf->rss_config_list);
> >
> > @@ -2364,6 +2360,7 @@ i40e_dev_start(struct rte_eth_dev *dev)
> > uint32_t intr_vector = 0;
> > struct i40e_vsi *vsi;
> > uint16_t nb_rxq, nb_txq;
> > + uint16_t max_frame_size;
> >
> > hw->adapter_stopped = 0;
> >
> > @@ -2502,6 +2499,9 @@ i40e_dev_start(struct rte_eth_dev *dev)
> > "please call hierarchy_commit() "
> > "before starting the port");
> >
> > + max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD;
> > + i40e_set_mac_max_frame(dev, max_frame_size);
> > +
>
> And now this change tries to configure, at dev_start step, the max
> frame size that the application actually requested.
> Which seems ok, afaiu.
>
>
> > return I40E_SUCCESS;
> >
> > tx_err:
> > @@ -2848,6 +2848,9 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev)
> > return i40e_phy_conf_link(hw, abilities, speed, false);
> > }
> >
> > +#define CHECK_INTERVAL 100 /* 100ms */
> > +#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */
> > +
> > static __rte_always_inline void
> > update_link_reg(struct i40e_hw *hw, struct rte_eth_link *link)
> > {
> > @@ -2914,8 +2917,6 @@ static __rte_always_inline void
> > update_link_aq(struct i40e_hw *hw, struct rte_eth_link *link,
> > bool enable_lse, int wait_to_complete)
> > {
> > -#define CHECK_INTERVAL 100 /* 100ms */
> > -#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */
> > uint32_t rep_cnt = MAX_REPEAT_TIME;
> > struct i40e_link_status link_status;
> > int status;
> > @@ -6719,6 +6720,7 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev)
> > if (!ret)
> > rte_eth_dev_callback_process(dev,
> > RTE_ETH_EVENT_INTR_LSC, NULL);
> > +
> > break;
> > default:
> > PMD_DRV_LOG(DEBUG, "Request %u is not supported yet",
> > @@ -12103,6 +12105,31 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
> > return ret;
> > }
> >
> > +static void
> > +i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size)
> > +{
> > + struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > + uint32_t rep_cnt = MAX_REPEAT_TIME;
> > + struct rte_eth_link link;
> > + enum i40e_status_code status;
> > +
> > + do {
> > + update_link_reg(hw, &link);
> > + if (link.link_status)
> > + break;
> > +
> > + rte_delay_ms(CHECK_INTERVAL);
> > + } while (--rep_cnt);
> > +
> > + if (link.link_status) {
> > + status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false, NULL);
> > + if (status != I40E_SUCCESS)
> > + PMD_DRV_LOG(ERR, "Failed to set max frame size at port level");
> > + } else {
> > + PMD_DRV_LOG(ERR, "Set max frame size at port level not applicable on link down");
> > + }
>
> But here, applying the configuration when the link is up only, looks
> very wrong to me.
>
> If the link is down, then this configuration will never get applied.
> And what is worse, the application has no indication of it since no
> error code is propagated.
>
> So the application thinks the port started, but it will never get "big" packets.
>
>
> Like for example:
> - unplug cable,
> - start testpmd and configure mtu to 9000,
> # dpdk-testpmd -l 0,2,3 --in-memory -a 0000:05:00.1 -- -i --disable-link-check
> ...
> Configuring Port 0 (socket 0)
> i40e_set_mac_max_frame(): Set max frame size at port level not
> applicable on link down
> Port 0: 3C:FD:FE:A0:A4:C5
> Done
>
> testpmd> port stop 0
> Stopping ports...
> Done
>
> testpmd> port config mtu 0 9000
>
> testpmd> port start 0
> i40e_set_mac_max_frame(): Set max frame size at port level not
> applicable on link down
> Port 0: 3C:FD:FE:A0:A4:C5
> Done
>
> - plug back cable, and send "big" packets, like for example ping with
> -s 8000 from other side of the cable,
> - no "big" packet is received on the dpdk port, and its rx_errors
> xstats keeps increasing,
>
>
> Looking at the original change, why did you not ignore link status state?
> I would just move "i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX,
> TRUE, false, 0, NULL);" in dev_start.
>
>
@@ -386,6 +386,7 @@ static int i40e_set_default_mac_addr(struct rte_eth_dev *dev,
struct rte_ether_addr *mac_addr);
static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
+static void i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size);
static int i40e_ethertype_filter_convert(
const struct rte_eth_ethertype_filter *input,
@@ -1709,11 +1710,6 @@ eth_i40e_dev_init(struct rte_eth_dev *dev, void *init_params __rte_unused)
*/
i40e_add_tx_flow_control_drop_filter(pf);
- /* Set the max frame size to 0x2600 by default,
- * in case other drivers changed the default value.
- */
- i40e_aq_set_mac_config(hw, I40E_FRAME_SIZE_MAX, TRUE, false, 0, NULL);
-
/* initialize RSS rule list */
TAILQ_INIT(&pf->rss_config_list);
@@ -2364,6 +2360,7 @@ i40e_dev_start(struct rte_eth_dev *dev)
uint32_t intr_vector = 0;
struct i40e_vsi *vsi;
uint16_t nb_rxq, nb_txq;
+ uint16_t max_frame_size;
hw->adapter_stopped = 0;
@@ -2502,6 +2499,9 @@ i40e_dev_start(struct rte_eth_dev *dev)
"please call hierarchy_commit() "
"before starting the port");
+ max_frame_size = dev->data->mtu + I40E_ETH_OVERHEAD;
+ i40e_set_mac_max_frame(dev, max_frame_size);
+
return I40E_SUCCESS;
tx_err:
@@ -2848,6 +2848,9 @@ i40e_dev_set_link_down(struct rte_eth_dev *dev)
return i40e_phy_conf_link(hw, abilities, speed, false);
}
+#define CHECK_INTERVAL 100 /* 100ms */
+#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */
+
static __rte_always_inline void
update_link_reg(struct i40e_hw *hw, struct rte_eth_link *link)
{
@@ -2914,8 +2917,6 @@ static __rte_always_inline void
update_link_aq(struct i40e_hw *hw, struct rte_eth_link *link,
bool enable_lse, int wait_to_complete)
{
-#define CHECK_INTERVAL 100 /* 100ms */
-#define MAX_REPEAT_TIME 10 /* 1s (10 * 100ms) in total */
uint32_t rep_cnt = MAX_REPEAT_TIME;
struct i40e_link_status link_status;
int status;
@@ -6719,6 +6720,7 @@ i40e_dev_handle_aq_msg(struct rte_eth_dev *dev)
if (!ret)
rte_eth_dev_callback_process(dev,
RTE_ETH_EVENT_INTR_LSC, NULL);
+
break;
default:
PMD_DRV_LOG(DEBUG, "Request %u is not supported yet",
@@ -12103,6 +12105,31 @@ i40e_cloud_filter_qinq_create(struct i40e_pf *pf)
return ret;
}
+static void
+i40e_set_mac_max_frame(struct rte_eth_dev *dev, uint16_t size)
+{
+ struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ uint32_t rep_cnt = MAX_REPEAT_TIME;
+ struct rte_eth_link link;
+ enum i40e_status_code status;
+
+ do {
+ update_link_reg(hw, &link);
+ if (link.link_status)
+ break;
+
+ rte_delay_ms(CHECK_INTERVAL);
+ } while (--rep_cnt);
+
+ if (link.link_status) {
+ status = i40e_aq_set_mac_config(hw, size, TRUE, 0, false, NULL);
+ if (status != I40E_SUCCESS)
+ PMD_DRV_LOG(ERR, "Failed to set max frame size at port level");
+ } else {
+ PMD_DRV_LOG(ERR, "Set max frame size at port level not applicable on link down");
+ }
+}
+
RTE_LOG_REGISTER_SUFFIX(i40e_logtype_init, init, NOTICE);
RTE_LOG_REGISTER_SUFFIX(i40e_logtype_driver, driver, NOTICE);
#ifdef RTE_ETHDEV_DEBUG_RX