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 E8563A034C for ; Mon, 12 Dec 2022 14:58:35 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C3B0040687; Mon, 12 Dec 2022 14:58:35 +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 C22B940684 for ; Mon, 12 Dec 2022 14:58:34 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1670853514; 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=i85uHOcVKTOa+KlDBq5L4rh5ehhfFfuCEsACB7jRq6k1MSqqNe7ZRgm2ZJomz+4uA60mfh xXAdJvEM0eE4Feqv4rJjh0+xFCCBKFpR2YKmgdERITBUre+8uC8+qA5v1xbQWlHZAGjunN qGsnoiAzAws8XQwcwmM/CFFCQ4PBMa4= Received: from mail-pg1-f198.google.com (mail-pg1-f198.google.com [209.85.215.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-156-3Li7geNaNLSXJxBpZPYifw-1; Mon, 12 Dec 2022 08:58:31 -0500 X-MC-Unique: 3Li7geNaNLSXJxBpZPYifw-1 Received: by mail-pg1-f198.google.com with SMTP id g1-20020a636b01000000b00479222d9875so5067940pgc.12 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=KTBXiEut5/8nMSgvxzcamTPeOoC1dJk6pVmjIpg320b+0LtoClkB9vcfeRPU0TEkcf m+8uZ6clzl+qoZTdSFd5OswbBybyIJjQfjmFhwH9kAIUL1XJ5INsqUEf1CgDkjTFCC4l MSOyrLZTUYNfANr3UT4IMb+AenIPJgYmlKAoqHiTvwczzGIWzhwlmkU0UGlA1qmaph4A jYztkYMzmvUZTM5kEJeB23Y5v8NSh3yL/uIj0nU07Z15FJtRUD2CiuqxmUlhCFKHnT/s krSX95FdGuPzgvj6ssl2BraR9DhdFo5W3rvSfCR9Modoe8FicxvhYq9ITJErzNABioJg qc1w== X-Gm-Message-State: ANoB5pmLrXJ+vriLmKVl2iP1rOy02oOnqSinF6cQyeRcvRd5pT1eJrB9 Fx1Ah9pDClHnHnI/fKLmSUcckRLaYWRiwdFKNvMmVekfLAfZFPVR04ilmutHwBgT6ZGEmgQthGo CHW2acfhQjfsQTysaYRN9L4I= X-Received: by 2002:a17:903:3302:b0:189:9590:4f98 with SMTP id jk2-20020a170903330200b0018995904f98mr48024014plb.148.1670853509916; 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: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-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