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 F12B9A034C; Mon, 12 Dec 2022 14:54:05 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 54EEA4113F; Mon, 12 Dec 2022 14:54:05 +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 536F040687 for ; Mon, 12 Dec 2022 14:54:04 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1670853243; 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=kNgbuVrXOLuIAhcHTIyzHnmRCWEuMGTlWxzRNWB1qTc=; b=WeQ/aTK3GHAaAN+nD3ZRJyfI5/Pz1z8Z8j0kw5Q+cpn50eT7C3qm3Wbr1PfwOSTmuz5+Dl cPY7wCm+Fzp/3G86ltv6dFIIYFX4LF8TiFECHHB1FDoibRHPgcwrnDOOSz5FDfT5bNaQDb uLVYc3YoKwv7AVHNsqpJaM7wb9aBELQ= Received: from mail-pj1-f72.google.com (mail-pj1-f72.google.com [209.85.216.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-120-8HE9xOUNPqWymI4yYh_4xg-1; Mon, 12 Dec 2022 08:54:02 -0500 X-MC-Unique: 8HE9xOUNPqWymI4yYh_4xg-1 Received: by mail-pj1-f72.google.com with SMTP id x17-20020a17090a8a9100b002196a3b190cso12336042pjn.6 for ; Mon, 12 Dec 2022 05:54:02 -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=kNgbuVrXOLuIAhcHTIyzHnmRCWEuMGTlWxzRNWB1qTc=; b=bJRceqCOaQA12JRl6Itt6NQZk43kj/Ms08xA8+/pDQ5zH537vV7lXtQqjUeDUPPhu8 7OssFeRvPHXgc8IRcueS1ecSAbndg/WBulT0LjSeZxgfIyTdzrZ69zDYd8zU1XPRadji X7IKb5GWdyxfE1Ns1o3uPEcK/ulgeYBPsWJwaT1XOO/nFAAdZl4x5RbjyaS9ehvR8Mmn PrfnhdtKnwutNSH9F0182aTZrarbBrSbCO4u18a4T54YC+quxmGI3Rhs66l/Xy1eDq8O 85m73gO4luoHKxpqogR2HAaLrIwqcSyWmypV6wyrVlmAJVkxQ9ShuHALqCgXa9d56DAW aolw== X-Gm-Message-State: ANoB5pnbMZ0hN7yiCbKkDaTIzNstg6uyWc3GJxrQ6gG8T/zIcyaQlCey y7HAGwk1TDpJSI53Yh0AJMqny2yqIfTJUitNnuuSLVwaeQ4Z642UhjdCkSLgVd5IWrWQyaid15f NwzcIRDQsoJKdkuRkqYk= X-Received: by 2002:a05:6a00:140c:b0:56b:b520:3751 with SMTP id l12-20020a056a00140c00b0056bb5203751mr79780260pfu.29.1670853241349; Mon, 12 Dec 2022 05:54:01 -0800 (PST) X-Google-Smtp-Source: AA0mqf6wTlUtXvi7+oyMLRQb5IDX6XdHzeZzqQe8hDfjG7KO2K+ek5ULuQpjLjVPUn/uLEIwLA4yiej+aQia45YEGv4= X-Received: by 2002:a05:6a00:140c:b0:56b:b520:3751 with SMTP id l12-20020a056a00140c00b0056bb5203751mr79780248pfu.29.1670853240984; Mon, 12 Dec 2022 05:54:00 -0800 (PST) MIME-Version: 1.0 References: <20211207085946.121032-1-dapengx.yu@intel.com> In-Reply-To: <20211207085946.121032-1-dapengx.yu@intel.com> From: David Marchand Date: Mon, 12 Dec 2022 14:53:49 +0100 Message-ID: Subject: Re: [PATCH] net/i40e: enable max frame size at port level To: dapengx.yu@intel.com Cc: Beilei Xing , dev@dpdk.org, 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 Tue, Dec 7, 2021 at 10:02 AM wrote: > > From: Dapeng Yu > > 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