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 4835142B22; Tue, 16 May 2023 15:40:04 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 25C1841141; Tue, 16 May 2023 15:40:04 +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 CC: , References: <1673615567-20873-1-git-send-email-vipinp@vmware.com> <1673615567-20873-2-git-send-email-vipinp@vmware.com> From: "Burakov, Anatoly" 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: 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 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 > Acked-by: Kumara Parameshwaran > --- > 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 > #include > #include > +#include 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