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 D8FF8A04E7; Thu, 5 Nov 2020 16:12:06 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3C03FC87E; Thu, 5 Nov 2020 16:12:05 +0100 (CET) Received: from mail-ot1-f65.google.com (mail-ot1-f65.google.com [209.85.210.65]) by dpdk.org (Postfix) with ESMTP id 767D8C866 for ; Thu, 5 Nov 2020 16:12:02 +0100 (CET) Received: by mail-ot1-f65.google.com with SMTP id n15so1698766otl.8 for ; Thu, 05 Nov 2020 07:12:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=myrj7CZXZHTr63bON2FmWfbrzxRYdZNkWlyrkkHL5II=; b=QqJDj24SwE0/CudMeEsq2MKL6nkvg7rX/r0b7gvlNCQOZ7Q73EGFv+12r9l4f3s6ZJ ts7xfGeYgzeCxiR6f4a0s3S52eE551VGMhQi3u3mmnyazwUYFwDD7ggOEzAQq3pyl53w RYNQApkAEyCvc0cSV/W9KcPidY0mOpDIqUtKo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=myrj7CZXZHTr63bON2FmWfbrzxRYdZNkWlyrkkHL5II=; b=WKu2hW0HtK0l4n7YwTM/RUrSPNV6fetH12woL94d6WjdRMSSOGXFK8UF702m7EywCb u7v88yRcK34KPfoUU65lSxllsBkAR+9lAEudiAvOT+URD1wHfN3YsMR08OvoGOTQWMFh VYq//WikVw3Sp79AH/dGOrzHn9+XJAcP/t7hIDASyKYgQNlNZrBiRJDKRsmA/KMup2y1 0jWhJFLwyJqT8eIAPTG4neaHZUF5n3hT5DZmILLmDJMdfyus/30xtOier5IAfsVXxIur 4psSBXlWE6nrVS/1uUkyj6urfB56Fd8O3D8EZYC4LGk1qTUpzCmgf0KoOpAOPysYXH5z u25Q== X-Gm-Message-State: AOAM533D1JbrtmygfGrhkxGHIaEDwfnErCrHO84fdm1l6Y431yUiJV8d uFJFVvxn6HtFPcw05Rj2mkY7sc0sB97gzNyRA1DdOA== X-Google-Smtp-Source: ABdhPJwVBYxBmx+P0A7i/JcEleS1h5/TczYKHRtiNDNsVH3pYz6x2CcpusEs7LWCHB2lU+6dUxxlOPChSziHwf+A5zc= X-Received: by 2002:a05:6830:1347:: with SMTP id r7mr2022079otq.172.1604589120113; Thu, 05 Nov 2020 07:12:00 -0800 (PST) MIME-Version: 1.0 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> <20201105135226.GV1898@platinum> In-Reply-To: <20201105135226.GV1898@platinum> From: Lance Richardson Date: Thu, 5 Nov 2020 10:11:48 -0500 Message-ID: To: Olivier Matz Cc: Ferruh Yigit , 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 Kumar Khaparde , Maxime Coquelin , matan@nvidia.com, viacheslavo@nvidia.com, hemant.agrawal@nxp.com, Bruce Richardson , Stephen Hemminger Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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" With this change, the bnxt driver fails to initialize under testpmd: Configuring Port 0 (socket 0) Port 0 failed to enable Rx offload JUMBO_FRAME Fail to configure port 0 EAL: Error - exiting with code: 1 It appears that the cause is this bit of code in bnxt_ethdev.c: if (bp->eth_dev->data->mtu > RTE_ETHER_MTU) { bp->eth_dev->data->dev_conf.rxmode.offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME; bp->flags |= BNXT_FLAG_JUMBO; } else { bp->eth_dev->data->dev_conf.rxmode.offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME; bp->flags &= ~BNXT_FLAG_JUMBO; } Should a PMD be overriding this offload on dev_start()? Or should this test be changed to be based on max_rx_pkt_len instead of mtu? Thanks, Lance On Thu, Nov 5, 2020 at 8:52 AM Olivier Matz wrote: > > 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/