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 0329BA055A; Mon, 21 Nov 2022 11:08:23 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9F3EB40150; Mon, 21 Nov 2022 11:08:23 +0100 (CET) Received: from NAM11-DM6-obe.outbound.protection.outlook.com (mail-dm6nam11on2040.outbound.protection.outlook.com [40.107.223.40]) by mails.dpdk.org (Postfix) with ESMTP id 349384014F for ; Mon, 21 Nov 2022 11:08:22 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=H9A5uGUOSGopSgzfZ6WQ3aIp3PmVBh2EX0FL9k68MvJcZ1tml8hsIKw/Z/PTQWsy/CwWfyqx/3RiJs095h3uQttqwnx9cRobTS6bHCjVdXHhsYci838jRYvG06vaGP5CbPfrAogOuUpci4xL+f1Ou2E1xqXMq5QP1aR3NyJkF9Z1FrUmLmrlvEeNDULuHfifY33FoteSetDn8IAZxnEQACgO/jTShkC54W/4zO8Vgnl/235l5DoUF+SZS4Xi2Bfgdh0/KRwzfjGNill2AZowSqxtGj4CDZAjZVv3V2Oy8QTO7/AObW1cps9zpjoRcCtH0aKOQgB9W70/mDPi5A9meA== 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=UkhdHFDifCJ6Gkj9yXUKZERZarCFDN7hYJAzcxBxi14=; b=V5HLO8MZaHtzYU91n3+m7+mzRqVhUXdZAAj0HthdkxDNeuJwF3/ONyBfgaOc7qNoMsa6HEV7NUx/FQMRK+3Ah/2ON1LcyaffUKzfv9yJ8C7727qF+Ma62FQDxXJHMnYqeDDEaF3h1JkHWD8/xDD7YEGnQXUAQF/C3I0IH5vW/b4lX/VIJ0UNEakxqvXUNHV1VGmQmKXpGpvc1G/GXJ4ejISpF63X64EAshLKjQ/t0YTSPTsAbyRVT2QmdIq3TpBWGYZBkiZgYtSF7obv3vEKDRdaVIe//adH55O6KcZ5MeH+j4dYrDYHI2gigoqSoPWWIdOXAn2sF+daNxoYZKLimg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=UkhdHFDifCJ6Gkj9yXUKZERZarCFDN7hYJAzcxBxi14=; b=xapGuRzFFwyps0lsZYwgmZIh+PT9oJfc4gEO1YhT0bzCM/qsl0GFEdg3LyOa5a5wDZ/w+yWaGdba+gwHlqcjjtO9//zOHJpEeNKBcnNk5F4EG/vWtHsTQaaW2CmB/pwnSPkXyyKNB7GEGMub5Pc3E4Q8ug1y0GTvUp6YmUqRdW8= Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com; Received: from CH2PR12MB4294.namprd12.prod.outlook.com (2603:10b6:610:a9::11) by MN2PR12MB4206.namprd12.prod.outlook.com (2603:10b6:208:1d5::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5834.15; Mon, 21 Nov 2022 10:08:19 +0000 Received: from CH2PR12MB4294.namprd12.prod.outlook.com ([fe80::b482:d5bd:c7d0:3842]) by CH2PR12MB4294.namprd12.prod.outlook.com ([fe80::b482:d5bd:c7d0:3842%9]) with mapi id 15.20.5834.015; Mon, 21 Nov 2022 10:08:19 +0000 Message-ID: <31f1f1b3-e04d-4a99-64a7-d1d69004752f@amd.com> Date: Mon, 21 Nov 2022 10:08:12 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 To: Hanumanth Reddy Pothula , "thomas@monjalon.net" , "andrew.rybchenko@oktetlabs.ru" , Nithin Kumar Dabilpuram Cc: "dev@dpdk.org" , "yux.jiang@intel.com" , Jerin Jacob Kollanukkaran , Aman Singh , Yuying Zhang References: <20221118111358.3563760-1-hpothula@marvell.com> <20221118141334.3825072-1-hpothula@marvell.com> Content-Language: en-US From: Ferruh Yigit Subject: Re: [EXT] Re: [PATCH v4 1/1] app/testpmd: add valid check to verify multi mempool feature In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-ClientProxiedBy: LO6P265CA0015.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:339::8) To CH2PR12MB4294.namprd12.prod.outlook.com (2603:10b6:610:a9::11) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: CH2PR12MB4294:EE_|MN2PR12MB4206:EE_ X-MS-Office365-Filtering-Correlation-Id: d2928c4e-ca66-43b0-a219-08dacba84fc5 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: c8rdro3ZQuJFBaPC2j+veB4nJKAYkc5VekIi9s+ZnnNtPMi97NqvUc1CkO0VEC7oTCKMNbalczOnmOFx7lsWEwvOKIIMcBJgA4uM+bh74/0YDJjRmXRhhR/5wSxkrUBfdlUftKPlSDG1/UvrzubgKvuUa/KkZFOT8T56tKeCDAwUUsorILhLrAhm4/5pVXif7yAGl6aVJvOQCIQEkgAjV7iGNbnbqRDeNxtzKKHFg877eJR7LdR3un2arcDZNOemv7G6Tws0xtSmae6TZ+0ypzz6etsOMCtL7+ZUYUQHlIgMsz8DaADIfJuR5YavcxdvoeGjykbczQu6lj0pOPD2igRJgoOJffGCqBUtI6wIS5Br+x3ck4yFZYT6VMIa2DUUXEQYnvbpNZDRmjXLPPosmyWaRUekU3+FReu0DpVkCqZVXH/MQ/WqAtTU54lh7n5Cu+I6SNMHJsKsCBVysJ7wkNrsUNIIGG9IA7EzVY3ixVnY0ihVwW+Lcy9KAYvsOcc/yr/2t3G1b5Xhva1MQIoqEsBJ/yu2UVBmCkN021Nm5SpqIp1I94EXSm3fiUu5fjASQbvo4cg8Os+FjSbkhYb/0+4ykA8JNt+tRJWaoUGdkSmy0lWNpBaGIrVSC+sDeRG2dC1Tk0picq5jhUQeM1nq1pyapPFUdW1+IUofaR21om9Hzq59JLhklOg9axqNZ09ahoVZsoO2ocGoGfrEdOlx7xlxhNSj64BXBJDXT37nQIA= X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:CH2PR12MB4294.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230022)(4636009)(366004)(396003)(376002)(346002)(39860400002)(136003)(451199015)(31686004)(2906002)(83380400001)(38100700002)(478600001)(86362001)(31696002)(186003)(6506007)(6486002)(316002)(44832011)(6512007)(8936002)(2616005)(5660300002)(53546011)(26005)(4326008)(66556008)(66476007)(66946007)(8676002)(36756003)(15650500001)(110136005)(54906003)(6666004)(41300700001)(45980500001)(43740500002); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NXpYNURxOHcwQUptTEZ2amN3VUNDV3J3eGNOeHhrQk1TVXZsM1hQNGpkMEk2?= =?utf-8?B?TzNBYXIzR3ZJUk5iU1FaZG04anB6dWRmalB6MXNzMEtNaTRTNDFYYTIvSVBV?= =?utf-8?B?Nm9GakJaVmtSSHYrUlpnM1VHZFgwSWZsbzViVHExVHQ2bkRqMmZTUjBCUEto?= =?utf-8?B?Wkc1cFVFUnBWdGdWSjZxTUFDSENnV0M5bWl5VUZYZVE1UndBbTFkM3lZM2J6?= =?utf-8?B?Uzd3Vko3eE9HRkZZWThNZkpJa3p2ZVlnMi81VWV3VG9OcUgvS0V4QURDdTlX?= =?utf-8?B?REVHM3F1S0htU2RPTEVtUG5HdWtENTlUVStwWlVJeEY0VVZ4SzVvY0NRZUJM?= =?utf-8?B?RTZZOVV6cmxINjBMdzdxZ1ljZnBySDMwWFVaNVhCNE5hdkw1WFlVMmFJa2cz?= =?utf-8?B?VTZxM2JjOGtDSk1aa3pPdXA0MVdqeVFkcWV1M3E5MWZqUFR1ZjM0bnh5bFZs?= =?utf-8?B?c3lIWS9IUHJ4M01iM2VtZWFtejZnRFR6MkgyNmR5cjN4MWdlanhIZFRPVUkr?= =?utf-8?B?dXJkdjJnTnhEbDFoN0NSVzRQbzBuQ0lJVGNHVktIcWhpb2xFd3hOWmtoTUdq?= =?utf-8?B?Z2FNVDlPZDdacFNSNm14Si9RYWZRTDU1Vk1Bd0E1cytveXpISnBTTWFoTnpI?= =?utf-8?B?UStwNUhHd25IcCtaVVYxbTRuWnRZZ2sxM1JVY2ZLbm4vV1drZ21sSkRUN3Bk?= =?utf-8?B?YnVIVkNuNFFUcW1MalpWWHBCYzNOaGdIVUYxY2JoOUFEMW5STXpSSGtqZmZU?= =?utf-8?B?REUxNHphS01JZ2ErMkZSWGpidUJ2ZVdIQnpXcWh0OUhadXFmbHoyT1FscUhB?= =?utf-8?B?WFRBTTR4YzVVTk14cHgrOWNrd0p3NmlvZTYxUUZnTjBmaWFIWW4yV3Q3Sy84?= =?utf-8?B?bHRYQmE2M1d6QkEySWdyb3NmbnkxbWY4TGZwajgyS3N3LzI5TCtpSDhmdFFt?= =?utf-8?B?azdBUVY5VVFsdE5jL1daNnZQUkxXUVNWTWRkd29tajgvektQZzZIUnRVTHFE?= =?utf-8?B?REhEL0dZdGMrSFgzOThjYXV0OVlsdzhURWdFVGlWN0YyNjViMEZid0hqUUFQ?= =?utf-8?B?citySXFwVzdubk1NMnZvQnVnWDVpVHJSMzlyeEptUEhwT0E5ZVAxNmcxZlJ4?= =?utf-8?B?TGFhSmRTbUFWVlV0dWhPbzdwVTNCeldsL1RhcEF0amJEWWl1OHBUS2xMMzBk?= =?utf-8?B?azlDRmZjYlBqa2Fjci9rYmhORmxJK1ZkUXlSeHpjWTRWTmVpek9vSjBEMVFa?= =?utf-8?B?MCtUUGc0OTZNRElzSVk3YzdiclUrb25hL2JTcjlOMEZ1eHE5SExtazcwazhE?= =?utf-8?B?bk5saHowYzYxTitmVzVwNzVQZjdZNXJVWDFFK2dUYWNmbkM5S0djVkNoekRD?= =?utf-8?B?VVp0MXVzRk85TTUwMVh4UFlZSk9VVEczLzNBSmNlWWJIdi94c2w0YUFTQUZ6?= =?utf-8?B?WSt2TlczY3lPakZJSTY3WEN0eGlpcEszakpyR2VPTkhEbnRHWVVxSmQyQUts?= =?utf-8?B?TXhTTFVLUnAyOUxML2Fab3YrQmhrckliKzJ3cEdrd0RBZHlBVUEreTZhTldV?= =?utf-8?B?QnRxUkRrRmZyT0NpTnoxVUt4dlVrN1h4UlJTaERabHU0dGhDUVRDTHpXYVZB?= =?utf-8?B?dmtDRnBVREhBYWtGS2lJZm10VS96bXlGRVFseVQ4a3ppSmVIN1NXY25ZSU1V?= =?utf-8?B?QUVaeFpEekRlbW9mWHJhZzRIbFFhcGkxN3hKYkFURXlMclhwSlZIV2tscWJy?= =?utf-8?B?dkRwdzhXU1NycXJpbWZSRGdUUHJnMGQ1bW45SHJIMFhLM2VEMkQ2aWZETU9n?= =?utf-8?B?eGpEWE5DamJpVXBLemVXdkc2Z3F5UWRrRVRoaEVEdzEybXp4ek5LWm11S3VE?= =?utf-8?B?ZEJna3hRdGhtczhvbGhqT3FTUVVjNjVUN0dscmVrL2FDWW40dkl0d1dWQm5u?= =?utf-8?B?SlMySUM1SDY2bUhwbG5LWEdiMnFYL2VaNmgrcWh5ZDh6WDFHMUFQaTIzZ0pw?= =?utf-8?B?YW40ODVSVjlFTDZwSVBMUWZKUUR3UVdEYVlIMFlmMVJDUmVjaXRCMEdYaWRQ?= =?utf-8?B?ME1FK2hQYy8zUDhtcGNhMjJONGlSOE5mNUs2SGFCWUpBRW1nK1VOQXFVS3Rk?= =?utf-8?Q?2Zu8hHlMEhIJJIv/8meVyOjJl?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: d2928c4e-ca66-43b0-a219-08dacba84fc5 X-MS-Exchange-CrossTenant-AuthSource: CH2PR12MB4294.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Nov 2022 10:08:19.5443 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: ES2Me3hOGCLMGNsIF/6tNSUZ4bq6L3Awf1ePwNKDeB3/0MwGFVSTfH4HRMvW3lJU X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4206 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 On 11/19/2022 12:00 AM, Hanumanth Reddy Pothula wrote: > > >> -----Original Message----- >> From: Ferruh Yigit >> Sent: Saturday, November 19, 2022 2:26 AM >> To: Hanumanth Reddy Pothula ; >> thomas@monjalon.net; andrew.rybchenko@oktetlabs.ru; Nithin Kumar >> Dabilpuram >> Cc: dev@dpdk.org; yux.jiang@intel.com; Jerin Jacob Kollanukkaran >> ; Aman Singh ; Yuying >> Zhang >> Subject: [EXT] Re: [PATCH v4 1/1] app/testpmd: add valid check to verify >> multi mempool feature >> >> External Email >> >> ---------------------------------------------------------------------- >> On 11/18/2022 2:13 PM, Hanumanth Pothula wrote: >>> Validate ethdev parameter 'max_rx_mempools' to know whether device >>> supports multi-mempool feature or not. >>> >> >> My preference would be revert the testpmd patch [1] that adds this new >> feature after -rc2, and add it back next release with new testpmd argument >> and below mentioned changes in setup function. >> >> @Andrew, @Thomas, @Jerin, what do you think? >> >> >> [1] >> 4f04edcda769 ("app/testpmd: support multiple mbuf pools per Rx queue") >> >>> Bugzilla ID: 1128 >>> >> >> Can you please add fixes line? >> > Ack >>> Signed-off-by: Hanumanth Pothula >> >> Please put the changelog after '---', which than git will take it as note. >> > Ack >>> v4: >>> - updated if condition. >>> v3: >>> - Simplified conditional check. >>> - Corrected spell, whether. >>> v2: >>> - Rebased on tip of next-net/main. >>> --- >>> app/test-pmd/testpmd.c | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index >>> 4e25f77c6a..c1b4dbd716 100644 >>> --- a/app/test-pmd/testpmd.c >>> +++ b/app/test-pmd/testpmd.c >>> @@ -2655,17 +2655,23 @@ rx_queue_setup(uint16_t port_id, uint16_t >> rx_queue_id, >>> union rte_eth_rxseg rx_useg[MAX_SEGS_BUFFER_SPLIT] = {}; >>> struct rte_mempool *rx_mempool[MAX_MEMPOOL] = {}; >>> struct rte_mempool *mpx; >>> + struct rte_eth_dev_info dev_info; >>> unsigned int i, mp_n; >>> uint32_t prev_hdrs = 0; >>> int ret; >>> >>> + ret = rte_eth_dev_info_get(port_id, &dev_info); >>> + if (ret != 0) >>> + return ret; >>> + >>> /* Verify Rx queue configuration is single pool and segment or >>> * multiple pool/segment. >>> + * @see rte_eth_dev_info::max_rx_mempools >>> * @see rte_eth_rxconf::rx_mempools >>> * @see rte_eth_rxconf::rx_seg >>> */ >>> - if (!(mbuf_data_size_n > 1) && !(rx_pkt_nb_segs > 1 || >>> - ((rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) != >> 0))) { >>> + if ((dev_info.max_rx_mempools == 0) && (rx_pkt_nb_segs <= 1 || >> >> Using `dev_info.max_rx_mempools` for check means if device supports >> multiple mempool, multiple mempool will be configured independent from >> user configuration. But user may prefer singe mempool or buffer split. >> > Please find my suggested logic. > >> Right now only PMD support multiple mempool is 'cnxk', so this doesn't >> impact others but I think this is not correct. >> >> Instead of re-using testpmd "mbuf-size" parameter (it is already used for >> two other features, and this is the reason of the defect) it would be better >> to have an explicit parameter for multiple mempool feature. >> >> >>> + ((rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) == >> 0))) { >>> /* Single pool/segment configuration */ >>> rx_conf->rx_seg = NULL; >>> rx_conf->rx_nseg = 0; >> >> >> Logic seems correct, although I have not tested. >> >> Current functions tries to detect the requested feature and setup queues >> accordingly, features are: >> - single mempool >> - packet split (to multiple mempool) >> - multiple mempool (various size) >> >> And the logic in the function is: >> `` >> if ( (! multiple mempool) && (! packet split)) >> setup for single mempool >> exit >> >> if (packet split) >> setup packet split >> else >> setup multiple mempool >> `` >> >> What do you think to >> a) simplify logic by making single mempool as fallback and last option, >> instead of detecting non existence of other configs >> b) have explicit check for multiple mempool >> >> Like: >> >> `` >> if (packet split) >> setup packet split >> exit >> else if (multiple mempool) >> setup multiple mempool >> exit >> >> setup for single mempool >> `` >> >> I think this both solves the defect and simplifies the code. > > Yes Ferruh your suggested logic simplifies the code. > > In the lines of your proposed logic, below if conditions might work fine for all features(buffer-split/multi-mempool) supported by PMD and user preference, > > if (rx_pkt_nb_segs > 1 || > rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) { > /*multi-segment (buffer split)*/ > } else if (mbuf_data_size_n > 1 && dev_info.max_rx_mempools > 1) { > /*multi-mempool*/ > } else { > /* single pool and segment */ > } > `mbuf_data_size_n > 1` may mean user is requesting multiple segment, or buffer split, so I am not sure about using this value to decide on multiple mempool feature, it can create side effect as Bug 1128 does. > Or adding new Rx offload parameter for multi_mempool feature, I think it might not be required, using dev_info.max_rx_mempools works fine. > In ethdev level, we don't need an offload flag, since separated config options clarify the intention there. What is needed is a way to understand users intention, for application (testpmd) and configure device accordingly. That is why I think 'dev_info.max_rx_mempools' is not working fine, because that is a way for device to say multiple mempool is supported, it is not to get user intention. In your logic when device supports multiple mempool, use it independent from what user request. I suggest having a testpmd argument explicitly to say multiple mempool feature is requested, this will help to distinguish buffer split, multiple mempool, single mempool features. Thanks. > if (rx_pkt_nb_segs > 1 || > rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) { > /*multi-segment (buffer split)*/ > } else if (mbuf_data_size_n > 1 && rx_conf->offloads & RTE_ETH_RX_OFFLOAD_MULTI_MEMPOOL ) { > /*multi-mempool*/ > } else { > /* single pool and segment */ > } > > Please let me know your inputs on above logic. >