Re: [PATCH] net/i40e: enable max frame size at port level

[David Marchand <david.marchand@redhat.com>]

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.


-- 
David Marchand