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 7828CA04E4; Tue, 11 Aug 2020 22:39:11 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 200191C026; Tue, 11 Aug 2020 22:39:10 +0200 (CEST) Received: from mail-pj1-f68.google.com (mail-pj1-f68.google.com [209.85.216.68]) by dpdk.org (Postfix) with ESMTP id 881D21C025 for ; Tue, 11 Aug 2020 22:39:08 +0200 (CEST) Received: by mail-pj1-f68.google.com with SMTP id f9so22108pju.4 for ; Tue, 11 Aug 2020 13:39:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=hr5jI0FRr9XDi+WIm5rD52/auOCN7VlaxRoiPyp2z6Q=; b=eB0LYzh8SaXI+IxO64HoRTHd8/SFNSn2y5otm/btmOuKozA6s1jlC1aNMDIPuWk7cr jmRthwptHyGHsI0RLq3FkeCbOx2fr3JoVKe7vt7WpUn2du6wkKhG5eFCIqVIKMwLJprx PeDVDACVndTDffe9Lb//Nlx4/g9oe+Xag7eNaqEyV6JBJKKWkhkE/rQannSgM87clK7h MLShVGbZt7xK6wba5GLaNugd24EhvOAuCc3DoFGK/wKWHBREdYrfkT0DGtgkcVcgm4Rr SF6nt1v/DR+i849PoXHLT2YoLeFE1vsoxlYRWXUAc36dONAZRZNOgjAk1VqRLNHoM9bF Uh5g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=hr5jI0FRr9XDi+WIm5rD52/auOCN7VlaxRoiPyp2z6Q=; b=geJLGEj+AqkIbooL0hMg3aPaNxjObv52hNgT4b1cenwBa72Mf6mip1M0R4ORlxbAR/ I2znSD2aazEOS+kCCPJ3+UN3G4u0BETPmVqKlQBggjL6lD9EZhsorIunY+ZoQHN0/W7I /oGRfH6oSLVW1fSfOjZOSaIopWd/7hjDwVGj2RFKYXU9PaGs1HCNCAl4aqdEzhBhb7VE ga9FfVbP0iBK+bmBrZwH2wcA+RreRU5dXr7N1K/vcWv1yMeadeJgsI9Z3GxRyUjozmPk 3dWGTC0RQTmYZwhWBNTYqybBe0jhUK+BtZar2gc/M8RgObGhSeNsmO+6TSj7WEMPNLCO u7lg== X-Gm-Message-State: AOAM533dHst4ZkIDlcjW/MFEUHh5Wznl9d7P35MmSwaz6BI5kxYi1VTX Im26ofL+6D3LQhw3C8V8JgZoiw== X-Google-Smtp-Source: ABdhPJydzEkSfT+ZArn50pOyFP4Y/hacQl6nnY70jOMwIzEQiPt3GuRJ0ylIONjVVcIGsKhtyinwHg== X-Received: by 2002:a17:90a:e381:: with SMTP id b1mr2783980pjz.218.1597178347587; Tue, 11 Aug 2020 13:39:07 -0700 (PDT) Received: from hermes.lan (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id j8sm6397191pff.133.2020.08.11.13.39.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 11 Aug 2020 13:39:07 -0700 (PDT) Date: Tue, 11 Aug 2020 13:38:58 -0700 From: Stephen Hemminger To: "Eads, Gage" Cc: Steven Lariau , Olivier Matz , "dev@dpdk.org" , "honnappa.nagarahalli@arm.com" , "dharmik.thakkar@arm.com" , "nd@arm.com" Message-ID: <20200811133858.04ec8369@hermes.lan> In-Reply-To: References: <20200805155721.19808-1-steven.lariau@arm.com> <20200805155721.19808-2-steven.lariau@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/4] test/stack: avoid trivial memory allocations 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" On Tue, 11 Aug 2020 20:13:24 +0000 "Eads, Gage" wrote: > Hi Steven, > > > -----Original Message----- > > From: Steven Lariau > > Sent: Wednesday, August 5, 2020 10:57 AM > > To: Eads, Gage ; Olivier Matz > > > > Cc: dev@dpdk.org; honnappa.nagarahalli@arm.com; > > dharmik.thakkar@arm.com; nd@arm.com; Steven Lariau > > > > Subject: [PATCH 1/4] test/stack: avoid trivial memory allocations > > > > Replace the arguments array by one argument. > > All objects in the args array have the same values, so there is no need > > to use an array, only one struct is enough. > > The args object is a lot smaller, and the allocation can be replaced > > with a stack variable. > > > > The allocation of obj_table isn't needed either, because MAX_BULK is > > small. The allocation can instead be replaced with a static array. > > > > Signed-off-by: Steven Lariau > > Reviewed-by: Dharmik Thakkar > > Reviewed-by: Phil Yang > > Reviewed-by: Ruifeng Wang > > --- > > app/test/test_stack.c | 39 ++++++--------------------------------- > > 1 file changed, 6 insertions(+), 33 deletions(-) > > > > diff --git a/app/test/test_stack.c b/app/test/test_stack.c > > index c8dac1f55..5a7273a7d 100644 > > --- a/app/test/test_stack.c > > +++ b/app/test/test_stack.c > > @@ -280,16 +280,9 @@ static int > > stack_thread_push_pop(void *args) > > { > > struct test_args *t = args; > > - void **obj_table; > > + void *obj_table[MAX_BULK]; > > int i; > > > > - obj_table = rte_calloc(NULL, STACK_SIZE, sizeof(void *), 0); > > - if (obj_table == NULL) { > > - printf("[%s():%u] failed to calloc %zu bytes\n", > > - __func__, __LINE__, STACK_SIZE * sizeof(void *)); > > - return -1; > > - } > > - > > for (i = 0; i < NUM_ITERS_PER_THREAD; i++) { > > unsigned int success, num; > > > > @@ -310,28 +303,25 @@ stack_thread_push_pop(void *args) > > if (rte_stack_push(t->s, obj_table, num) != num) { > > printf("[%s():%u] Failed to push %u pointers\n", > > __func__, __LINE__, num); > > - rte_free(obj_table); > > return -1; > > } > > > > if (rte_stack_pop(t->s, obj_table, num) != num) { > > printf("[%s():%u] Failed to pop %u pointers\n", > > __func__, __LINE__, num); > > - rte_free(obj_table); > > return -1; > > } > > > > rte_atomic64_sub(t->sz, num); > > } > > > > - rte_free(obj_table); > > return 0; > > } > > Agreed, the dynamic allocation is unnecessary. > > > > > static int > > test_stack_multithreaded(uint32_t flags) > > { > > - struct test_args *args; > > + struct test_args args; > > unsigned int lcore_id; > > struct rte_stack *s; > > rte_atomic64_t size; > > @@ -344,45 +334,28 @@ test_stack_multithreaded(uint32_t flags) > > printf("[%s():%u] Running with %u lcores\n", > > __func__, __LINE__, rte_lcore_count()); > > > > - args = rte_malloc(NULL, sizeof(struct test_args) * RTE_MAX_LCORE, > > 0); > > - if (args == NULL) { > > - printf("[%s():%u] failed to malloc %zu bytes\n", > > - __func__, __LINE__, > > - sizeof(struct test_args) * RTE_MAX_LCORE); > > - return -1; > > - } > > - > > s = rte_stack_create("test", STACK_SIZE, rte_socket_id(), flags); > > if (s == NULL) { > > printf("[%s():%u] Failed to create a stack\n", > > __func__, __LINE__); > > - rte_free(args); > > return -1; > > } > > > > rte_atomic64_init(&size); > > + args.s = s; > > + args.sz = &size; > > > > RTE_LCORE_FOREACH_SLAVE(lcore_id) { > > - args[lcore_id].s = s; > > - args[lcore_id].sz = &size; > > - > > if (rte_eal_remote_launch(stack_thread_push_pop, > > - &args[lcore_id], lcore_id)) > > + &args, lcore_id)) > > rte_panic("Failed to launch lcore %d\n", lcore_id); > > } > > > In general we shouldn't pass a stack variable to other threads. Though your > code here looks fine, I'd rather err on the safe side in case this is ever used > as a template/basis for some other code...particularly since there's no > performance/correctness/etc. penalty to using dynamically allocated memory. > > To support patch 2/4, you can instead convert the rte_malloc to allocate a > single shared test_args structure. Or perhaps move patch 4 earlier in the series, > and simply pass the stack pointer instead. > > Thanks, > Gage There is no gain to using rte_malloc unless you are doing primary/secondary process or trying to test rte_malloc. Why not use regular malloc which has good tools and library support.