From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 098E2A034C; Mon, 12 Dec 2022 14:58:42 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EF8A840687; Mon, 12 Dec 2022 14:58:41 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 0E28040684 for ; Mon, 12 Dec 2022 14:58:39 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1670853519; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=hWwUSwTUNoObe8eAb25CWrE77xWOhTXkRVAjVlC8BEk=; b=IcI76tBIxl8ZPYTkXj/iQYwvfas+LPdxVJgcL4X/lAHPiAUf5G5ucVI498Edy409Zb1AyK JGc3RvSxG6CIgTt6ZhG0fl2mqAPmr+hi9U7FI47lcQdzqyprpi0S6E1IuCj9OJtHN2wQLY AFubb4s+7ed9EQ7kVmUg3rwFs1YOx9U= Received: from mail-pg1-f200.google.com (mail-pg1-f200.google.com [209.85.215.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-588-MiWH06XlNfGDeFwsYF0U1w-1; Mon, 12 Dec 2022 08:58:31 -0500 X-MC-Unique: MiWH06XlNfGDeFwsYF0U1w-1 Received: by mail-pg1-f200.google.com with SMTP id q63-20020a632a42000000b0045724b1dfb9so7613140pgq.3 for ; Mon, 12 Dec 2022 05:58:30 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=hWwUSwTUNoObe8eAb25CWrE77xWOhTXkRVAjVlC8BEk=; b=yqDa1ZkZmWDiKgyD0U7aT+MJhn5oysHm/YfucZuefZEcxYqBwJKTaVPjSbCyxt2Dle C74Yc4WeVXxtGYSdGVSndyLMKA8L1TFshyaWNs77P/P7tLQLGJ/3Dy86It9ogKSeylYB cN9arVh4BssVpmtwXGGOdtvAuz5xh7ZexfGy3B0enGChYA/GCgEYdVi5FABphy9lLHkC pjEqodkN4iGEy2dzVLkqCdlU9gYbBRCKaq4efCZB4HqS4D8T2SAHwPo2k9nHuL7vxljw GyDzhPLGKjwJfL5tWzMTIzd3E/B3zYVFvnJSLFBRtNIj8TJ21xYzwDDgYO3yjpU7WcSx 0VXQ== X-Gm-Message-State: ANoB5pkOR2g3fFJhJ43iNlGQOrIvrZgLUFfPuKa6HstDC2ICZdd6czhZ cjPMThzMC6G8q7jq4QIDRnGzqyHkvVubmcY4645z/fziwuYWUM0MlUak7h9UveyGj8MYPXqG3vT Z0o0cvXvtzonXrolBMf4= X-Received: by 2002:a17:903:3302:b0:189:9590:4f98 with SMTP id jk2-20020a170903330200b0018995904f98mr48024026plb.148.1670853509966; Mon, 12 Dec 2022 05:58:29 -0800 (PST) X-Google-Smtp-Source: AA0mqf4/Ws+Kf8ZRFo+TveEPUTYnYcs9xb25C4vU5gMXJeoL3SSgH8Z0kyjZTD/V0pWhOA3BXIPrW0HsDpIH76RMWDc= X-Received: by 2002:a17:903:3302:b0:189:9590:4f98 with SMTP id jk2-20020a170903330200b0018995904f98mr48024009plb.148.1670853509597; Mon, 12 Dec 2022 05:58:29 -0800 (PST) MIME-Version: 1.0 References: <20211207085946.121032-1-dapengx.yu@intel.com> In-Reply-To: From: David Marchand Date: Mon, 12 Dec 2022 14:58:18 +0100 Message-ID: Subject: Re: [PATCH] net/i40e: enable max frame size at port level To: Jie Wang , "Yuan, DukaiX" , Wenxuan Wu , Yuying Zhang Cc: Beilei Xing , dev@dpdk.org, Dapeng Yu , stable@dpdk.org, Qi Zhang , Thomas Monjalon , Kevin Traynor , Luca Boccassi , Ferruh Yigit , Timothy Redaelli , Christophe Fontaine X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Mon, Dec 12, 2022 at 2:53 PM David Marchand wrote: > On Tue, Dec 7, 2021 at 10:02 AM wrote: > > > > From: Dapeng Yu 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 > > 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