From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E38B2A0597; Thu, 9 Apr 2020 03:32:07 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EBC911C11C; Thu, 9 Apr 2020 03:32:06 +0200 (CEST) Received: from EUR03-AM5-obe.outbound.protection.outlook.com (mail-eopbgr30081.outbound.protection.outlook.com [40.107.3.81]) by dpdk.org (Postfix) with ESMTP id 793291C10F; Thu, 9 Apr 2020 03:32:05 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=wkCR4K0GLyce5gY4tMEHe5DWUwZTzgtD0JqGpfDULPs=; b=7wBDNcYhpXWMDlwc+cNS+xoDZBonC+vt1QFmsoOWBI/xsdn0axiwBQ/qSc1E91txIhCQBGmzlL55EVPKZmH9qCHcnaHkDvNxRrA3171ICg3gGnlv7EIXwFq4xVYom2jD4D9ileiIqf1F0vOUVKibui8RW+vUBXzf01shTtf0p1s= Received: from AM6P194CA0044.EURP194.PROD.OUTLOOK.COM (2603:10a6:209:84::21) by AM4PR0802MB2129.eurprd08.prod.outlook.com (2603:10a6:200:5d::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2900.15; Thu, 9 Apr 2020 01:32:04 +0000 Received: from VE1EUR03FT027.eop-EUR03.prod.protection.outlook.com (2603:10a6:209:84:cafe::71) by AM6P194CA0044.outlook.office365.com (2603:10a6:209:84::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2900.15 via Frontend Transport; Thu, 9 Apr 2020 01:32:04 +0000 Authentication-Results: spf=pass (sender IP is 63.35.35.123) smtp.mailfrom=arm.com; dpdk.org; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com;dpdk.org; dmarc=bestguesspass action=none header.from=arm.com; Received-SPF: Pass (protection.outlook.com: domain of arm.com designates 63.35.35.123 as permitted sender) receiver=protection.outlook.com; client-ip=63.35.35.123; helo=64aa7808-outbound-1.mta.getcheckrecipient.com; Received: from 64aa7808-outbound-1.mta.getcheckrecipient.com (63.35.35.123) by VE1EUR03FT027.mail.protection.outlook.com (10.152.18.154) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2900.15 via Frontend Transport; Thu, 9 Apr 2020 01:32:03 +0000 Received: ("Tessian outbound 1425309d4c0b:v50"); Thu, 09 Apr 2020 01:32:03 +0000 X-CR-MTA-TID: 64aa7808 Received: from 6bc0c536cabd.2 by 64aa7808-outbound-1.mta.getcheckrecipient.com id 841063DD-1952-43C6-85E7-9DF4F161DE48.1; Thu, 09 Apr 2020 01:31:58 +0000 Received: from EUR02-AM5-obe.outbound.protection.outlook.com by 64aa7808-outbound-1.mta.getcheckrecipient.com with ESMTPS id 6bc0c536cabd.2 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384); Thu, 09 Apr 2020 01:31:58 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jcqWnctRQF+QFvHBG38eIf/Q5A3s2qrQLPJDOYSkr5IRd5182fI4i2Jfcn8qmSIcoAT2lIprEmXHxka8/EAnDoTIi5uqjcOMEf5E+iZl7GJfFxuKW124QQFf9sGZSbgRfA1nZ/LgBV0uZalJvDLJuuacqwLvRBocRGISiLi5/RL+E5t1D5QnxCBkCewwKOB3Ka7H+xA+HGwG08gCiC3+KjNMmvPd0KKbmwi4WkHUfKCY15AaOGi9NN5wYY0PLtcafByvlu1Elnp741Q1XBwNb4YS+oV4VVjJedK2T2oVrr+AafvObl1bpsH5Re2xevj2iVgIWsMN4xkjpSKBArSx8A== 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=wkCR4K0GLyce5gY4tMEHe5DWUwZTzgtD0JqGpfDULPs=; b=URyS4R/Qd1bmqW3C8qL5FJlxuO5NUHb7JOP4iabHMHVRQBzinT4nZdIZ8TpfTQv2mtmppSFm0VZ1MCBi20HAMCFSq7FkBbAT0kS2cHNXoGaYiHVqI2vLUmIEAWKxPnGWXE+ypr/lw0dLC3zAcKrVxuz4lp6wPZyjv27wiLbAFiNQJUb08bneDsf/KelI3GyRIvl1hAy1VKakoz//DiVk6oHtIA6RR3ji9NGnxILxovDX4TMGoBsIMkggJIi47m24EQFyZLNqqyjlX+5u0UhkLbdDL98/gnsIXPUKPYJbd0XEDr7jv5JmKGqS4vggDBVOJolpRgiNKHge3yBbGh4a9g== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector2-armh-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=wkCR4K0GLyce5gY4tMEHe5DWUwZTzgtD0JqGpfDULPs=; b=7wBDNcYhpXWMDlwc+cNS+xoDZBonC+vt1QFmsoOWBI/xsdn0axiwBQ/qSc1E91txIhCQBGmzlL55EVPKZmH9qCHcnaHkDvNxRrA3171ICg3gGnlv7EIXwFq4xVYom2jD4D9ileiIqf1F0vOUVKibui8RW+vUBXzf01shTtf0p1s= Received: from AM6PR08MB4644.eurprd08.prod.outlook.com (10.255.98.10) by AM6PR08MB4022.eurprd08.prod.outlook.com (20.179.3.223) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2900.15; Thu, 9 Apr 2020 01:31:56 +0000 Received: from AM6PR08MB4644.eurprd08.prod.outlook.com ([fe80::f943:cc7e:200b:f54e]) by AM6PR08MB4644.eurprd08.prod.outlook.com ([fe80::f943:cc7e:200b:f54e%5]) with mapi id 15.20.2878.021; Thu, 9 Apr 2020 01:31:56 +0000 From: Honnappa Nagarahalli To: "Van Haaren, Harry" , Phil Yang , "thomas@monjalon.net" , "Ananyev, Konstantin" , "stephen@networkplumber.org" , "maxime.coquelin@redhat.com" , "dev@dpdk.org" CC: "david.marchand@redhat.com" , "jerinj@marvell.com" , "hemant.agrawal@nxp.com" , Gavin Hu , Ruifeng Wang , Joyce Kong , nd , "stable@dpdk.org" , Honnappa Nagarahalli , nd Thread-Topic: [PATCH v3 09/12] service: avoid race condition for MT unsafe service Thread-Index: AQHWCa8oxGah6HGPZEGhfjgtjJWkFahn43MggAep2gCAABczAA== Date: Thu, 9 Apr 2020 01:31:56 +0000 Message-ID: References: <1583999071-22872-1-git-send-email-phil.yang@arm.com> <1584407863-774-1-git-send-email-phil.yang@arm.com> <1584407863-774-10-git-send-email-phil.yang@arm.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ts-tracking-id: 715e29cc-12f6-4dcf-828c-c47eeeec9f0b.0 x-checkrecipientchecked: true Authentication-Results-Original: spf=none (sender IP is ) smtp.mailfrom=Honnappa.Nagarahalli@arm.com; x-originating-ip: [70.113.25.165] x-ms-publictraffictype: Email X-MS-Office365-Filtering-HT: Tenant X-MS-Office365-Filtering-Correlation-Id: f95e8934-44bd-41d3-5845-08d7dc25cee4 x-ms-traffictypediagnostic: AM6PR08MB4022:|AM6PR08MB4022:|AM4PR0802MB2129: x-ld-processed: f34e5979-57d9-4aaa-ad4d-b122a662184d,ExtAddr x-ms-exchange-transport-forked: True X-Microsoft-Antispam-PRVS: x-checkrecipientrouted: true nodisclaimer: true x-ms-oob-tlc-oobclassifiers: OLM:7219;OLM:7219; x-forefront-prvs: 0368E78B5B X-Forefront-Antispam-Report-Untrusted: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:AM6PR08MB4644.eurprd08.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(10009020)(4636009)(396003)(136003)(39850400004)(366004)(376002)(346002)(52536014)(5660300002)(4326008)(2906002)(478600001)(26005)(55016002)(8676002)(9686003)(186003)(81166007)(86362001)(33656002)(8936002)(316002)(81156014)(7416002)(71200400001)(6506007)(110136005)(76116006)(7696005)(66476007)(66556008)(66946007)(30864003)(66446008)(64756008)(54906003); DIR:OUT; SFP:1101; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam-Untrusted: BCL:0; X-Microsoft-Antispam-Message-Info-Original: 0Vh3s+43bMLgcp/TJzAwRNKYTxfy6zqcQdV6FzTbs2u1z89Pt3VA5KrEYK6bzdWLfS5s0HZo1B+hiM5xQ8DTPDlRIEQS1QdnrEMvlt0AHzvXH73U/Hx8AKx5U97ranwq87rL822rIHYnGDm4Mt9lR7ilidxbhQGCXw0G2GlxtmIzpjDdzmgiJ1zSW1UHhU72unNIZzwPxBlbR84dO0P5w84fDgU6e5BmMmS2PSRYhQb7J/ExUbWkxlnme9qGtxkH10oA0ra/eepesp21sHxvfwVzNjuV6UgDEs1hNmIvpFBw551lHgkTOdJL2cxLByaQB5lul9tPstM+lbKtqhvZpydb5yTbJWMGS3N2iULyJc1sz9fRtebnMWHYNEFgXGSAdqqR7GFuABnfILs5dD62uqfyO0qLgrfLyAmblhV1JAoGdtIW1yuB2oeEHETZvvt4 x-ms-exchange-antispam-messagedata: /dMx2q+wy7F+3mmoDiuN2UwhpaarTbRtXGuZPw3+btcbiYb3wkcFeyoU/to5JFnr1E4Ib0Gv47XxcJY0MLG/IFNVuf6LR8wI9EykAYov+Ph31nScspKJgJFW3nx0tJzLOx5KHUijJER1Ih2rZzH3/w== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR08MB4022 Original-Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Honnappa.Nagarahalli@arm.com; X-EOPAttributedMessage: 0 X-MS-Exchange-Transport-CrossTenantHeadersStripped: VE1EUR03FT027.eop-EUR03.prod.protection.outlook.com X-Forefront-Antispam-Report: CIP:63.35.35.123; CTRY:IE; LANG:en; SCL:1; SRV:; IPV:CAL; SFV:NSPM; H:64aa7808-outbound-1.mta.getcheckrecipient.com; PTR:ec2-63-35-35-123.eu-west-1.compute.amazonaws.com; CAT:NONE; SFTY:; SFS:(10009020)(4636009)(39850400004)(376002)(136003)(346002)(396003)(46966005)(478600001)(5660300002)(86362001)(336012)(450100002)(4326008)(26826003)(2906002)(9686003)(33656002)(186003)(8676002)(81166007)(26005)(55016002)(110136005)(8936002)(52536014)(47076004)(81156014)(316002)(30864003)(54906003)(70586007)(6506007)(36906005)(7696005)(70206006)(82740400003)(356004); DIR:OUT; SFP:1101; X-MS-Office365-Filtering-Correlation-Id-Prvs: 0ca2f9f2-4864-4ed8-ba47-08d7dc25ca55 X-Forefront-PRVS: 0368E78B5B X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: lHmb8FZowPjoajkGm0chU0RdYfoYFGfbb/pKgc+n42rh5pE+MfuRNKLVbHDEgn5wPFJTd2qK/8fbwCS3z3yjIgcqGhhceHgeWchZHuRkBYMkWPN2m3ZZrYjEWernoLKasX2SnQbXqFEcodL9Dx19p6SAp1CydhPSo9n4dwuG+sYJqAwH6JkIMIrPbQKH1qzOHJtfgEtgcK9qC+zeM0v6W/l95AJBI8hwdyHkiVBseKp0d9PVO8dsPcKsxpiHVgPROTSNNOYjY4a8FVFpW15UDEBAtHN3Cxh4ne3gGxMZIyF5HaldTA6dW2rIBsNOCmsuy7DYLBvkXI5Skby+14aG9OS8OGA8sG8hb5Icl5G3niTPu9vaViBKlOyjAhoUHnMDsWuWdSVti+Eds+X6WzGZCconxb6x0FcKgvbkC8LcFnDMloCJp4aW3IYBdgHxOIKANBltHh2GRcr/B6fekXMEm5D4nP8/HyXe71phyl+4oZKdNpouOcZf2IcNwaprHXxi7YxRL+ZNmNPqlla5wtbSbA== X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Apr 2020 01:32:03.7334 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: f95e8934-44bd-41d3-5845-08d7dc25cee4 X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=f34e5979-57d9-4aaa-ad4d-b122a662184d; Ip=[63.35.35.123]; Helo=[64aa7808-outbound-1.mta.getcheckrecipient.com] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR0802MB2129 Subject: Re: [dpdk-dev] [PATCH v3 09/12] service: avoid race condition for MT unsafe service X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" > > > > > > Subject: [PATCH v3 09/12] service: avoid race condition for MT > > > > unsafe service > > > > > > > > From: Honnappa Nagarahalli > > > > > > > > There has possible that a MT unsafe service might get configured > > > > to run on another core while the service is running currently. > > > > This might result in the MT unsafe service running on multiple > > > > cores simultaneously. Use 'execute_lock' always when the service > > > > is MT unsafe. > > > > > > > > Fixes: e9139a32f6e8 ("service: add function to run on app lcore") > > > > Cc: stable@dpdk.org > > > > > > > > Signed-off-by: Honnappa Nagarahalli > > > > Reviewed-by: Phil Yang > > > > Reviewed-by: Gavin Hu > > > > > > We should put "fix" in the title, once converged on an implementation= . > > Ok, will replace 'avoid' with 'fix' (once we agree on the solution) > > > > > > > > Regarding Fixes and stable backport, we should consider if fixing > > > this in > > stable > > > with a performance degradation, fixing with more complex solution, > > > or documenting a known issue a better solution. > > > > > > > > > This fix (always taking the atomic lock) will have a negative > > > performance impact on existing code using services. We should > > > investigate a way to fix > > it > > > without causing datapath performance degradation. > > Trying to gauge the impact on the existing applications... > > The documentation does not explicitly disallow run time mapping of > > cores to service. > > 1) If the applications are mapping the cores to services at run time, > > they are running with a bug. IMO, bug fix resulting in a performance > > drop should be acceptable. > > 2) If the service is configured to run on single core > > (num_mapped_cores =3D=3D 1), but service is set to MT unsafe - this wil= l > > have a (possible) performance impact. > > a) This can be solved by setting the service to MT safe and can be > > documented. This might be a reasonable solution for applications which > > are compiling with > > future DPDK releases. > > b) We can also solve this using symbol versioning - the old version > > of this function will use the old code, the new version of this > > function will use the code in > > this patch. So, if the application is run with > > future DPDK releases without recompiling, it will continue to use the > > old version. If the application is compiled > > with future releases, they can use solution in 2a. > > We also should think if this is an appropriate solution as this would > > force 1) to recompile to get the fix. > > 3) If the service is configured to run on multiple cores > > (num_mapped_cores > 1), then for those applications, the lock is being > > taken already. These applications might see some improvements as this > > patch removes few instructions. > > > > > > > > I think there is a way to achieve this by moving more checks/time to > > > the control path (lcore updating the map), and not forcing the > > > datapath lcore to always take an atomic. > > I think 2a above is the solution. >=20 > 2a above is e.g. the Eventdev SW routines like Rx/Tx scheduler services. I scanned through the code briefly I see that Eth RX/TX, Crypto adapters are setting the MT_SAFE capabilities,= can be ignored. Timer adaptor and some others do not set MT_SAFE. Seems like the cores to r= un on are mapped during run time. But it is not clear to me if it can get m= apped to run on multiple cores. If they are, they are running with the bug. But, these are all internal to DPDK and can be fixed. Are there no performance tests in these components that we can run? > We should strive to not reduce datapath performance at all here. >=20 >=20 > > > In this particular case, we have a counter for number of iterations > > > that a > > Which counter are you thinking about? > > All the counters I checked are not atomic operations currently. If we > > are going to use counters they have to be atomic, which means > > additional cycles in the data path. >=20 > I'll try to explain the concept better, take this example: > - One service core is mapped to a MT_UNSAFE service, like event/sw pmd > - Application wants to map a 2nd lcore to the same service > - You point out that today there is a race over the lock > -- the lock is not taken if (num_mapped_lcores =3D=3D 1) > -- this avoids an atomic lock/unlock on the datapath >=20 > To achieve our desired goal; > - control thread doing mapping performs the following operations > -- write service->num_mapped_lcores++ (atomic not required, only sing= le- > writer allowed by APIs) This has to be atomic because of rte_service_run_iter_on_app_lcore API. Per= formance should be fine as this API is not called frequently. But need to c= onsider the implications of more than one thread updating num_mapped_cores. > -- MFENCE (full st-ld barrier) to flush write, and force later loads = to issue > after I am not exactly sure what MFENCE on x86 does. On Arm platforms, the full b= arrier (DMB ISH) just makes sure that memory operations are not re-ordered = around it. It does not say anything about when that store is visible to oth= er cores. It will be visible at some point in time to cores. But, I do not think we need to be worried about flushing to memory. > -- read the "calls_per_service" counter for each lcores, add them up. This can be trimmed down to the single core the service is mapped to curren= tly, no need to add all the counters. > ---- Wait :) > -- re-read the "calls_per_service", and ensure the count has changed. Basically, polling. This causes more traffic on the interconnect between th= e cores. But might be ok since this API might not be called frequently. > ---- The fact that the calls_per_service has changed ensures the serv= ice- > lcore > has seen the new "num_mapped_cores" value, and has now taken the > lock! > -- *now* it is safe to map the 2nd lcore to the service >=20 > There is a caveat here that the increments to the "calls_per_service" var= iable > must become globally-observable. To force this immediately would require = a > write memory barrier, which could impact datapath performance. Given the > service is now taking a lock, the unlock() thereof would ensure the > "calls_per_service" > is flushed to memory. If we increment this variable only when the lock is held, we should be fine= . We could have a separate variable. >=20 > Note: we could use calls_per_service, or add a new variable to the servic= e > struct. > Writes to this do not need to be atomic, as it is either mapped to a sing= le core, > or else there's a lock around it. I think it is better to have a separate variable that is updated only when = the lock is held. I do not see any change in API sequence. We do this hand-shake only if the = service is running (which is all controlled in the writer thread), correct? This does not solve the problem with rte_service_run_iter_on_app_lcore gett= ing called on multiple cores concurrently for the same service.=20 >=20 >=20 > > > service has done. If this increments we know that the lcore running > > > the service has re-entered the critical section, so would see an > > > updated "needs atomic" flag. > > > > > > This approach may introduce a predictable branch on the datapath, > > > however the cost of a predictable branch vs always taking an atomic > > > is order(s?) of magnitude, so a branch is much preferred. > > > > > > It must be possible to avoid the datapath overhead using a scheme lik= e > this. > > It > > > will likely be more complex than your proposed change below, however > > > if it avoids datapath performance drops I feel that a more complex > > > solution is worth investigating at least. >=20 > > I do not completely understand the approach you are proposing, may be > > you can elaborate more. >=20 > Expanded above, showing a possible solution that does not require additio= nal > atomics on the datapath. >=20 >=20 > > But, it seems to be based on a counter approach. Following is my > > assessment on what happens if we use a counter. Let us say we kept > > track of how many cores are running the service currently. We need an > > atomic counter other than 'num_mapped_cores'. Let us call that counter > 'num_current_cores'. > > The code to call the service would look like below. > > > > 1) rte_atomic32_inc(&num_current_cores); /* this results in a full > > memory barrier */ > > 2) if (__atomic_load_n(&num_current_cores, __ATOMIC_ACQUIRE) =3D=3D 1) = { > > /* rte_atomic_read is not enough here as it does not provide the > > required memory barrier for any architecture */ > > 3) run_service(); /* Call the service */ > > 4) } > > 5) rte_atomic32_sub(&num_current_cores); /* Calling rte_atomic32_clear > > is not enough as it is not an atomic operation and does not provide > > the required memory barrier */ > > > > But the above code has race conditions in lines 1 and 2. It is > > possible that none of the cores will ever get to run the service as > > they all could simultaneously increment the counter. Hence lines 1 and > > 2 together need to be atomic, which is nothing but 'compare-exchange' > operation. > > > > BTW, the current code has a bug where it calls 'rte_atomic_clear(&s- > > >execute_lock)', it is missing memory barriers which results in > > >clearing the > > execute_lock before the service has completed running. I suggest > > changing the 'execute_lock' to rte_spinlock_t and using > > rte_spinlock_try_lock and rte_spinlock_unlock APIs. >=20 > I don't think a spinlock is what we want here: >=20 > The idea is that a service-lcore can be mapped to multiple services. > If one service is already being run (by another thread), we do not want t= o spin > here waiting for it to become "free" to run by this thread, it should con= tinue > to the next service that it is mapped to. Agree. I am suggesting to use 'rte_spinlock_try_lock' (does not spin) which= is nothing but 'compare-exchange'. Since the API is available, we should m= ake use of it instead of repeating the code. >=20 >=20 > > > > > > A unit test is required to validate a fix like this - although > > > perhaps found > > by > > > inspection/review, a real-world test to validate would give confidenc= e. > > Agree, need to have a test case. > > > > > > > > > > > Thoughts on such an approach? > > > >