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 9EF13A0C51; Wed, 21 Jul 2021 18:47:26 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1E6DE4014D; Wed, 21 Jul 2021 18:47:26 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by mails.dpdk.org (Postfix) with ESMTP id 8B0EC40143 for ; Wed, 21 Jul 2021 18:47:24 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10052"; a="275294477" X-IronPort-AV: E=Sophos;i="5.84,258,1620716400"; d="scan'208";a="275294477" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Jul 2021 09:47:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.84,258,1620716400"; d="scan'208";a="576748948" Received: from orsmsx602.amr.corp.intel.com ([10.22.229.15]) by fmsmga001.fm.intel.com with ESMTP; 21 Jul 2021 09:47:21 -0700 Received: from orsmsx611.amr.corp.intel.com (10.22.229.24) by ORSMSX602.amr.corp.intel.com (10.22.229.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.10; Wed, 21 Jul 2021 09:47:21 -0700 Received: from orsmsx604.amr.corp.intel.com (10.22.229.17) by ORSMSX611.amr.corp.intel.com (10.22.229.24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.10; Wed, 21 Jul 2021 09:47:20 -0700 Received: from ORSEDG602.ED.cps.intel.com (10.7.248.7) by orsmsx604.amr.corp.intel.com (10.22.229.17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.10 via Frontend Transport; Wed, 21 Jul 2021 09:47:20 -0700 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (104.47.57.174) by edgegateway.intel.com (134.134.137.103) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2242.10; Wed, 21 Jul 2021 09:47:20 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Boe2naAM+5N2RQiZgEiyEFTl6pucVaGV0ll+GFrw9NwA3c+4U8XCCJrJnOxEXdBgNEZmzFUG2Ih/SszZBSs56Wtex9YXC3jH9yJLWyS/abFyQJbMrsIieJ65pIwfYNUklWpyTnymGVwH02nDmveR80h4ELrAt98m7JSWuBbPYvPPkp/FRV0rKJ6NBXVtLNbeTMGvWhTGjQ6iVQHFXMukAXBYCZdd6JdS0NGgxeaBqWToxJKJCdG/YqCdtvtuMkkyV9R4YrxlWnNe06YX0UY+tkY7ZeFnhA4T5j3W5pxcM0CInUC6UPjEnFEQvtNgN/Wz3xNLC0Aow8gqhGvTwKla6Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=wRQXYH4qglDCW1pf1M8QakOdpwd2oasnIIHpAFQBDo4=; b=hlaR8bZH/Kmmv98EZUj9nKy50eJ21B9e3xuy1yNv6VplUcZRQCalM1zgeiC08Oo4ouj7fvOXVuHUCFx+KMg5f0Y23J42o1kQU/QS2eNONYqP0F56deFWQjNDhYFRrKLvZtlm9gfQN7mLRI3dxybylALx0Kwy8tTJu9vYzo/4qQBw8sDfLEiFNNLqZ3PSpB2hHTkDwcAy8e1BYZ4Z9bCvlIyr+2Ken2Mg02s2eo2h5eqQFK1YEbe7gtDJllCAvbnKB1jb78iwmT840FNNFT9dbMv4Cfav0SQAGcelUCbUOFKL3kJVsb2h4ri8AYY4WPKThE87+WzZfRhWRiCevSElbw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=wRQXYH4qglDCW1pf1M8QakOdpwd2oasnIIHpAFQBDo4=; b=akMbPJqNSO1V5CJIYs6tcuOKn4P7DO4ZHql0NUp3NCN/cnVyw5ZABODdAVzHM4yr/7pN/oVcAgkSQNydL6i5SQmW25g0cTgYtjpkFnxUrrlmb/OiNe3A9XEYt4WtHjX/eH6laFb112NQHZbKXqgvpPG2IogKhtHx4xkxVCoq+IE= Authentication-Results: dpdk.org; dkim=none (message not signed) header.d=none;dpdk.org; dmarc=none action=none header.from=intel.com; Received: from SJ0PR11MB5005.namprd11.prod.outlook.com (2603:10b6:a03:2d3::21) by BYAPR11MB3606.namprd11.prod.outlook.com (2603:10b6:a03:b5::25) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4331.25; Wed, 21 Jul 2021 16:47:13 +0000 Received: from SJ0PR11MB5005.namprd11.prod.outlook.com ([fe80::a018:c8bc:f54e:514c]) by SJ0PR11MB5005.namprd11.prod.outlook.com ([fe80::a018:c8bc:f54e:514c%7]) with mapi id 15.20.4331.034; Wed, 21 Jul 2021 16:47:13 +0000 To: Andrew Rybchenko , Jerin Jacob , Xiaoyun Li , Chas Williams , "Min Hu (Connor)" , Hemant Agrawal , Sachin Saxena , Qi Zhang , Xiao Wang , Matan Azrad , Shahaf Shuler , Viacheslav Ovsiienko , Harman Kalra , Maciej Czekaj , Ray Kinsella , Neil Horman , Bernard Iremonger , Bruce Richardson , Konstantin Ananyev , John McNamara , Igor Russkikh , Pavel Belous , Steven Webster , Matt Peters , Somalapuram Amaranath , Rasesh Mody , Shahed Shaikh , Ajit Khaparde , "Somnath Kotur" , Nithin Dabilpuram , Kiran Kumar K , "Sunil Kumar Kori" , Satha Rao , "Rahul Lakkireddy" , Haiyue Wang , Marcin Wojtas , Michal Krawczyk , Guy Tzalik , Evgeny Schemeilin , Igor Chauskin , Gagandeep Singh , John Daley , Hyong Youb Kim , Ziyang Xuan , Xiaoyun Wang , Guoyang Zhou , "Yisen Zhuang" , Lijun Ou , Beilei Xing , Jingjing Wu , Qiming Yang , Andrew Boyer , Rosen Xu , Shijith Thotton , Srisivasubramanian Srinivasan , Zyta Szpak , Liron Himi , Heinrich Kuhn , Devendra Singh Rawat , Keith Wiles , Jiawen Wu , Jian Wang , "Maxime Coquelin" , Chenbo Xia , Nicolas Chautru , David Hunt , Harry van Haaren , Cristian Dumitrescu , Radu Nicolau , Akhil Goyal , Tomasz Kantecki , Declan Doherty , Pavan Nikhilesh , Kirill Rybalchenko , Jasvinder Singh , Thomas Monjalon CC: References: <20210709172923.3369846-1-ferruh.yigit@intel.com> <6655b8c1-82d8-27af-087c-c63153e79d3d@oktetlabs.ru> From: Ferruh Yigit X-User: ferruhy Message-ID: <61fbde10-1e3e-45a4-9877-f57f0aac2360@intel.com> Date: Wed, 21 Jul 2021 17:46:36 +0100 In-Reply-To: <6655b8c1-82d8-27af-087c-c63153e79d3d@oktetlabs.ru> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-ClientProxiedBy: AM6PR0502CA0067.eurprd05.prod.outlook.com (2603:10a6:20b:56::44) To SJ0PR11MB5005.namprd11.prod.outlook.com (2603:10b6:a03:2d3::21) MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.0.206] (37.228.236.146) by AM6PR0502CA0067.eurprd05.prod.outlook.com (2603:10a6:20b:56::44) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4331.21 via Frontend Transport; Wed, 21 Jul 2021 16:46:40 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 7165fcea-2b43-4a5b-df20-08d94c67305d X-MS-TrafficTypeDiagnostic: BYAPR11MB3606: X-LD-Processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:7691; X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: SEwCMZWXYHrK67ItvGc5qzQBRvx87irvD1MibAkzIj+iz9atvWMhNTXSZvm89x6yph2ydR59qw8eNZVGdme5LproCqXs8I+5nfRdioheQRnk/x9LzB3O50PI56l3ThknC6e0S45+u6HCs/4MYQdaKV0Za65A9AHfO3PkJQtjxZ0/m+wJGePcVEg/fcQdq2NlwMhbF7kLhKuEZbVr0qXUuG4QqkrVSADKnh3o3lSlz8fhNQsM5hC67fPoC7pIye+loTn8qG0eWX9veRYGQ9YEbSJ+7IVMRu8kB4oKFKDOOP12IoenqgsdM60qS/JWwi6xasV8JgSse4nrE3Ca+hSnNHzKlqzfJX6kQm4182DStfK+NyVLcRrBfaamX3gqjwl1lHleZOQAuFNl4oRFSyKFHwwb5k7mC4XrRYLWsXKEKR0JiFGOSNxJ0ssEjbIPLvYmWhf7RZgwH9TtP12NEKcwJt4pQNxNsxP6j/hebQxLYTMzBdk2yn+ALvEpT//kUi5+kIUJ32qfB4S6n0fkRGpkJbLMYaMgCgGwlktpLdqVwJSJm3Z8FK3zcQxS5gILEdlCnjm89w4Y66sczvBPH3iUyMLE91S1cWG/3aO9kNT2XuYaMgGJU1zgpqjgARkRu/BA4UNhtZGiJtyzjnz2iGXjswkH2ENqX32MH3FsCFgd5+P8ei7VyrQP3hD8SChpF4woICPzzlkjKdaimgIPNEY3S1pkj/IdSeldIU+8c1N+4vI= X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:SJ0PR11MB5005.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(346002)(366004)(39860400002)(376002)(136003)(396003)(921005)(38100700002)(478600001)(30864003)(5660300002)(6666004)(8676002)(36756003)(83380400001)(7366002)(1191002)(7416002)(6486002)(7406005)(53546011)(2616005)(8936002)(16576012)(66476007)(26005)(4326008)(186003)(316002)(31686004)(31696002)(66556008)(2906002)(44832011)(110136005)(66946007)(956004)(86362001)(45980500001); DIR:OUT; SFP:1102; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?b09YYlIzNnhrTDhtd2pXRE41dy9jbkRDbElLZ29hQmNtNkJ4NGp3R2ZjRFE0?= =?utf-8?B?SmlZci9FUjFxaUE5T3RKcGgzWHVlK0hkNFZWaEpYMlAxcFFhU042OTVwaUdh?= =?utf-8?B?QTA1Y2s2MUcwbHZyQWFFd2Qyb0gwWnJ0Y1FFY1lQNmFRQnlHRnB3T1VRSUJ2?= =?utf-8?B?WFNsSGZ6T0luQ0JzdzdDS3NWTmk2Uk1odlVaMkJwM2s5QWV6SitKWjdBcEpO?= =?utf-8?B?SEl6dXZnSzZja2wzSyticEhVYTJ1cDdNRDhIVVRnMS9kOWx5N09XYStibStN?= =?utf-8?B?Smw2a2o3bVJKcVg1c1VOcWFFMnQzWU5ESlMwQUEyYlpmeXB2VUtSNWpqcmVw?= =?utf-8?B?ZldwNVNIdmltemkzTmxqWFBNYmZRdWpVVG9VdVN4K2FsT0N4bTZ5SlVGM253?= =?utf-8?B?WFgwaDZneURmNjFDK2VvbWJVdXNUMWhVRHlxSkpDRXVBRzRkRHpXL0xSYlpp?= =?utf-8?B?YkUyUWo3amVUWHZmR2s0TFZVQnYrMW5Rb3lFcWdlSWJVZ0Z1dkw3SEhuQUpi?= =?utf-8?B?eXRCVWQwT2JFRkwzWkRkaDU2UmtveTRJRFNyb0RmSGhzYmFENXNSMldpMUpR?= =?utf-8?B?Q3JqRGZWRVJaTGR5S0hIY1d3aWFZSFpCM1pUV2xCeEFZSEJ2ZThRN3pUQksv?= =?utf-8?B?ZUs4Tm04cXR6dlFqYjlZQ21ZcDJ6V2phM01lQy9zZEFtajc3Mytad2V3MHZP?= =?utf-8?B?RGl1M3B5dmdKYlo5SXZuR0NvZHlLSTY5dkxOb1dZODVhN2dnM3d2TjhZYlJ1?= =?utf-8?B?SGM1QXRRbDdxVEhuOXROYm5JR2dpUndWcENXcG9zbFUzMTdMcnhnWmROaXEy?= =?utf-8?B?cVRDNVc0THlQYzIwUmgycy9tclZNaFpJdFhNeEJFMTUvQzVMcS9sTFA3Q2Z5?= =?utf-8?B?N2ZiNi9TWVJjZ0dUbWQvZ0VuMldCOVFmZEx6dWUvclJWUTlldVBVTEtUVUlJ?= =?utf-8?B?SFI4R3FqWVZBdUJOR1RnWWp1L1E5TmttMy9rTnF0RDlCOVFnc0JSRnhyRCtW?= =?utf-8?B?UWk1LzFqYm0wQm5hcmVvcnlnelNqNjVYRGdCeHMxbkVlcVhpWmJ1K2JCMTU0?= =?utf-8?B?dmZZVVh6VHU3NG11UTVrQklnRjRHYmxKTmdMS3VjK0ZCTW1GQXZWUW9IMW1y?= =?utf-8?B?b3BXWVBhMjdrOWtEd1RHOUhYSHlYUjZOVXN3V1gzMXFFazNYT3hjQWhIblBr?= =?utf-8?B?Ym13V2MvUnVGektkbEJlL3lWMjdMN3VXRHM5TUxUMUs2dkZoZXF6M28zT1Ix?= =?utf-8?B?UkpTUDdKeVdjTnViZFFqZXZyOFRncFBZSnBqR2lZUXl5Rk1MZC9Fa0UyZnJ0?= =?utf-8?B?KzJCbjQrT083SjdNTHFYVVhOZ2VKSFJtbnFsc0NZZnl1anpOTGdJcXQ5QUla?= =?utf-8?B?Zm1iU05UdkVJZlNENjBsZ3VHYUk1S29vdWxtdWNWR1NCTHZQRzd6ZzkzcitE?= =?utf-8?B?R0ZDNXl3Ukt2UXV1NkRVK2FrMkw0aEg3VXJuc2lqdUJ6TVo4c2EyMk1PYTdo?= =?utf-8?B?dTNFcWFraEhhQlRpV2hnMVEvcVRqUWZ3blQrYmNEeXZoeUthYldxRFpObDJ0?= =?utf-8?B?M1Y4NEtIQVRFb3Npc0p0Y05mZjRxRmR4S29BekxFajcwWTU2UkloS0piRlQ0?= =?utf-8?B?N3F0NVl1T3JGbFRiNDFtVU5QSkJ5OERpREY2VUtDTVRDWnNkQTZPeUp3Wmpv?= =?utf-8?B?T1BKeFNOR3dTY2w3cDcycktZUEduTHR3YW1xakIrVjd0L0tuaHBMSjNkSGlx?= =?utf-8?Q?YMhyl2tP9kYKCDdjzDze0/P7X9WhdJL5dBkQkNx?= X-MS-Exchange-CrossTenant-Network-Message-Id: 7165fcea-2b43-4a5b-df20-08d94c67305d X-MS-Exchange-CrossTenant-AuthSource: SJ0PR11MB5005.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Jul 2021 16:47:13.0050 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: sg0wUkNEiSpvIdD7z/sRN5+YylMVKZtBUnkfrtY/I/BkXPuiQyxmMTnnHeBq9Ms1Pl9oz8vNxdNlYqpC2Lj+5A== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3606 X-OriginatorOrg: intel.com Subject: Re: [dpdk-dev] [PATCH 1/4] ethdev: fix max Rx packet length 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 7/13/2021 1:47 PM, Andrew Rybchenko wrote: > On 7/9/21 8:29 PM, Ferruh Yigit wrote: >> There is a confusion on setting max Rx packet length, this patch aims to >> clarify it. >> >> 'rte_eth_dev_configure()' API accepts max Rx packet size via >> 'uint32_t max_rx_pkt_len' filed of the config struct 'struct >> rte_eth_conf'. >> >> Also 'rte_eth_dev_set_mtu()' API can be used to set the MTU, and result >> stored into '(struct rte_eth_dev)->data->mtu'. >> >> These two APIs are related but they work in a disconnected way, they >> store the set values in different variables which makes hard to figure >> out which one to use, also two different related method is confusing for >> the users. >> >> Other issues causing confusion is: >> * maximum transmission unit (MTU) is payload of the Ethernet frame. And >> 'max_rx_pkt_len' is the size of the Ethernet frame. Difference is >> Ethernet frame overhead, but this may be different from device to >> device based on what device supports, like VLAN and QinQ. >> * 'max_rx_pkt_len' is only valid when application requested jumbo frame, >> which adds additional confusion and some APIs and PMDs already >> discards this documented behavior. >> * For the jumbo frame enabled case, 'max_rx_pkt_len' is an mandatory >> field, this adds configuration complexity for application. >> >> As solution, both APIs gets MTU as parameter, and both saves the result >> in same variable '(struct rte_eth_dev)->data->mtu'. For this >> 'max_rx_pkt_len' updated as 'mtu', and it is always valid independent >> from jumbo frame. >> >> For 'rte_eth_dev_configure()', 'dev->data->dev_conf.rxmode.mtu' is user >> request and it should be used only within configure function and result >> should be stored to '(struct rte_eth_dev)->data->mtu'. After that point >> both application and PMD uses MTU from this variable. >> >> When application doesn't provide an MTU during 'rte_eth_dev_configure()' >> default 'RTE_ETHER_MTU' value is used. >> >> As additional clarification, MTU is used to configure the device for >> physical Rx/Tx limitation. Other related issue is size of the buffer to >> store Rx packets, many PMDs use mbuf data buffer size as Rx buffer size. >> And compares MTU against Rx buffer size to decide enabling scattered Rx >> or not, if PMD supports it. If scattered Rx is not supported by device, >> MTU bigger than Rx buffer size should fail. >> > > Do I understand correctly that target is 21.11? > Yes, it is for 21.11, I should clarify it. > Really huge work. Many thanks. > > See my notes below. > >> Signed-off-by: Ferruh Yigit > > [snip] > >> diff --git a/app/test-eventdev/test_pipeline_common.c b/app/test-eventdev/test_pipeline_common.c >> index 6ee530d4cdc9..5fcea74b4d43 100644 >> --- a/app/test-eventdev/test_pipeline_common.c >> +++ b/app/test-eventdev/test_pipeline_common.c >> @@ -197,8 +197,9 @@ pipeline_ethdev_setup(struct evt_test *test, struct evt_options *opt) >> return -EINVAL; >> } >> >> - port_conf.rxmode.max_rx_pkt_len = opt->max_pkt_sz; >> - if (opt->max_pkt_sz > RTE_ETHER_MAX_LEN) >> + port_conf.rxmode.mtu = opt->max_pkt_sz - RTE_ETHER_HDR_LEN - >> + RTE_ETHER_CRC_LEN; > > Subtract requires overflow check. May max_pkt_size be 0 or just > smaller that RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN? > There is a "opt->max_pkt_sz < RTE_ETHER_MIN_LEN" check above this, which ensures it won't overflow. >> + if (port_conf.rxmode.mtu > RTE_ETHER_MTU) >> port_conf.rxmode.offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME; >> >> t->internal_port = 1; >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c >> index 8468018cf35d..8bdc042f6e8e 100644 >> --- a/app/test-pmd/cmdline.c >> +++ b/app/test-pmd/cmdline.c >> @@ -1892,43 +1892,36 @@ cmd_config_max_pkt_len_parsed(void *parsed_result, >> __rte_unused void *data) >> { >> struct cmd_config_max_pkt_len_result *res = parsed_result; >> - uint32_t max_rx_pkt_len_backup = 0; >> - portid_t pid; >> + portid_t port_id; >> int ret; >> >> + if (strcmp(res->name, "max-pkt-len")) { >> + printf("Unknown parameter\n"); >> + return; >> + } >> + >> if (!all_ports_stopped()) { >> printf("Please stop all ports first\n"); >> return; >> } >> >> - RTE_ETH_FOREACH_DEV(pid) { >> - struct rte_port *port = &ports[pid]; >> - >> - if (!strcmp(res->name, "max-pkt-len")) { >> - if (res->value < RTE_ETHER_MIN_LEN) { >> - printf("max-pkt-len can not be less than %d\n", >> - RTE_ETHER_MIN_LEN); >> - return; >> - } >> - if (res->value == port->dev_conf.rxmode.max_rx_pkt_len) >> - return; >> - >> - ret = eth_dev_info_get_print_err(pid, &port->dev_info); >> - if (ret != 0) { >> - printf("rte_eth_dev_info_get() failed for port %u\n", >> - pid); >> - return; >> - } >> + RTE_ETH_FOREACH_DEV(port_id) { >> + struct rte_port *port = &ports[port_id]; >> >> - max_rx_pkt_len_backup = port->dev_conf.rxmode.max_rx_pkt_len; >> + if (res->value < RTE_ETHER_MIN_LEN) { >> + printf("max-pkt-len can not be less than %d\n", > > fprintf() to stderr, please. > Here and in a number of places below. > Overall I agree, but not sure to have this change in this patch. The patch is already complex, I am for keeping logging part same as before, what do you think to update all usage later on its own patch? >> + RTE_ETHER_MIN_LEN); >> + return; >> + } >> >> - port->dev_conf.rxmode.max_rx_pkt_len = res->value; >> - if (update_jumbo_frame_offload(pid) != 0) >> - port->dev_conf.rxmode.max_rx_pkt_len = max_rx_pkt_len_backup; >> - } else { >> - printf("Unknown parameter\n"); >> + ret = eth_dev_info_get_print_err(port_id, &port->dev_info); >> + if (ret != 0) { >> + printf("rte_eth_dev_info_get() failed for port %u\n", >> + port_id); >> return; >> } >> + >> + update_jumbo_frame_offload(port_id, res->value); >> } >> >> init_port_config(); >> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c >> index 04ae0feb5852..a87265d7638b 100644 >> --- a/app/test-pmd/config.c >> +++ b/app/test-pmd/config.c > > [snip] > >> @@ -1155,20 +1154,17 @@ port_mtu_set(portid_t port_id, uint16_t mtu) >> return; >> } >> diag = rte_eth_dev_set_mtu(port_id, mtu); >> - if (diag) >> + if (diag) { >> printf("Set MTU failed. diag=%d\n", diag); >> - else if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) { >> - /* >> - * Ether overhead in driver is equal to the difference of >> - * max_rx_pktlen and max_mtu in rte_eth_dev_info when the >> - * device supports jumbo frame. >> - */ >> - eth_overhead = dev_info.max_rx_pktlen - dev_info.max_mtu; >> + return; >> + } >> + >> + rte_port->dev_conf.rxmode.mtu = mtu; >> + >> + if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) { >> if (mtu > RTE_ETHER_MTU) { >> rte_port->dev_conf.rxmode.offloads |= >> DEV_RX_OFFLOAD_JUMBO_FRAME; >> - rte_port->dev_conf.rxmode.max_rx_pkt_len = >> - mtu + eth_overhead; >> } else > > I guess curly brackets should be removed now. > ack >> rte_port->dev_conf.rxmode.offloads &= >> ~DEV_RX_OFFLOAD_JUMBO_FRAME; > > [snip] > >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c >> index 1cdd3cdd12b6..2c79cae05664 100644 >> --- a/app/test-pmd/testpmd.c >> +++ b/app/test-pmd/testpmd.c > > [snip] > >> @@ -1465,7 +1473,7 @@ init_config(void) >> rte_exit(EXIT_FAILURE, >> "rte_eth_dev_info_get() failed\n"); >> >> - ret = update_jumbo_frame_offload(pid); >> + ret = update_jumbo_frame_offload(pid, 0); >> if (ret != 0) >> printf("Updating jumbo frame offload failed for port %u\n", >> pid); >> @@ -1512,14 +1520,19 @@ init_config(void) >> */ >> if (port->dev_info.rx_desc_lim.nb_mtu_seg_max != UINT16_MAX && >> port->dev_info.rx_desc_lim.nb_mtu_seg_max != 0) { >> - data_size = rx_mode.max_rx_pkt_len / >> - port->dev_info.rx_desc_lim.nb_mtu_seg_max; >> + uint32_t eth_overhead = get_eth_overhead(&port->dev_info); >> + uint16_t mtu; >> >> - if ((data_size + RTE_PKTMBUF_HEADROOM) > >> + if (rte_eth_dev_get_mtu(pid, &mtu) == 0) { >> + data_size = mtu + eth_overhead / >> + port->dev_info.rx_desc_lim.nb_mtu_seg_max; >> + >> + if ((data_size + RTE_PKTMBUF_HEADROOM) > > > Unnecessary parenthesis. > This part already changed in upstream. >> mbuf_data_size[0]) { >> - mbuf_data_size[0] = data_size + >> - RTE_PKTMBUF_HEADROOM; >> - warning = 1; >> + mbuf_data_size[0] = data_size + >> + RTE_PKTMBUF_HEADROOM; >> + warning = 1; >> + } >> } >> } >> } > > [snip] > >> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c >> index c515de3bf71d..0a8d29277aeb 100644 >> --- a/drivers/net/tap/rte_eth_tap.c >> +++ b/drivers/net/tap/rte_eth_tap.c >> @@ -1627,13 +1627,8 @@ tap_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) >> { >> struct pmd_internals *pmd = dev->data->dev_private; >> struct ifreq ifr = { .ifr_mtu = mtu }; >> - int err = 0; >> >> - err = tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE); >> - if (!err) >> - dev->data->mtu = mtu; >> - >> - return err; >> + return tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE); > > The cleanup could be done separately before the patch, since > it just makes the long patch longer and unrelated in fact, > since assignment after callback is already done. > Yes, and agree it can be updated seperately, but I think change is related, not sure about having too many commits. If you have strong opinion I can update it. >> } >> >> static int > > [snip] > >> diff --git a/examples/ip_fragmentation/main.c b/examples/ip_fragmentation/main.c >> index 77a6a18d1914..f97287ce2243 100644 >> --- a/examples/ip_fragmentation/main.c >> +++ b/examples/ip_fragmentation/main.c >> @@ -146,7 +146,7 @@ struct lcore_queue_conf lcore_queue_conf[RTE_MAX_LCORE]; >> >> static struct rte_eth_conf port_conf = { >> .rxmode = { >> - .max_rx_pkt_len = JUMBO_FRAME_MAX_SIZE, >> + .mtu = JUMBO_FRAME_MAX_SIZE, > > Before the patch JUMBO_FRAME_MAX_SIZE inluded overhad, but > after the patch it is used as it is does not include overhead. > > There a number of similiar cases in other apps. > ack, Huisong also highlighted it, I will update. >> .split_hdr_size = 0, >> .offloads = (DEV_RX_OFFLOAD_CHECKSUM | >> DEV_RX_OFFLOAD_SCATTER | > > [snip] > >> diff --git a/examples/ip_pipeline/link.c b/examples/ip_pipeline/link.c >> index 16bcffe356bc..8628db22f56b 100644 >> --- a/examples/ip_pipeline/link.c >> +++ b/examples/ip_pipeline/link.c >> @@ -46,7 +46,7 @@ static struct rte_eth_conf port_conf_default = { >> .link_speeds = 0, >> .rxmode = { >> .mq_mode = ETH_MQ_RX_NONE, >> - .max_rx_pkt_len = 9000, /* Jumbo frame max packet len */ >> + .mtu = 9000, /* Jumbo frame MTU */ > > Strictly speaking 9000 included overhead before the patch and > does not include overhead after the patch. > > There a number of similiar cases in other apps. > ack >> .split_hdr_size = 0, /* Header split buffer size */ >> }, >> .rx_adv_conf = { > > [snip] > >> diff --git a/examples/l3fwd-acl/main.c b/examples/l3fwd-acl/main.c >> index a1f457b564b6..913037d5f835 100644 >> --- a/examples/l3fwd-acl/main.c >> +++ b/examples/l3fwd-acl/main.c > > [snip] > >> @@ -1833,12 +1832,12 @@ parse_args(int argc, char **argv) >> print_usage(prgname); >> return -1; >> } >> - port_conf.rxmode.max_rx_pkt_len = ret; >> + port_conf.rxmode.mtu = ret - (RTE_ETHER_HDR_LEN + >> + RTE_ETHER_CRC_LEN); >> } >> - printf("set jumbo frame max packet length " >> - "to %u\n", >> - (unsigned int) >> - port_conf.rxmode.max_rx_pkt_len); >> + printf("set jumbo frame max packet length to %u\n", >> + (unsigned int)port_conf.rxmode.mtu + >> + RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN); > > > I think that overhead should be obtainded from dev_info with > fallback to value used above. > Right, but since at this stage it is hard to get the overhead for the application, I wonder if we should change the applications to get the MTU as paramter. And overall this work makes it harder for application to use frame size, since it brings 'rte_eth_dev_info_get()' API call requirement. Not sure how big a problem is this, and if we can provide some help to applications, any suggestion is welcome. Specific to above sample app, it can be possible to record user parameter in a temp var, and set the 'port_conf.rxmode.max_rx_pkt_len' when app has the dev_info, I will update it. > There are many similar cases in other apps. > ack > [snip] > >> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c >> index c607eabb5b0c..3451125639f9 100644 >> --- a/lib/ethdev/rte_ethdev.c >> +++ b/lib/ethdev/rte_ethdev.c >> @@ -1249,15 +1249,15 @@ rte_eth_dev_tx_offload_name(uint64_t offload) >> >> static inline int >> eth_dev_check_lro_pkt_size(uint16_t port_id, uint32_t config_size, >> - uint32_t max_rx_pkt_len, uint32_t dev_info_size) >> + uint32_t max_rx_pktlen, uint32_t dev_info_size) >> { >> int ret = 0; >> >> if (dev_info_size == 0) { >> - if (config_size != max_rx_pkt_len) { >> + if (config_size != max_rx_pktlen) { >> RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%d max_lro_pkt_size" >> " %u != %u is not allowed\n", >> - port_id, config_size, max_rx_pkt_len); >> + port_id, config_size, max_rx_pktlen); > > This patch looks a bit unrelated and make the long patch > even more longer. May be it is better to do the cleanup > first (before the patch). > I also had same doubt, but it is somehow related. Previously the variable name in the two struct was slightly different: dev_info.max_rx_pktlen => max Rx packet lenght device capability rxmode.max_rx_pkt_len => max Rx packet length configuration This slight difference was bothering me :), since we are removing 'rxmode.max_rx_pkt_len' now, I though it is good opportunity to unifiy variabe name to 'max_rx_pktlen'. After this patch only avp driver has 'max_rx_pkt_len' usage (because of usage on its interface). I am not sure if above change worth to have its own patch, another option is discard this change, if you have strong opinion on it I can drop the changes. >> ret = -EINVAL; >> } >> } else if (config_size > dev_info_size) { >> @@ -1325,6 +1325,19 @@ eth_dev_validate_offloads(uint16_t port_id, uint64_t req_offloads, >> return ret; >> } >> >> +static uint16_t >> +eth_dev_get_overhead_len(uint32_t max_rx_pktlen, uint16_t max_mtu) >> +{ >> + uint16_t overhead_len; >> + >> + if (max_mtu != UINT16_MAX && max_rx_pktlen > max_mtu) >> + overhead_len = max_rx_pktlen - max_mtu; >> + else >> + overhead_len = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN; >> + >> + return overhead_len; >> +} >> + >> int >> rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, >> const struct rte_eth_conf *dev_conf) >> @@ -1332,6 +1345,7 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, >> struct rte_eth_dev *dev; >> struct rte_eth_dev_info dev_info; >> struct rte_eth_conf orig_conf; >> + uint32_t max_rx_pktlen; >> uint16_t overhead_len; >> int diag; >> int ret; >> @@ -1375,11 +1389,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, >> goto rollback; >> >> /* Get the real Ethernet overhead length */ >> - if (dev_info.max_mtu != UINT16_MAX && >> - dev_info.max_rx_pktlen > dev_info.max_mtu) >> - overhead_len = dev_info.max_rx_pktlen - dev_info.max_mtu; >> - else >> - overhead_len = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN; >> + overhead_len = eth_dev_get_overhead_len(dev_info.max_rx_pktlen, >> + dev_info.max_mtu); >> >> /* If number of queues specified by application for both Rx and Tx is >> * zero, use driver preferred values. This cannot be done individually >> @@ -1448,49 +1459,45 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, >> } >> >> /* >> - * If jumbo frames are enabled, check that the maximum RX packet >> - * length is supported by the configured device. >> + * Check that the maximum RX packet length is supported by the >> + * configured device. >> */ >> - if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) { >> - if (dev_conf->rxmode.max_rx_pkt_len > dev_info.max_rx_pktlen) { >> - RTE_ETHDEV_LOG(ERR, >> - "Ethdev port_id=%u max_rx_pkt_len %u > max valid value %u\n", >> - port_id, dev_conf->rxmode.max_rx_pkt_len, >> - dev_info.max_rx_pktlen); >> - ret = -EINVAL; >> - goto rollback; >> - } else if (dev_conf->rxmode.max_rx_pkt_len < RTE_ETHER_MIN_LEN) { >> - RTE_ETHDEV_LOG(ERR, >> - "Ethdev port_id=%u max_rx_pkt_len %u < min valid value %u\n", >> - port_id, dev_conf->rxmode.max_rx_pkt_len, >> - (unsigned int)RTE_ETHER_MIN_LEN); >> - ret = -EINVAL; >> - goto rollback; >> - } >> + if (dev_conf->rxmode.mtu == 0) >> + dev->data->dev_conf.rxmode.mtu = RTE_ETHER_MTU; >> + max_rx_pktlen = dev->data->dev_conf.rxmode.mtu + overhead_len; >> + if (max_rx_pktlen > dev_info.max_rx_pktlen) { >> + RTE_ETHDEV_LOG(ERR, >> + "Ethdev port_id=%u max_rx_pktlen %u > max valid value %u\n", >> + port_id, max_rx_pktlen, dev_info.max_rx_pktlen); >> + ret = -EINVAL; >> + goto rollback; >> + } else if (max_rx_pktlen < RTE_ETHER_MIN_LEN) { >> + RTE_ETHDEV_LOG(ERR, >> + "Ethdev port_id=%u max_rx_pktlen %u < min valid value %u\n", >> + port_id, max_rx_pktlen, RTE_ETHER_MIN_LEN); >> + ret = -EINVAL; >> + goto rollback; >> + } >> >> - /* Scale the MTU size to adapt max_rx_pkt_len */ >> - dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len - >> - overhead_len; >> - } else { >> - uint16_t pktlen = dev_conf->rxmode.max_rx_pkt_len; >> - if (pktlen < RTE_ETHER_MIN_MTU + overhead_len || >> - pktlen > RTE_ETHER_MTU + overhead_len) >> + if ((dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) == 0) { >> + if (dev->data->dev_conf.rxmode.mtu < RTE_ETHER_MIN_MTU || >> + dev->data->dev_conf.rxmode.mtu > RTE_ETHER_MTU) >> /* Use default value */ >> - dev->data->dev_conf.rxmode.max_rx_pkt_len = >> - RTE_ETHER_MTU + overhead_len; >> + dev->data->dev_conf.rxmode.mtu = RTE_ETHER_MTU; > > I don't understand it. It would be good to add comments to > explain logic above. > This part will be updated in next patches, and I will extract the checks to a common function, can you please check the final out output of next patch, if it makes sense? >> } >> >> + dev->data->mtu = dev->data->dev_conf.rxmode.mtu; >> + >> /* >> * If LRO is enabled, check that the maximum aggregated packet >> * size is supported by the configured device. >> */ >> if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_TCP_LRO) { >> if (dev_conf->rxmode.max_lro_pkt_size == 0) >> - dev->data->dev_conf.rxmode.max_lro_pkt_size = >> - dev->data->dev_conf.rxmode.max_rx_pkt_len; >> + dev->data->dev_conf.rxmode.max_lro_pkt_size = max_rx_pktlen; >> ret = eth_dev_check_lro_pkt_size(port_id, >> dev->data->dev_conf.rxmode.max_lro_pkt_size, >> - dev->data->dev_conf.rxmode.max_rx_pkt_len, >> + max_rx_pktlen, >> dev_info.max_lro_pkt_size); >> if (ret != 0) >> goto rollback; > > [snip] > >> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h >> index faf3bd901d75..9f288f98329c 100644 >> --- a/lib/ethdev/rte_ethdev.h >> +++ b/lib/ethdev/rte_ethdev.h >> @@ -410,7 +410,7 @@ enum rte_eth_tx_mq_mode { >> struct rte_eth_rxmode { >> /** The multi-queue packet distribution mode to be used, e.g. RSS. */ >> enum rte_eth_rx_mq_mode mq_mode; >> - uint32_t max_rx_pkt_len; /**< Only used if JUMBO_FRAME enabled. */ >> + uint32_t mtu; /**< Requested MTU. */ > > Maximum Transmit Unit looks a bit confusing in Rx mode > structure. > True, but I think it is already used for Rx already as concept, I believe the intention will be clear enough. Do you think will be more clear if we pick a DPDK specific variable name? >> /** Maximum allowed size of LRO aggregated packet. */ >> uint32_t max_lro_pkt_size; >> uint16_t split_hdr_size; /**< hdr buf size (header_split enabled).*/ > > [snip] >