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 1ADC741C97; Tue, 14 Feb 2023 19:16:29 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F2F9342F88; Tue, 14 Feb 2023 19:16:28 +0100 (CET) Received: from NAM02-DM3-obe.outbound.protection.outlook.com (mail-dm3nam02on2083.outbound.protection.outlook.com [40.107.95.83]) by mails.dpdk.org (Postfix) with ESMTP id 4F2F640EE4 for ; Tue, 14 Feb 2023 19:16:27 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YphDB8u4INwnvwbWxFTRAGjGgSfxDqZahSFifx5+IYcDhl7uWIwOq0blKG64rLnpl3rO7cWY2PPLWQ+u482rjPVBjDWajMMrqrc5rI0Npnf8fuLL48eKpy6pInoVV4q96LCujdBgPMWEihI5M7+vMMoEyCJMoPNzjQpB/bw+QsMVmTqr/6TeplkGlWUSBw1DhWCJ93cBB6WS5yz3xGVHeze1UfYVd1zzKpKyZ1dmqew7JgVn7mxrAK0OhxGa4ebHi6gHmlUv5qyVX3v2RrV3nrSsQ+RKvYDrCVAEV1niwbWRNS1fsMbWegwcVYEmO/Y9ZL7+BT16kN70r5O6jRNljA== 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=zDMSys+Fmda45G+mhaEgHkl0CIYeUMhth/n/nC8C4K4=; b=h47GBwTjlk7zkbflp9yzbPc4itFA+GG8SrvzXai5ZOL9vxloMRb+eDpoYdpfe0ILbsDJOx6dy43Ar2IODZHv3PAT0VB5kDKysXtjWVjysr+KjxfNNwpubOmIsjraphUnY878JJsMpG1qKuBX93byEju0mfXgcJdkD77QXk8tKGfGaaVJWl7uq3f1c1MIk/wfl7JFHbzmVHuY9nuyzwJywRagiTm+H2F9OHtpkB3tv2aVoNS90+cWbIh3WDw1Xyoz/e1o5CBW0rYXNDAzWTkQv+EeXxW1j9TpYPJShGyxwMFhm+jWw3VGCLP/uP3oxwC2bDmB3q8IvoeWV4oXvysJ3g== 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=zDMSys+Fmda45G+mhaEgHkl0CIYeUMhth/n/nC8C4K4=; b=r0DqmWp7/duvU9zBeP1ISthbC0qumAPph05bBxENsLrNmwGuHwGjOnDqrTL2NeOGHfqiVBuPINeNCRpszP+NSQohisLl8k8QYYr5B8HQUrUgsdPtL/VzzzZM3SB9JawTKuC9INKyckuXyzyG6OCFWMt8DU8LsCeOS/d6zcw7Me0= 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 CO6PR12MB5442.namprd12.prod.outlook.com (2603:10b6:5:35b::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6086.24; Tue, 14 Feb 2023 18:16:25 +0000 Received: from CH2PR12MB4294.namprd12.prod.outlook.com ([fe80::3614:22ed:ed5:5b48]) by CH2PR12MB4294.namprd12.prod.outlook.com ([fe80::3614:22ed:ed5:5b48%8]) with mapi id 15.20.6086.026; Tue, 14 Feb 2023 18:16:25 +0000 Message-ID: Date: Tue, 14 Feb 2023 18:16:19 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.7.2 Content-Language: en-US To: David Marchand , dev@dpdk.org Cc: Aman Singh , Yuying Zhang , Robin Jarry References: <20230124104742.1265439-1-david.marchand@redhat.com> <20230124104742.1265439-7-david.marchand@redhat.com> From: Ferruh Yigit Subject: Re: [PATCH 6/6] app/testpmd: factorize fwd engine Tx In-Reply-To: <20230124104742.1265439-7-david.marchand@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-ClientProxiedBy: LO4P123CA0667.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600:316::20) To CH2PR12MB4294.namprd12.prod.outlook.com (2603:10b6:610:a9::11) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: CH2PR12MB4294:EE_|CO6PR12MB5442:EE_ X-MS-Office365-Filtering-Correlation-Id: 2d0d6a91-309d-46bc-8d0f-08db0eb7951d X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: sO2yEYKcA0zOauTI1detWeUouOQ13Vnp9JMdTxDIfNzTixwQYKo5ivA+eKq8N9WUSxhxlk0uVmTIAp8jvAS4aymNsJ9GepDxiahabGIuR5NSwrvejOvw5P3pFm38ugL+32dZxd9Hm0EJSVuXKGAj9rYR3/FrB1YuDUNUrRGElDKbiHfft4VuE31S0GxC8i6XXFd9CB0K9QXtJv5O/Ol8NW0nQ/Px3j2qHicwZZDy53UInoZAYC71BjehJ/f7MlIDWGOsVlxkB5Y3prWSb2hO29jca7zEwBCPEwjaP1MRNEC//ptmuGjggOfA08XjglsKr6MH4oO99nM+8SLYoH05t5+C1DuGrqSjyTWtjjARu6XC9gBgi7wimGPyeK3EJOiUGba8G7cuQfJi+F8XAINjMYqgUu0FRXKmsKk7Lb2gyFV9Ehv5xsUgi30XgiJT7hGCuUgibshn2V2ZeYKgbqQDqHQ9vtLHxWCw4ILbAomOXlkP9LfHnoVaIN9WcaJ5TGhw9MRyMyP8MZY92TCNKCBzV/9BSMUjHtAs4iA3X2mhGnaHnlH6+aQowj+1pO3TzfSA3oDAJKWXOaAPi82BwOGRDTv8dh/z/IP9EWzT+ecZkth9nj+t9240B88HRz9QX6blXASxV6COms6gSrqgn1CR8cxm7OorIsdFF8Kt6cLb7FFuIQ6KaY4tyaX26TVs5ahhKphxZW5FmBJXoVyMRaqq2HCXm7ZSb3JRyStfu0+T28U= 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:(13230025)(4636009)(396003)(136003)(39860400002)(366004)(346002)(376002)(451199018)(44832011)(2906002)(5660300002)(4326008)(66476007)(54906003)(66556008)(66946007)(316002)(8676002)(6486002)(36756003)(6666004)(6506007)(53546011)(478600001)(31696002)(186003)(41300700001)(31686004)(8936002)(2616005)(86362001)(83380400001)(6512007)(26005)(38100700002)(43740500002)(45980500001); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UCtER25uZWpJSlRKWWdTVS9zMDlhV1ZBU1ZLMUxuamtNQ1ptaGcvb3hyQWFH?= =?utf-8?B?aEN6Q2xUUnFOWEFLd21zait5aUZleGE1b2F2ekZkMTNXT01lOGtyQU9tWkZj?= =?utf-8?B?TU83Nmd2WFppN3Y5ZHo4RisybW1nOFFId0h5cG8xSzl0OERMZys2eTVVNzhC?= =?utf-8?B?ZjNEQ1NmcU1lbytTdzh2bWo5OG43SGx4OUtGNHRjcjAwbk1NL3AwNHR4M2hT?= =?utf-8?B?dCt5V29MamRuTEY5elFZQmVQU256bzhSNTFoKzJIUWxoSHBmb2ozNUF1TlVH?= =?utf-8?B?Q1lmOW9yR0ttWVhLVzZKaGpvWlVkcDhpMHhwb2NnNlJDd2dOWi9CTFJOQVkx?= =?utf-8?B?NW84MkFnWUdRM3ZYSm1nN00vZEpadkQrZnV5TzJOUUdrb21sV3ZUTDRzazdB?= =?utf-8?B?U0IvanJ4UnpMOHl6VVlJNGk4TGxMOXNTaXVVOFJWeFpRTzBwUVgwUXlteGN1?= =?utf-8?B?NDBwby9EemlPR1NHNVRKRTRBNExwc3NUSUdPcXdyRVY2ZE5UUWQzSDlTMUVk?= =?utf-8?B?dWtaOFF2ZWVXc0QzUWludVFWWWxydUY5SSt3bE1oVEQ2MURDTHlKS1Bmdms2?= =?utf-8?B?Zi9sQU9LcU1YMlhmUnYzT2JqejBRSHYxMkNDYzg4YzN3cGVaU25lQXowcEZ1?= =?utf-8?B?SnVTNlhXODB2K0oyeUprekVOVzIxbmJJOXJpUTVidnRZVEhQQ1ZDTzJNWjdw?= =?utf-8?B?SytpRXFZWkFkUHM1dHNsM2pQbmFRZjEyRG1sN0VmWURySnlzY243UThXd01j?= =?utf-8?B?RHNBWG16MTBkN3FxZ2xxeis5eC9HUzg1b3BnUzJrUFd5ekx1Q2hoVDNZN1Vm?= =?utf-8?B?akt0S0ZhZjIwS0kzS3QvMk15YitIcVc1V1dyUUw0ZERFdmZxVkwzdE8rMTZR?= =?utf-8?B?dVpQdkM4dU14VXRSVkQwNlBKanI2RCtmei9Oc2pDZ3FENG9xaTBZeHNHTW41?= =?utf-8?B?aS96K2EzSUJIdjJ6MW5WNFdRQllUYUlIYWxGRnowd0RYZlFvWkx6ZmhERGxB?= =?utf-8?B?MFFKc2lKeXRnTTlVVU5LSFYxUTRDRlQ0bXg4NVpTNHkyditCUTRwN3hSRE1G?= =?utf-8?B?MGovdDhVUzZZdy9NZWRpOGtJL1gvTFZsNTg1blAxcDdEL1ZrbGFpRmFnNUtT?= =?utf-8?B?THpXODlsd3RNQTJoMXpSZTdGN3laSTZxWEdpVC9WSHRkLzJIc2M2OTBkaSta?= =?utf-8?B?NlFWMjNSZWIzVGpybER0WTlld3hxV1ZUTjVtQUJybDRCdWxpRTZHK0JSMUk0?= =?utf-8?B?NFlSQTRibkdxUU1EK0FhUUo4amd1bGpDSXRuUGkzRlAxczlTZU1RSjdtb29j?= =?utf-8?B?WHY0SlhlMXNUajdXdFUxWmlHdExOYXNrQnlCVjdraXNWSHZzNUhtT25mQmIy?= =?utf-8?B?ZTZNMjc0bXcwV3p2WHZkWlVIWmtOdVhxb1dmbG9aQVoydnFHWVlzRGtNQnh1?= =?utf-8?B?WXlrL0Y3cDVDOGtSQ2tkY1QyU0dMT3dwVVNxWkxJMW1pTkJtempRZDFzVzJ1?= =?utf-8?B?M3Nhb1ZucGpZMVVsUWtUc01QbzM2WWYyOU1naURRaCtXMUFvT3YxdS9RbDFs?= =?utf-8?B?c0oyZm92dko0RFA5OHd6VTd6TFkvMUROOThRMmV0UGhha3E4NEFZTFJoR2ZQ?= =?utf-8?B?bFE5SzVXMVRYQWErMTFSdDM5TS9JOWkvY3lYMDRiSWs5cTZ0QWUzQzBKVmJL?= =?utf-8?B?VVlacFFaNkxkbVJSNzZjRFY5TVZRL3FHUGVzU3cxazNVajVrYTVrOUVCTnh4?= =?utf-8?B?bVo3Y0dha1RHR1JsRG9UaE1HMlpPL1k5TTgzbDZTd1dhTDRKbHJWZkpHUjNv?= =?utf-8?B?TmdOTnM1MVJ3eDhCcURWSkgvMHUrNlJWQXFrRUFOYzcrSDN3VXBaZWRsV0Zv?= =?utf-8?B?Mm92b1NJUUl3dld0WWQ2RTc3NEhkUk9QQXVIdkVBbGhBbklBdjl2TEI5QSto?= =?utf-8?B?QVhqcEV4bitNTVdON0lBbmI1NVVnaXVaQVcxdGMxT3BPdFhFL1JLMjA2VWlN?= =?utf-8?B?WnByVWY4Z1d4VlE3aWl6Nmcya2pQaFJiclpKdjBOcFBsWmliU0pZaG9rVGJl?= =?utf-8?B?a3ZNbisvZjZpNjI3Qy9EZktHK3VoQ2JqR09nZmJheVZ3cmZMbW1leW1lNnlH?= =?utf-8?Q?SUj3Mngyrf6yS846WlHUYlyKN?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 2d0d6a91-309d-46bc-8d0f-08db0eb7951d X-MS-Exchange-CrossTenant-AuthSource: CH2PR12MB4294.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Feb 2023 18:16:24.9571 (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: SzKiXPRpDfn/J1IYvNCS6xU/WILU0oU8ZbYnECunOFtJuwm2iOHeWWCt6Zzt0CMv X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR12MB5442 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 1/24/2023 10:47 AM, David Marchand wrote: > Reduce code duplication by introducing a helper that takes care of > transmitting, retrying if enabled and incrementing tx counter. > > Signed-off-by: David Marchand <...> > diff --git a/app/test-pmd/noisy_vnf.c b/app/test-pmd/noisy_vnf.c > index 937d5a1d7d..3875590132 100644 > --- a/app/test-pmd/noisy_vnf.c > +++ b/app/test-pmd/noisy_vnf.c > @@ -93,30 +93,6 @@ sim_memory_lookups(struct noisy_config *ncf, uint16_t nb_pkts) > } > } > > -static uint16_t > -do_retry(uint16_t nb_rx, uint16_t nb_tx, struct rte_mbuf **pkts, > - struct fwd_stream *fs) > -{ > - uint32_t retry = 0; > - > - while (nb_tx < nb_rx && retry++ < burst_tx_retry_num) { > - rte_delay_us(burst_tx_delay_time); > - nb_tx += rte_eth_tx_burst(fs->tx_port, fs->tx_queue, > - &pkts[nb_tx], nb_rx - nb_tx); > - } > - > - return nb_tx; > -} > - > -static uint32_t > -drop_pkts(struct rte_mbuf **pkts, uint16_t nb_rx, uint16_t nb_tx) > -{ > - if (nb_tx < nb_rx) > - rte_pktmbuf_free_bulk(&pkts[nb_tx], nb_rx - nb_tx); > - > - return nb_rx - nb_tx; > -} > - > /* > * Forwarding of packets in noisy VNF mode. Forward packets but perform > * memory operations first as specified on cmdline. > @@ -156,38 +132,23 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs) > > if (!ncf->do_buffering) { > sim_memory_lookups(ncf, nb_rx); > - nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, > - pkts_burst, nb_rx); > - if (unlikely(nb_tx < nb_rx) && fs->retry_enabled) > - nb_tx += do_retry(nb_rx, nb_tx, pkts_burst, fs); > - inc_tx_burst_stats(fs, nb_tx); > - fs->tx_packets += nb_tx; > - fs->fwd_dropped += drop_pkts(pkts_burst, nb_rx, nb_tx); > + nb_tx = common_fwd_stream_transmit(fs, pkts_burst, nb_rx); > 'nb_tx' is not used or necessary in this context, so assignment is not necassary. PS: In the latest next-net head, there is a 'goto' here instead of return, but that is becuase of recording cycles, becuase of optimization in this set (patch 1/6) that needs to turn back to 'return' that is what I did while applying patch. > kreturn true; > } > > fifo_free = rte_ring_free_count(ncf->f); > if (fifo_free >= nb_rx) { > - nb_enqd = rte_ring_enqueue_burst(ncf->f, > - (void **) pkts_burst, nb_rx, NULL); > - if (nb_enqd < nb_rx) > - fs->fwd_dropped += drop_pkts(pkts_burst, > - nb_rx, nb_enqd); > - } else { > - nb_deqd = rte_ring_dequeue_burst(ncf->f, > - (void **) tmp_pkts, nb_rx, NULL); > - nb_enqd = rte_ring_enqueue_burst(ncf->f, > - (void **) pkts_burst, nb_deqd, NULL); > - if (nb_deqd > 0) { > - nb_tx = rte_eth_tx_burst(fs->tx_port, > - fs->tx_queue, tmp_pkts, > - nb_deqd); > - if (unlikely(nb_tx < nb_rx) && fs->retry_enabled) > - nb_tx += do_retry(nb_rx, nb_tx, tmp_pkts, fs); > - inc_tx_burst_stats(fs, nb_tx); > - fs->fwd_dropped += drop_pkts(tmp_pkts, nb_deqd, nb_tx); > + nb_enqd = rte_ring_enqueue_burst(ncf->f, (void **) pkts_burst, nb_rx, NULL); > + if (nb_enqd < nb_rx) { > + fs->fwd_dropped += nb_rx - nb_enqd; > + rte_pktmbuf_free_bulk(&pkts_burst[nb_enqd], nb_rx - nb_enqd); Why not keep 'drop_pkts()' for this block, it is easier to read with it. > } > + } else { > + nb_deqd = rte_ring_dequeue_burst(ncf->f, (void **) tmp_pkts, nb_rx, NULL); > + nb_enqd = rte_ring_enqueue_burst(ncf->f, (void **) pkts_burst, nb_deqd, NULL); > + if (nb_deqd > 0) > + nb_tx = common_fwd_stream_transmit(fs, tmp_pkts, nb_deqd); 'nb_tx' assignment looks wrong, function returns 'nb_dropped' not 'nb_tx'. 'nb_tx' used below to detect if flush needed ('needs_flush'), so 'needs_flush' may be set wrong becuase dropped packet is used instead number of Tx packets. > } > > sim_memory_lookups(ncf, nb_enqd); > @@ -204,15 +165,9 @@ pkt_burst_noisy_vnf(struct fwd_stream *fs) > needs_flush = delta_ms >= noisy_tx_sw_buf_flush_time && > noisy_tx_sw_buf_flush_time > 0 && !nb_tx; > while (needs_flush && !rte_ring_empty(ncf->f)) { > - unsigned int sent; > nb_deqd = rte_ring_dequeue_burst(ncf->f, (void **)tmp_pkts, > MAX_PKT_BURST, NULL); > - sent = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, > - tmp_pkts, nb_deqd); > - if (unlikely(sent < nb_deqd) && fs->retry_enabled) > - nb_tx += do_retry(nb_rx, nb_tx, tmp_pkts, fs); > - inc_tx_burst_stats(fs, nb_tx); > - fs->fwd_dropped += drop_pkts(tmp_pkts, nb_deqd, sent); > + nb_tx = common_fwd_stream_transmit(fs, tmp_pkts, nb_deqd); similaryly 'nb_tx' assignment can be wrong here, and 'nb_tx' used for return value to hint if record cycle is required, using number of dropped packets (what 'common_fwd_stream_transmit()' returns) gives wrong result. > ncf->prev_time = rte_get_timer_cycles(); > } > > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h > index e6b28b4748..71ff70f55b 100644 > --- a/app/test-pmd/testpmd.h > +++ b/app/test-pmd/testpmd.h > @@ -870,6 +870,36 @@ common_fwd_stream_receive(struct fwd_stream *fs, struct rte_mbuf **burst, > return nb_rx; > } > > +/* Returns count of dropped packets. */ > +static inline uint16_t > +common_fwd_stream_transmit(struct fwd_stream *fs, struct rte_mbuf **burst, > + unsigned int count) I would use 'nb_pkts' instead of 'count' as variable name since it is more common, but of course this is subjective. > +{ > + uint16_t nb_tx; > + uint32_t retry; > + > + nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, burst, count); > + /* > + * Retry if necessary > + */ > + if (unlikely(nb_tx < count) && fs->retry_enabled) { > + retry = 0; > + while (nb_tx < count && retry++ < burst_tx_retry_num) { > + rte_delay_us(burst_tx_delay_time); > + nb_tx += rte_eth_tx_burst(fs->tx_port, fs->tx_queue, > + &burst[nb_tx], count - nb_tx); > + } > + } > + fs->tx_packets += nb_tx; > + inc_tx_burst_stats(fs, nb_tx); > + if (unlikely(nb_tx < count)) { > + fs->fwd_dropped += (count - nb_tx); > + rte_pktmbuf_free_bulk(&burst[nb_tx], count - nb_tx); > + } > + > + return count - nb_tx; Instead of returning number of dropped packets, what about returning number of packets sent ('nb_tx')? Intuitively this is what expected from a function named 'common_fwd_stream_transmit()', and even if it is more optimised to return number of dropped packet, this may have only a little impact on the caller code. And even 'fs->tx_packets' updated withing the function, updating it externally and explicitly feels me as it clarifies usage more, although this part is up to you, I mean usage like: fs->tx_packets += common_fwd_stream_transmit(fs, pkts, nb_pkts);