DPDK patches and discussions
 help / color / mirror / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: dapengx.yu@intel.com
Cc: Beilei Xing <beilei.xing@intel.com>,
	dev@dpdk.org, stable@dpdk.org,  Qi Zhang <qi.z.zhang@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>,
	 Kevin Traynor <ktraynor@redhat.com>,
	Luca Boccassi <bluca@debian.org>,
	 Ferruh Yigit <ferruh.yigit@amd.com>,
	Timothy Redaelli <tredaelli@redhat.com>,
	 Christophe Fontaine <cfontain@redhat.com>
Subject: Re: [PATCH] net/i40e: enable max frame size at port level
Date: Mon, 12 Dec 2022 14:53:49 +0100	[thread overview]
Message-ID: <CAJFAV8whu0FnWCjMP2wCnOEXbO=eK-MX_a3fGEjn=igOLjuf_g@mail.gmail.com> (raw)
In-Reply-To: <20211207085946.121032-1-dapengx.yu@intel.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


  parent reply	other threads:[~2022-12-12 13:54 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-07  8:59 dapengx.yu
     [not found] ` <BYAPR11MB2711AB91F380189FB6AFCC52FE359@BYAPR11MB2711.namprd11.prod.outlook.com>
     [not found]   ` <BYAPR11MB27110434B9262C0E2BED3AD3FE3A9@BYAPR11MB2711.namprd11.prod.outlook.com>
2022-02-21  9:28     ` Zhang, Peng1X
2022-02-21 10:42     ` Zhang, Peng1X
2022-02-21 11:22       ` Zhang, Qi Z
2022-02-22  9:52         ` Kevin Traynor
2022-12-12 13:53 ` David Marchand [this message]
2022-12-12 13:58   ` David Marchand
2022-12-12 14:37     ` [PATCH] net/i40e: drop link check when configuring frame size David Marchand
2022-12-13  9:18       ` [PATCH v2] net/i40e: don't check link status on device start David Marchand
2023-01-03 14:02         ` David Marchand
2023-01-09  9:21           ` David Marchand
2023-01-13 13:33             ` David Marchand
2023-01-13 13:39               ` Zhang, Helin
2023-01-13 13:46                 ` David Marchand
2023-01-13 13:50                   ` Zhang, Helin
2023-01-13 13:53                     ` David Marchand
2023-01-16 11:02                       ` Su, Simei
2023-02-07 11:31                         ` Thomas Monjalon
2023-02-07 14:05                           ` Su, Simei
2023-03-06  6:53         ` Su, Simei
2023-03-06 11:05           ` Zhang, Qi Z

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='CAJFAV8whu0FnWCjMP2wCnOEXbO=eK-MX_a3fGEjn=igOLjuf_g@mail.gmail.com' \
    --to=david.marchand@redhat.com \
    --cc=beilei.xing@intel.com \
    --cc=bluca@debian.org \
    --cc=cfontain@redhat.com \
    --cc=dapengx.yu@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@amd.com \
    --cc=ktraynor@redhat.com \
    --cc=qi.z.zhang@intel.com \
    --cc=stable@dpdk.org \
    --cc=thomas@monjalon.net \
    --cc=tredaelli@redhat.com \
    /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).