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 73706A0548; Fri, 23 Apr 2021 13:42:21 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id ED3FF410D8; Fri, 23 Apr 2021 13:42:20 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mails.dpdk.org (Postfix) with ESMTP id 6B0534014F for ; Fri, 23 Apr 2021 13:42:19 +0200 (CEST) IronPort-SDR: X6JFh5bG72uqWdp1ZHxYux/HLGmFu2CLGsdf9E51Yu2S0VGDAYYE7E6H6QUXjYSjw+1Ds8aBo5 Y39x26rI00Cg== X-IronPort-AV: E=McAfee;i="6200,9189,9962"; a="192866618" X-IronPort-AV: E=Sophos;i="5.82,245,1613462400"; d="scan'208";a="192866618" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Apr 2021 04:42:18 -0700 IronPort-SDR: nR2eS9L5ti3FOG728qKeIdQD3E7qtpfYsBUXVFZOFtYjRiGaJciYMPF3hmIWPWfni8QkU3BRKL R9jsDtvc6FyA== X-IronPort-AV: E=Sophos;i="5.82,245,1613462400"; d="scan'208";a="464297481" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.255.228]) ([10.213.255.228]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Apr 2021 04:42:17 -0700 From: Ferruh Yigit To: Andrew Boyer Cc: dev@dpdk.org, Andrew Rybchenko , Thomas Monjalon References: <20201210024657.72099-1-aboyer@pensando.io> <0f4866fc-b76d-cf83-75e8-86326c02814b@intel.com> X-User: ferruhy Message-ID: <9a32bce1-aa9e-7e73-c58a-38396569bfa3@intel.com> Date: Fri, 23 Apr 2021 12:42:12 +0100 MIME-Version: 1.0 In-Reply-To: <0f4866fc-b76d-cf83-75e8-86326c02814b@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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 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. >> >> 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. > >> >> 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). >> Hi Andrew, 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? Please see a few comments below if there will be new version. <...> >> 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 = 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. > 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. >> - >> +    /* Note: mtu check against min/max is done by the API */ >>       err = ionic_lif_change_mtu(lif, mtu); >>       if (err) >>           return err; >> +    /* Update max frame size */ >> +    max_frame_size = 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 = max_frame_size; >> + 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. >> +    ionic_lif_set_rx_buf_size(lif); >> + 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?