From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <stable-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id 445BD42B22
for <public@inbox.dpdk.org>; Tue, 16 May 2023 15:40:05 +0200 (CEST)
Received: from mails.dpdk.org (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id 3E3F242BDA;
Tue, 16 May 2023 15:40:05 +0200 (CEST)
Received: from mga05.intel.com (mga05.intel.com [192.55.52.43])
by mails.dpdk.org (Postfix) with ESMTP id B047F410EE;
Tue, 16 May 2023 15:40:02 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple;
d=intel.com; i=@intel.com; q=dns/txt; s=Intel;
t=1684244403; x=1715780403;
h=message-id:date:subject:to:cc:references:from:
in-reply-to:content-transfer-encoding:mime-version;
bh=uQQ6iuF/v0tzhn6OzMoze+1qDXLP2PeLOiRPBim+g58=;
b=RYi1N/QqXZTHCdLVbi6kR+x3hXFX2oavRMLcRRYZrGpiqBvjDe640mTm
avBgbB4rJMcP3RlZWh/1OsFRZ+ZdDTClR1k7xrdnrfGXWKiziUs4lJNiZ
LTRJ4DZNAF3E1lWi8J2ZsHJz1UY4c65R30hCJIdIKAAo/9xKMi+x60Zjb
3s/EdbPe2h0RO2ddZ67z2kZQFjv9xCPRHWamKvFbeem2bzmrxoC6F/PQh
uci/ngPAcyz6Z6bycF3ehACdFuWcnjOwFFzOVmd/VsmbT1UGDCyc3Y55u
mw0yT6tbhU1OTlAi3bp+voLlvUOBRafKDhh5vcv9xgOJ1yxypmSMdJ96o A==;
X-IronPort-AV: E=McAfee;i="6600,9927,10711"; a="437820261"
X-IronPort-AV: E=Sophos;i="5.99,278,1677571200"; d="scan'208";a="437820261"
Received: from orsmga002.jf.intel.com ([10.7.209.21])
by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
16 May 2023 06:40:00 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=McAfee;i="6600,9927,10711"; a="701349925"
X-IronPort-AV: E=Sophos;i="5.99,278,1677571200"; d="scan'208";a="701349925"
Received: from fmsmsx601.amr.corp.intel.com ([10.18.126.81])
by orsmga002.jf.intel.com with ESMTP; 16 May 2023 06:39:59 -0700
Received: from fmsmsx612.amr.corp.intel.com (10.18.126.92) by
fmsmsx601.amr.corp.intel.com (10.18.126.81) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
15.1.2507.23; Tue, 16 May 2023 06:39:58 -0700
Received: from fmsmsx610.amr.corp.intel.com (10.18.126.90) by
fmsmsx612.amr.corp.intel.com (10.18.126.92) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
15.1.2507.23; Tue, 16 May 2023 06:39:58 -0700
Received: from fmsedg602.ED.cps.intel.com (10.1.192.136) by
fmsmsx610.amr.corp.intel.com (10.18.126.90) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
15.1.2507.23 via Frontend Transport; Tue, 16 May 2023 06:39:58 -0700
Received: from NAM10-BN7-obe.outbound.protection.outlook.com (104.47.70.102)
by edgegateway.intel.com (192.55.55.71) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id
15.1.2507.23; Tue, 16 May 2023 06:39:57 -0700
ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none;
b=AM73cgnVCSh/m5vEaE57BcqWxly8vJGmMtQnQdOulpedLRWHAc0bHzb0iHr+eMs3kjq7sLzfZouTDEGQjGsv/FqdZJO+9mJTcpfBJ4zIgYyGF43hQRtdFIQFLw3qE/dD9p3/DIp4UTRDkDXyIiQ95eh3Ze42IyRuYkFm82jsEjOpgTRz5Z7jUrIUnS2769AgpF+MECt5b558qCTmdZvLbS5HY2rpjBlDwIf3IiyoZhlhq+cnCmCgR/X1wbzLFqgBWQDuFWh6cev2tYq89l7s+8gj4uDsmsWVE3PJXm9697QHwWfgU5uhAq9Nl9oqkSvN++gHuyUMSXMEBjScqJURsA==
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=2xOBdwJizdtr58VemiTrNMSov/wHZU5WweK5x9ubiIc=;
b=ciQx0MPG3ern7sLK90ZgccUxRc9kBHFvp/Q+bsPn6qapImqWKA0M8Sdwo1PEajJK8TfE1u6NO8VMwO7jTp6vdObtVChXMC1LBWhL/G8x/A6/YMZSKQfvVlbqWbktGg9jJCGvWKx/YhgwmUnmPHlYRcXV+9/ZTRuMDSmkYRb3SOfZqEO/5naN7IOTet+94y1BIbhfC2fpn3PUXdIzMv/IiEqiXhMqqhFwUlKiQtNWmJ0t0TqtrxjWBRe1bXL03UTmaTBtOr03QbJHjrY65uWAoufYTAOkKaQNSX3eNT7XpeiFb0tzzWInDFNlyMz39lBWF6kniZluYtFnPAtt5hQ6yg==
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
Authentication-Results: dkim=none (message not signed)
header.d=none;dmarc=none action=none header.from=intel.com;
Received: from DM4PR11MB6502.namprd11.prod.outlook.com (2603:10b6:8:89::7) by
SA1PR11MB7062.namprd11.prod.outlook.com (2603:10b6:806:2b3::20) with
Microsoft SMTP Server (version=TLS1_2,
cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6387.33; Tue, 16 May
2023 13:39:56 +0000
Received: from DM4PR11MB6502.namprd11.prod.outlook.com
([fe80::49c0:aa4c:e5b4:e718]) by DM4PR11MB6502.namprd11.prod.outlook.com
([fe80::49c0:aa4c:e5b4:e718%5]) with mapi id 15.20.6387.033; Tue, 16 May 2023
13:39:55 +0000
Message-ID: <73232584-b595-93dd-01d2-778a2b8ea31d@intel.com>
Date: Tue, 16 May 2023 14:39:49 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
Firefox/102.0 Thunderbird/102.11.0
Subject: Re: [PATCH] Memory Allocation: Adding a new UT for fb_array
Content-Language: en-US
To: Vipin P R <vipinp@vmware.com>
CC: <dev@dpdk.org>, <stable@dpdk.org>
References: <1673615567-20873-1-git-send-email-vipinp@vmware.com>
<1673615567-20873-2-git-send-email-vipinp@vmware.com>
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
In-Reply-To: <1673615567-20873-2-git-send-email-vipinp@vmware.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-ClientProxiedBy: LO3P123CA0007.GBRP123.PROD.OUTLOOK.COM
(2603:10a6:600:ba::12) To DM4PR11MB6502.namprd11.prod.outlook.com
(2603:10b6:8:89::7)
MIME-Version: 1.0
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: DM4PR11MB6502:EE_|SA1PR11MB7062:EE_
X-MS-Office365-Filtering-Correlation-Id: 6dde2605-5eba-4ff5-2375-08db5613086a
X-MS-Exchange-SenderADCheck: 1
X-MS-Exchange-AntiSpam-Relay: 0
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: vXcsPvhjQppd8QPMVrsA4PW12SaGNNtZLzBaiaVXQn4wrHwrHk8uGMJPqvi8D53QptNvaBufVGoer0TfVMNBkFFvYzjXQdAsJCkMzamgtS7qXQXYuvB/xsaOJnF8WNPetJfNduqzkU+hxxOKpZlUEm5RgQPkvmE16e6ZisR3409kMEAMwc85AB0y+4+IcjQ1Cfqj5StXHnn+vcvtE30Kg1qXWeLOQLLQYEj0+BDSTlAaetmyHRL1M4FOyP1oJmvnQPi0ZunTpOqdXqVy+eQrq/OtpT564d3sO72srgzz5DkcGO0kMAfbgvJkN8mONsnc76/3AjbxlDzLw9WiUTze1hOJrFLoqDu3JdSSBjaknh7xoiQBXWX2kjsX18UaCoDdzW9JECFPQ3hPH+L3HJw3msHbvCTFyqJo0hKUGHLxPolI3U7Ks0AthpHU1EwkNDhWZ0J8aX+BD62yB4xbmaCnhD1hJwqQxBMzP5Ownd1zrvDlLwuVi8B8bQqRCAa+N6bqVfSmGpwVqF90Ep4G9E/0aHAW7hSM97ydiNRWe+IJsb2mDqgevinnZBuPONVMwWoF8hzsUGUyAlGvTbuTWY6z8VCUrqx9aeZy1BSMR7vAdtVttgSgrlYalKXwRJZWMHJEnxG4Gzd8+M+/C/9uJ6mEKw==
X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:;
IPV:NLI; SFV:NSPM; H:DM4PR11MB6502.namprd11.prod.outlook.com; PTR:; CAT:NONE;
SFS:(13230028)(376002)(136003)(366004)(346002)(396003)(39860400002)(451199021)(36756003)(6486002)(86362001)(316002)(66946007)(66556008)(66476007)(6916009)(4326008)(478600001)(6666004)(8936002)(8676002)(5660300002)(41300700001)(2906002)(31696002)(38100700002)(82960400001)(2616005)(6506007)(186003)(26005)(53546011)(6512007)(83380400001)(31686004)(45980500001)(43740500002);
DIR:OUT; SFP:1102;
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UmlvNFIxVVlOc1BCWHZwQ3p1T0ZMVHRGaGlObVJzRTVpYWpjSVMxWUZOa1hl?=
=?utf-8?B?QWxYR0h2TEw0dnFXUUtURXBUYnVPekFuTExGbzNSMGxWd3NmK3BJV1J5NWZu?=
=?utf-8?B?cjJkamJkRTNhWml4SUJ4L2pKYzIwZVdySWhlVjdRU1ZLbkZLRG10RDdNcW5a?=
=?utf-8?B?YXRPWGNMbmhZM1NFOWZ3bGNBNkpsaGZEdVZyN215TVVVbDhOaVRlMndvV0xs?=
=?utf-8?B?Uk9kcTJJYWxvQVNIL1ZieGlVeWRSclpjNHlUbjVLdisvNHkzVzNNYUFGNlR0?=
=?utf-8?B?aVVJbW9oRW5hblVCby9WYVJqWVVVeXlDU3hROFNtOUZmWE9EWUVMcWpLM3E4?=
=?utf-8?B?TVVYWUp0aSs5VFdrSzNWL2Q3dWljZ1hRemRib1M1VDdTNUVlY29GOXkzaUpC?=
=?utf-8?B?cis1Z1plZkF0eVI3dnRYTXZ5QUJXUWFoTW1rY3M2SHA5YU5CL0lJWTFjQTRV?=
=?utf-8?B?R09waEhBRnBzc2Vmc0VHcFVRUTJEM3ljK01mWEU3OTVmNXZsVXpyOUN5RGk3?=
=?utf-8?B?aUFUZUJDdHpZSTAyNExWQVhidVRRVGxNN2svZ1NkR3ZrRys4WjhZdXE1LzNs?=
=?utf-8?B?MHliNldoRVJUQlV4TWdaYmplNGRkMGFYaUZ6SWl0UWZ3b1RPS1hiMVJTWGxK?=
=?utf-8?B?elBqMzNTMnJQSDR2a2VVWlV2OThMZzJZWm9IK1ZNOFdZV0JDQTJQdUVMTFdP?=
=?utf-8?B?ZDlJaS9wTXd6MTVxMkJBUVVONnh2b2FwNy9xT0ZOQ1NYOWw2RklhQkVwcG5v?=
=?utf-8?B?S3ZqSTEzc0FFZ21IZGRsbHVHL0xDM29GUVVQaW5TbFBkU2ZPUVRLay9MRURH?=
=?utf-8?B?dXdnV3JPWHFHU00yS2g1UXNhMGU5L1ZUdHdXWUVLc3RJaEF3cnVTWllPZjFZ?=
=?utf-8?B?Z29VR0JPb1NTQzJ2Y1J4UHVMb2cyQzZneWpQemFab01NN0s1RVd5Tm9Eenp6?=
=?utf-8?B?YlBJM1RneXJhWXErQWoxeSs5T3YvaG9aN2psMmVrQkRpUXA1aWFQaFR2cEF6?=
=?utf-8?B?WndUWmdUckVaQTJGOUsxdUY1d0V2TTVtMzVXZ0NjR3kybGd1SWZTSmdVU0lq?=
=?utf-8?B?ZDZocVNYK01Dc00vU2djcG9XTDJRbCtLUlg5VlhzcVJqUlhoUmlERVRyNy9h?=
=?utf-8?B?cVlYLzdyTitpVXN6anRRaFFCT3pFTm43UXlLNDNvcVZvNzRCV1lCZFhBTEh1?=
=?utf-8?B?RVhSRTdiSzVLanRIcWFJaTkwRWM5S2ZlZzdOeENBZXh0THN5UlBXN3pGV2tW?=
=?utf-8?B?ZEYyeUpKMHpzcGdLL2dDbkFnK24xckM0OURsZWNaOE84VHRJYVhlQ1lkcXpV?=
=?utf-8?B?K3lralBSVXY4VjdOS01FOE43ZmxKSVh1ZGtiSWxkZE90R1NzdW5KZ1NwQm0w?=
=?utf-8?B?SXdtcGF4bzNPcWNSenhaUU5raXdyQWpYMzlrUGdUUjd4TnpNQkRYRWlyZ1dU?=
=?utf-8?B?WlhDMm0vT1RLYmlQMU9MV0lJOXlOVytaY1BiME0yc1FDbUwrbklZQmNKcE5Z?=
=?utf-8?B?TU9VSjFJd1ZyN1hYOFNTTDdxcDhFa29MYjUyTVNyWDRsU2h1ZFVFTjVWVDlV?=
=?utf-8?B?cWVoMmI2RG9Dd1FTSnV2RnkyU2dzd0luU010M0MwMFQzVWVIS1BCNlozWFND?=
=?utf-8?B?Qm45ajRqT3lKTUthd1AxNDJUaElqZmtXT1Z4b2xKOWJqLzNSRFFsQjBnS0xs?=
=?utf-8?B?cm1kRzRRMHRzak5mU1RKa0dhODdRN1ZFS2tSZys1aGVFZElGVTA1c1ZhMWlr?=
=?utf-8?B?dWxQWTQ2MEduNXNTQjNEWXNLR2c3aXR2VUw5cjNHY3ZzWFladVMyT0xRUGp1?=
=?utf-8?B?ODVBWGErSDNkV3F2VGRDVlMxKzVVMWJ1K3ppamcxNVkxN1lxQlA4UWRKUUo0?=
=?utf-8?B?QXBUU2lteTkxT2VKYUxYN1BSTG1iQytqL3FOS3doaFgyTk1SeE1COHc2UjdF?=
=?utf-8?B?L0VqK1Fqa08zK3Y3VkxtYWY3QVlTZkxEMCtkdEpZUXh4Z2svZlAvMG1rWnp6?=
=?utf-8?B?QUN3ZVdwSC94elVFeUQvVUVJcmFIdWRzR2gvRExvTjZ4d0VTa05McXRBVS9N?=
=?utf-8?B?OUs3b3J4c2YxU3ZMdUoyMnlXUjFmT0xkYkNzVmVUaU1TblhwVXpTNEJGcjlW?=
=?utf-8?B?Ym90S2NyV0x3enFtOUNPSXRtUmJDVU9kSkNwZ3Z0OTFjakJEUXMvNnVqN1Ja?=
=?utf-8?B?bFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6dde2605-5eba-4ff5-2375-08db5613086a
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6502.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 May 2023 13:39:55.1353 (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: D9rX+BTR5qvj4eZ0VTnGm/v0kdk0jWuExF9z8LvuzYRrHcBGPAtlShfuuqP2GMmdHQNh4ap8Qnm++I87lMQkMxi03PJ3TmA36i21oRm2KbQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB7062
X-OriginatorOrg: intel.com
X-BeenThere: stable@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: patches for DPDK stable branches <stable.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/stable>,
<mailto:stable-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/stable/>
List-Post: <mailto:stable@dpdk.org>
List-Help: <mailto:stable-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/stable>,
<mailto:stable-request@dpdk.org?subject=subscribe>
Errors-To: stable-bounces@dpdk.org
Hi Vipin!
Thanks for all of the work on this bug, it is highly appreciated. Below
are suggestions for improvements for this patch.
On 1/13/2023 1:12 PM, Vipin P R wrote:
> add test case coverage to cover the ms_idx jump
This message could be expanded to be more informative. Suggested rewording:
test/fbarray: add test case for incorrect lookahead behavior
>
> Cc: stable@dpdk.org
>
> Signed-off-by: Vipin P R <vipinp@vmware.com>
> Acked-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
> Depends-on: 0001-Memory-Allocation-Fixes-ms_idx-jump-lookahead-during.patch
> Depends-on: 0002-Memory-Allocation-Fixes-ms_idx-jump-lookbehind-durin.patch
This makes no difference for commit, but for future reference:
depends-on should reference link to actual patches, not a patch file name.
> ---
> app/test/test_fbarray.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/app/test/test_fbarray.c b/app/test/test_fbarray.c
> index a691bf4..275449c 100644
> --- a/app/test/test_fbarray.c
> +++ b/app/test/test_fbarray.c
> @@ -11,6 +11,7 @@
> #include <rte_debug.h>
> #include <rte_errno.h>
> #include <rte_fbarray.h>
> +#include <rte_memory.h>
This is presumably added to get access to `struct rte_memseg`, but this
is not needed, because the bug is in the mask behavior, which does not
depend on specific data size.
>
> #include "test.h"
>
> @@ -402,6 +403,53 @@ static int check_used_one(void)
> return 0;
> }
>
> +/* the following test case verifies that the jump in ms_idx for an fb-array is correct. */
> +static int test_jump(void)
> +{
> + struct rte_fbarray test_array;
> + int input[] = {1, 1070, 1, 2, 1, 2, 4, 12, 2, 2, 1, 2, 1};
I've managed to reduce this bug down to a more minimal example:
{ 63, 1, 2 }
> + int ms_idx, prev_ms_idx, delta;
> + int len;
> + ms_idx = prev_ms_idx = 0;
> +
> + int ret = rte_fbarray_init(&test_array, "test", 32768, sizeof(struct rte_memseg));
> + if (ret == 0) {
> + RTE_LOG(DEBUG, EAL, "FB array init success\n");
If the code did an early exit, an additional indentation level could've
been avoided, like so:
TEST_ASSERT(rte_fbarray_init(&test_array, "test", 256, 8) == 0,
"Failed to initialize fbarray\n");
Also, missing corresponding `rte_fbarray_destroy` call.
> + int k = 0;
Seems like the only place where this is used is in find_next_n_free, and
it never changes, so I don't think this variable is needed at all.
> + for(int i=0; i < sizeof(input)/sizeof(int); i++) {
RTE_DIM? Also, array indices are `unsigned int` rather than `int`,
compiler gives a warning.
> + if (i == 0) {
> + len = input[i];
> + } else {
> + len = input[i] + 1;
> + }
All of this could be rewritten as follows:
int len, hole;
/* if this is not the first iteration, create a hole */
hole = i != 0;
len = input[i] + hole;
> + prev_ms_idx = ms_idx;
> + ms_idx = rte_fbarray_find_next_n_free(&test_array, k, len);
Like I said above, `k` is unneeded, we can just replace it with 0.
> +
> + if (i != 0) {
> + ms_idx++;
> + }
Given suggestion above, could use `if (hole)` instead, would be more
readable.
> +
> + for (int j=0; j < input[i]; j++) {
Array indices are unsigned, and also could replace with
for (unsigned int j = hole; j < len; j++)
IMO would be more readable.
> + RTE_LOG(DEBUG, EAL, "ms_idx:%d\n", ms_idx);
I don't think this log is needed.
> + rte_fbarray_set_used(&test_array, ms_idx);
> + ms_idx++;
> + }
> +
> + if (prev_ms_idx) {
> + /* The value of ms_idx should be monotonically increasing
> + * given the above input sequence in test_array.
> + * */
> + delta = ms_idx - prev_ms_idx;
> + if (!(delta > 0)) {
Given above suggestions, this can be replaced with `if (delta != len)`.
Also, given the `TEST_ASSERT(0)` below, I think this could just be
replaced with an assert and a message, e.g.
TEST_ASSERT(delta == len, "Incorrect fbarray index\n");
--
Thanks,
Anatoly