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 23A71A0579; Sun, 25 Apr 2021 01:18:14 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A46824068C; Sun, 25 Apr 2021 01:18:12 +0200 (CEST) Received: from mail-pg1-f181.google.com (mail-pg1-f181.google.com [209.85.215.181]) by mails.dpdk.org (Postfix) with ESMTP id 32A804013F for ; Sun, 25 Apr 2021 01:18:11 +0200 (CEST) Received: by mail-pg1-f181.google.com with SMTP id t22so2424338pgu.0 for ; Sat, 24 Apr 2021 16:18:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pensando.io; s=google; h=from:message-id:mime-version:subject:date:in-reply-to:cc:to :references; bh=y36iiowBpi7W2V8tpLVkHI44pzBnZdeUcx67tgUMRSc=; b=mbYCFyQmQXuf38Ehm82NmnqWd7Yy/HXdsV6XfQLLb8PIqnxiNydiV6uImHLYLoKtyG VkZy7IzKJLkT1CdVGctCrMdxg60gtvKnlQDe1p6r5n+y+RtBa6hfu0T34fT6VcGi+rF4 4nKxsQiXmCaGCkMmfK+DIqYmJkcPLKeGiHLWWGu3DyG2oMsZSeRpLdpq97H/MyjaLv1q kkgp0ldkl9/mmESuypeeymlNNwasjzpP4D7gYFP34Uf6yuycW5X9ZSw67zQlAKWSwXku c3ITg0xOtTBItdrmFfpxL3am9UkKPtkf73xSYtjKxlQ5LcfyY1rgfoyTeAgjFYtbCm1p VBeQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:message-id:mime-version:subject:date :in-reply-to:cc:to:references; bh=y36iiowBpi7W2V8tpLVkHI44pzBnZdeUcx67tgUMRSc=; b=thWSybh359OL1qedj3xuTqWuFP917O3NoZA5Rg9u5Vm/DdWxkYVT7ROojjwT1LNzuw KiB9/xSEZKxX1Z2G4WmOOQ5yRUZIbDh2lQ/pgImp5eoWTas3I9Ey7JLVNfKic3cYfa03 D8X095f81XyRD8aYmP9+SdXh6W12X7rVUxcIPIGy9Ug8pPCNSJRTbWqMk1PyQq0fIwge Ezoppy87cdL/tc9hLhb5WCtdJHnIdZUDK3S/W/42QvtRgGETnxMBbC+bLJ0jyQ2zeA6P rsWB5VFzE4AFbsBpKLk3YWb7HVpvXkmJStGKwykHvS18VfJndRhOT0vyEBjP0Q4XPErn WALw== X-Gm-Message-State: AOAM5308u+f4w6AUYxNcc+L8uwXoyipUOTvfD3/Gtlowev63cCT+iabQ iee8DEJuLluGXaeDgANPI2Ydkw== X-Google-Smtp-Source: ABdhPJxhFtac1Aeacb6HqNnSHDaOs5uTsJpAXcI20F4RYlalADzr4ifBiWQpiETtZ6ePg0f92E3+Ww== X-Received: by 2002:a63:4e43:: with SMTP id o3mr10222716pgl.22.1619306290299; Sat, 24 Apr 2021 16:18:10 -0700 (PDT) Received: from ?IPv6:2600:1700:6b0:fde0:498b:9ed7:a65b:a06d? ([2600:1700:6b0:fde0:498b:9ed7:a65b:a06d]) by smtp.gmail.com with ESMTPSA id 137sm7836588pfx.172.2021.04.24.16.18.09 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sat, 24 Apr 2021 16:18:09 -0700 (PDT) From: Andrew Boyer Message-Id: <7D04044C-260F-4B15-A6EA-91BC9256045F@pensando.io> Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.60.0.2.21\)) Date: Sat, 24 Apr 2021 19:18:07 -0400 In-Reply-To: <9a32bce1-aa9e-7e73-c58a-38396569bfa3@intel.com> Cc: dev@dpdk.org, Andrew Rybchenko , Thomas Monjalon To: Ferruh Yigit References: <20201210024657.72099-1-aboyer@pensando.io> <0f4866fc-b76d-cf83-75e8-86326c02814b@intel.com> <9a32bce1-aa9e-7e73-c58a-38396569bfa3@intel.com> X-Mailer: Apple Mail (2.3654.60.0.2.21) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [dpdk-dev] [RFC] net/ionic: update MTU calculations 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 Sender: "dev" > On Apr 23, 2021, at 7:42 AM, Ferruh Yigit = wrote: >=20 > On 12/15/2020 12:26 PM, Ferruh Yigit wrote: >> On 12/10/2020 2:46 AM, Andrew Boyer wrote: >>> This RFC is in response to the threads about testpmd mtu settings >>> and the deprecation of max-rx-pkt-len. >>>=20 >>> It took us a while to figure out what we were supposed to be doing >>> in this part of the API. It is not clear if max_rx_pkt_len should be >>> an input to or an output from the PMD. >> 'max_rx_pkt_len' is input to the PMD, but it needs to be in sync with = MTU value, that is why PMDs update this value when MTU is updated to = keep the sync. >>>=20 >>> The code below represents what we believe should happen today, and = also >>> happens to pass the DTS tests which were failing prior to this = change >>> (checksum and jumbo_frame at least). >>>=20 >=20 > Hi Andrew, >=20 > I am updating the status of the patch as "change requested", what is = the status of this patch, will there be a new version? > Is DTS still failing? >=20 > Please see a few comments below if there will be new version. >=20 Thank you for your thorough review! I am working on a new feature at present so upstreaming has been = delayed. We were blocked from posting the next set of our patches by some arm64 = build stuff, but thanks to Juraj & co. I think that is cleared up. = It=E2=80=99s just a matter of when I will get to posting them. -Andrew > <...> >=20 >>> diff --git a/drivers/net/ionic/ionic_ethdev.c = b/drivers/net/ionic/ionic_ethdev.c >>> index 925da3e29..7000de3f9 100644 >>> --- a/drivers/net/ionic/ionic_ethdev.c >>> +++ b/drivers/net/ionic/ionic_ethdev.c >>> @@ -357,25 +357,19 @@ ionic_dev_mtu_set(struct rte_eth_dev *eth_dev, = uint16_t mtu) >>> int err; >>> IONIC_PRINT_CALL(); >>> + IONIC_PRINT(INFO, "Setting mtu %u\n", mtu); >>> - /* >>> - * Note: mtu check against IONIC_MIN_MTU, IONIC_MAX_MTU >>> - * is done by the the API. >>> - */ >>> - >>> - /* >>> - * Max frame size is MTU + Ethernet header + VLAN + QinQ >>> - * (plus ETHER_CRC_LEN if the adapter is able to keep CRC) >>> - */ >>> - max_frame_size =3D mtu + RTE_ETHER_HDR_LEN + 4 + 4; >>> - >>> - if (eth_dev->data->dev_conf.rxmode.max_rx_pkt_len < = max_frame_size) >>> - return -EINVAL; >> The max frame size calculation depends on what HW support, but if = VLAN header is supported above calculation and check is correct. >=20 > Removing above check seems correct thing to do, instead should check = against 'dev_info.max_mtu' which is already done in ethdev layer, so = nothing needed here. >=20 >>> - >>> + /* Note: mtu check against min/max is done by the API */ >>> err =3D ionic_lif_change_mtu(lif, mtu); >>> if (err) >>> return err; >>> + /* Update max frame size */ >>> + max_frame_size =3D mtu + RTE_ETHER_HDR_LEN; >> I guess you are removing the CRC because your HW strips the CRC in = the Rx buffer, but this limit is not for Rx buffer, it is for the frame = size HW accepts, and since recevied frame will have the CRC it should be = included into the calculation. >>> + eth_dev->data->dev_conf.rxmode.max_rx_pkt_len =3D = max_frame_size; >>> + >=20 > Although 'rxmode.max_rx_pkt_len' is input to driver, it is related = with MTU, which is also input from application in this function, so OK = to update 'rxmode.max_rx_pkt_len' here. >=20 >>> + ionic_lif_set_rx_buf_size(lif); >>> + >=20 > This seems to keep the local copy of 'rxmode.max_rx_pkt_len' and use = local copy instead, is this just refactoring or is there any other = reason for local copy?