From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id B30F8A04DD; Thu, 5 Nov 2020 14:52:32 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 921B3C806; Thu, 5 Nov 2020 14:52:31 +0100 (CET) Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by dpdk.org (Postfix) with ESMTP id 1E8E4C7FC for ; Thu, 5 Nov 2020 14:52:29 +0100 (CET) Received: by mail-wm1-f66.google.com with SMTP id h2so1676558wmm.0 for ; Thu, 05 Nov 2020 05:52:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=C+/e+utiGwz+IjXSwKRN2NoIrZpIMjmMmmq5eN4gZXU=; b=TKK+e9bnq+SHPx0UxAgdidMLQKlcEGAtPVQYYHNMOB/eECYRpRDVXlvn+4lb1kfCNK +ejnz62cMCJet9+aLMUzdfIAJvPhYYsKFq/ggaBOuFBGN0oPXcjmwoyC8WuH2qRd0n2V LWaiTik65bDANeUzBGd72cZa9ENeU5HfeEeEmsLomRpsfNfOQERUqFaQyK00ZyUiI/WH O0Kbrt08rkPhxGxE9mD+Emw6vaV4WXczTzN8kUs/heMYro5tXAcI9zkSAWIhGudsu6br rdWoezJC6jVT9qqPUcGkRTr60XZqbx+wtt4+XIoNC36e7xKv6a6nrlSlkpCMzXrFHGOP bWUQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=C+/e+utiGwz+IjXSwKRN2NoIrZpIMjmMmmq5eN4gZXU=; b=AZy6VDSK3z8wOgHnw/mtqLLq5LaE65mN/O9sJldtGmNmGBz6e4OKHor8kKNuhqKxjw DR6viM1KIBN03T2VDAzf/8laosiAPgr7SEVXSIrcNScaft+2z5TwHGTfoYvyaBAfsHSX uEkxbt8MfbrHk+k9GoG7ZYEs2Ts9Rr/ie896v9B2OYl1yB/suSLMArgUr7i7xO55zcX6 zWbd0inhMoH8Ka6DgDkcrO6xw0S3HJd7z+Pc5LxTCTX7M47dYNTSWriJMtF5UYodiXqW 0QKQvPfyye0s9YscT0Oiy5+MR2qU6zUdf7EahdrRqzgp3QCmhIHS8Bk1q4z569HCCh6K w/0A== X-Gm-Message-State: AOAM531SRz0ZqQP1SP8/NZ8YbFGcHQOhi3hZr1Ix7t2Oo4AbnCuG20/L 6F/bdxDefNyRON4Pb2kW1vgKXg== X-Google-Smtp-Source: ABdhPJzN1K07TiUtP+DKK80UmYahgUc3+N59YuPqhGNHEW6I1ZnoUyFpECDv7x3uSWXXtTN2Gqbz8A== X-Received: by 2002:a7b:c015:: with SMTP id c21mr2860626wmb.22.1604584347851; Thu, 05 Nov 2020 05:52:27 -0800 (PST) Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr. [2a01:cb0c:5:a600:3456:36f7:e65e:d1a0]) by smtp.gmail.com with ESMTPSA id v19sm2412499wmj.31.2020.11.05.05.52.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 Nov 2020 05:52:27 -0800 (PST) Date: Thu, 5 Nov 2020 14:52:26 +0100 From: Olivier Matz To: Ferruh Yigit Cc: Thomas Monjalon , "Yang, SteveX" , Andrew Rybchenko , "dev@dpdk.org" , "Ananyev, Konstantin" , "Xing, Beilei" , "Lu, Wenzhuo" , "Iremonger, Bernard" , "Yang, Qiming" , "mdr@ashroe.eu" , "david.marchand@redhat.com" , jerinj@marvell.com, ajit.khaparde@broadcom.com, maxime.coquelin@redhat.com, matan@nvidia.com, viacheslavo@nvidia.com, hemant.agrawal@nxp.com, bruce.richardson@intel.com, stephen@networkplumber.org Message-ID: <20201105135226.GV1898@platinum> References: <20201028030334.30300-1-stevex.yang@intel.com> <0c5f86c4-49e9-0cfe-fb98-5646712fbeb6@intel.com> <2157818.THrHgzhN9o@thomas> <2109640.M41klPuLie@thomas> <2aa37ca9-720d-c611-0867-44518a4f20ca@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2aa37ca9-720d-c611-0867-44518a4f20ca@intel.com> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH v8 1/2] app/testpmd: fix max rx packet length for VLAN packets X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Nov 05, 2020 at 10:50:45AM +0000, Ferruh Yigit wrote: > On 11/5/2020 10:48 AM, Thomas Monjalon wrote: > > + more maintainers Cc'ed > > > > We have a critical issue with testpmd in -rc2. > > It is blocking a lot of testing. > > Would be good to do a -rc3 today. > > Please see below. > > > > 05/11/2020 11:44, Thomas Monjalon: > > > 05/11/2020 11:37, Ferruh Yigit: > > > > On 11/5/2020 9:33 AM, Yang, SteveX wrote: > > > > > From: Andrew Rybchenko > > > > > > Sent: Thursday, November 5, 2020 4:54 PM > > > > > > To: Thomas Monjalon ; Yang, SteveX > > > > > > ; Yigit, Ferruh > > > > > > Cc: dev@dpdk.org; Ananyev, Konstantin ; > > > > > > Xing, Beilei ; Lu, Wenzhuo ; > > > > > > Iremonger, Bernard ; Yang, Qiming > > > > > > ; mdr@ashroe.eu; nhorman@tuxdriver.com; > > > > > > david.marchand@redhat.com > > > > > > Subject: Re: [dpdk-dev] [PATCH v8 1/2] app/testpmd: fix max rx packet > > > > > > length for VLAN packets > > > > > > > > > > > > On 11/4/20 11:39 PM, Thomas Monjalon wrote: > > > > > > > 04/11/2020 21:19, Ferruh Yigit: > > > > > > > > On 11/4/2020 5:55 PM, Thomas Monjalon wrote: > > > > > > > > > 04/11/2020 18:07, Ferruh Yigit: > > > > > > > > > > On 11/4/2020 4:51 PM, Thomas Monjalon wrote: > > > > > > > > > > > 03/11/2020 14:29, Ferruh Yigit: > > > > > > > > > > > > On 11/2/2020 11:48 AM, Ferruh Yigit wrote: > > > > > > > > > > > > > On 11/2/2020 8:52 AM, SteveX Yang wrote: > > > > > > > > > > > > > > When the max rx packet length is smaller than the sum of mtu > > > > > > > > > > > > > > size and ether overhead size, it should be enlarged, otherwise > > > > > > > > > > > > > > the VLAN packets will be dropped. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: 35b2d13fd6fd ("net: add rte prefix to ether defines") > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: SteveX Yang > > > > > > > > > > > > > > > > > > > > > > > > > > Reviewed-by: Ferruh Yigit > > > > > > > > > > > > > > > > > > > > > > > > Applied to dpdk-next-net/main, thanks. > > > > > > > > > > > > > > > > > > > > > > > > only 1/2 applied since discussion is going on for 2/2. > > > > > > > > > > > > > > > > > > > > > > I'm not sure this testpmd change is good. > > > > > > > > > > > > > > > > > > > > > > Reminder: testpmd is for testing the PMDs. > > > > > > > > > > > Don't we want to see VLAN packets dropped in the case described > > > > > > above? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The patch set 'max_rx_pkt_len' in a way to make MTU 1500 for all > > > > > > > > > > PMDs, otherwise testpmd set hard-coded 'RTE_ETHER_MAX_LEN' > > > > > > value, > > > > > > > > > > which makes MTU between 1492-1500 depending on PMD. > > > > > > > > > > > > > > > > > > > > It is application responsibility to provide correct 'max_rx_pkt_len'. > > > > > > > > > > I guess the original intention was to set MTU as 1500 but was not > > > > > > > > > > correct for all PMDs and this patch is fixing it. > > > > > > > > > > > > > > > > > > > > The same problem in the ethdev, (assuming 'RTE_ETHER_MAX_LEN' > > > > > > will > > > > > > > > > > give MTU 1500), the other patch in the set is to fix it later. > > > > > > > > > > > > > > > > > > OK but the testpmd patch is just hiding the issue, isn't it? > > > > > > > > > > > > > > > > > > > > > > > > > I don't think so, issue was application (testpmd) setting the > > > > > > 'max_rx_pkt_len' > > > > > > > > wrong. > > > > > > > > > > > > > > > > What is hidden? > > > > > > > > > > > > > > I was looking for adding a helper in ethdev API. > > > > > > > But I think I can agree with your way of thinking. > > > > > > > > > > > > > > > > > > > The patch breaks running testpmd on Virtio-Net because the driver > > > > > > populates dev_info.max_rx_pktlen but keeps dev_info.max_mtu equal to > > > > > > UINT16_MAX as it was filled in by ethdev. As the result: > > > > > > > > > > > > Ethdev port_id=0 max_rx_pkt_len 11229 > max valid value 9728 Fail to > > > > > > configure port 0 > > > > > > > > > > Similar issue occurred for other net PMD drivers which use default max_mtu (UINT16_MAX). > > > > > More strict checking condition will be added within new patch sooner. > > > > > > > > > > > > > :( > > > > > > > > For drivers not providing 'max_mtu' information explicitly, the default > > > > 'UINT16_MAX' is set in ethdev layer. > > > > This prevents calculating PMD specific 'overhead' and the logic in the patch is > > > > broken. > > > > > > > > Indeed this makes inconsistency in the driver too, for example for virtio, it > > > > claims 'max_rx_pktlen' as "VIRTIO_MAX_RX_PKTLEN (9728)" and 'max_mtu' as > > > > UINT16_MAX. From 'virtio_mtu_set()' we can see the real limit is > > > > 'VIRTIO_MAX_RX_PKTLEN'. > > > > > > > > When PMDs fixed, the logic in this patch can work but not sure if post -rc2 is > > > > good time to start fixing the PMDs. > > > > > > Do you suggest revert is the best choice here? > > > > > > (copy/pasting previous reply to this eamil) > > One option is revert, but than the issue this patch is trying to fix still remain. > > Other option is the extend the patch as Steve sent [1], the check there is > more like workaround in application, so not nice to have them, but with > extending the deprecation notice (other patch in this patchset) to fix PMDs > too in next release, I would be OK to have these checks. What do you think? +1 for this second option. I think it is ok to have a workaround to fix an issue. Clarifying and uniformizing the ethdev/drivers behavior in that area can come in a second time. > [1] > https://patches.dpdk.org/patch/83717/