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 12D54A046B for ; Fri, 26 Jul 2019 15:40:07 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E6ACE4C80; Fri, 26 Jul 2019 15:40:05 +0200 (CEST) Received: from mail-vs1-f68.google.com (mail-vs1-f68.google.com [209.85.217.68]) by dpdk.org (Postfix) with ESMTP id 42FA1160 for ; Fri, 26 Jul 2019 15:40:04 +0200 (CEST) Received: by mail-vs1-f68.google.com with SMTP id k9so36078175vso.5 for ; Fri, 26 Jul 2019 06:40:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=CvQ0VCiuU7in6WtjMn+pFz7b10Wq/qGvaROZjkxPVe8=; b=LikG9eA2Fr99u7gaqYT63hl5aLhjlPBVALaw4f60UKqv4efd5PmDKNtsC1fyjNmIdh 4TTHqi7TmzstmIjasBzmJDzcrUwEc9y3YQ+axpw3ePr6SUurGcqHCTPHV4/bAWvg5PTz Pdvhc1EQW9M5uiR3PcQWPc0T0r5EZIVxaKQl4qt/oGzTNAHl3FI38l3nMMO67NhnUgZm PWk2PppTJo5E6aQBjspXb0B2C39xSNCmZp3ztNwP/UqPd5Ji4pCvibGnPsIafSzGE/Aq TfoUGEm8duIRs7I9APrsNVXrvc0ropenlrxRZwN+KgPY5N7gu5s2AossLjn1ETt4c2LP FEQQ== X-Gm-Message-State: APjAAAV8cKpM5vs6NQHBq53ln24dDnNVx+mVNuwdNHuT2t9eUdMnmJl3 QWsMMQrsqMXikQYboysQN/I2tExGN8X4nfK3np0JlA== X-Google-Smtp-Source: APXvYqzvNA1kmDlnSNkLxH/YBcCz5z/kdB9qooK+UD55ckPf5/89/utRUaWY/M+20UNw+4Wfdkp9qNtYlEmQc+mjbf4= X-Received: by 2002:a67:e9ca:: with SMTP id q10mr28575749vso.105.1564148403496; Fri, 26 Jul 2019 06:40:03 -0700 (PDT) MIME-Version: 1.0 References: <20190711051532.21054-1-vattunuru@marvell.com> <20190712064320.8106-1-vattunuru@marvell.com> In-Reply-To: <20190712064320.8106-1-vattunuru@marvell.com> From: David Marchand Date: Fri, 26 Jul 2019 15:39:52 +0200 Message-ID: To: Vamsi Attunuru Cc: dev , Thomas Monjalon , Jerin Jacob Kollanukkaran , Aaron Conole Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v3 1/1] app/test: fix --socket-mem option in eal flag autotest 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" Hello, On Fri, Jul 12, 2019 at 8:43 AM wrote: > > From: Vamsi Attunuru > > eal flag autotest fails when multiple mem size flags are passed > to --socket-mem option irrespective of RTE_MAX_NUMA_NODES. > > Patch fixes --socket-mem option by setting enough numbers > of numa node mem flags based on RTE_MAX_NUMA_NODES config. > > Fixes: 45f1b6e8680a ("app: add new tests on eal flags") > > Signed-off-by: Vamsi Attunuru > --- > V3 changes: > * Pass buffer size info to populate socket mem routine which > strlcpy requires. > > V2 changes: > * Add routine to populate --socket-mem option and use it to form > valid and invalid test commands. > > app/test/test_eal_flags.c | 121 +++++++++++++++++++++++++++------------------- > 1 file changed, 71 insertions(+), 50 deletions(-) > > diff --git a/app/test/test_eal_flags.c b/app/test/test_eal_flags.c > index 672ca0a..053d694 100644 > --- a/app/test/test_eal_flags.c > +++ b/app/test/test_eal_flags.c > @@ -1250,12 +1250,46 @@ test_file_prefix(void) > return 0; > } > > +static void > +populate_socket_mem_param(int num_sockets, const char *mem, > + char *buf, size_t buf_size) > +{ > + char tmp[SOCKET_MEM_STRLEN]; You don't need this temp buffer. > + int i; > + > + snprintf(buf, SOCKET_MEM_STRLEN, "--socket-mem="); buf is buf_size large, not SOCKET_MEM_STRLEN. > + > + for (i = 0; i < num_sockets ; i++) { > + snprintf(tmp, sizeof(tmp), "%s%s", buf, mem); > + strlcpy(buf, tmp, buf_size); > + > + if (num_sockets + 1 - i > 1) { > + snprintf(tmp, sizeof(tmp), "%s,", buf); > + strlcpy(buf, tmp, buf_size); > + } > + } > +} > + How about the following: /* This function writes in passed buf pointer a valid --socket-mem= option * for num_sockets then concatenates the provided suffix string. * * Example for num_sockets 4, mem "2", suffix "plop" * --socket-mem=2,2,2,2plop */ static void populate_socket_mem_param(int num_sockets, const char *mem, const char *suffix, char *buf, size_t buf_size) { unsigned int offset = 0; int written; int i; written = snprintf(&buf[offset], buf_size - offset, "--socket-mem="); if (written < 0 || written + offset >= buf_size) return; offset += written; for (i = 0; i < num_sockets - 1; i++) { written = snprintf(&buf[offset], buf_size - offset, "%s,", mem); if (written < 0 || written + offset >= buf_size) return; offset += written; } written = snprintf(&buf[offset], buf_size - offset, "%s%s", mem, suffix); if (written < 0 || written + offset >= buf_size) return; offset += written; } See below for usage. > /* > * Tests for correct handling of -m and --socket-mem flags > */ > static int > test_memory_flags(void) > { > + char buf[SOCKET_MEM_STRLEN]; /* to avoid copying string onto itself */ You don't need this temp buffer. > + > +#ifdef RTE_EXEC_ENV_FREEBSD > + int num_sockets = 1; > +#else > + int num_sockets = RTE_MIN(get_number_of_sockets(), > + RTE_MAX_NUMA_NODES); > +#endif > + > + if (num_sockets <= 0) { > + printf("Error - cannot get number of sockets!\n"); > + return -1; > + } > + > #ifdef RTE_EXEC_ENV_FREEBSD > /* BSD target doesn't support prefixes at this point */ > const char * prefix = ""; > @@ -1277,85 +1311,72 @@ test_memory_flags(void) > "--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE}; > > /* valid (zero) --socket-mem flag */ > + char arg2_socket_mem[SOCKET_MEM_STRLEN]; > + populate_socket_mem_param(num_sockets - 1, "0", buf, sizeof(buf)); > + snprintf(arg2_socket_mem, sizeof(arg2_socket_mem), "%s%s", buf, "0"); With the proposed populate_socket_mem_param, it becomes: populate_socket_mem_param(num_sockets, "0", "", arg2_socket_mem, sizeof(arg2_socket_mem)); > const char *argv2[] = {prgname, > - "--file-prefix=" memtest, "--socket-mem=0,0,0,0"}; > + "--file-prefix=" memtest, arg2_socket_mem}; > > /* invalid (incomplete) --socket-mem flag */ > + char arg3_socket_mem[SOCKET_MEM_STRLEN]; > + populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf)); What happens when num_socket == 1? With the proposed populate_socket_mem_param, it would become: populate_socket_mem_param(num_sockets - 1, "2", ",", arg3_socket_mem, sizeof(arg3_socket_mem)); But we need to ensure that we have at least two sockets before starting the test on argv3. > + snprintf(arg3_socket_mem, sizeof(arg3_socket_mem), "%s%s", buf, "2,"); > const char *argv3[] = {prgname, > - "--file-prefix=" memtest, "--socket-mem=2,2,"}; > + "--file-prefix=" memtest, arg3_socket_mem}; > > /* invalid (mixed with invalid data) --socket-mem flag */ > + char arg4_socket_mem[SOCKET_MEM_STRLEN]; > + populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf)); > + snprintf(arg4_socket_mem, sizeof(arg4_socket_mem), "%s%s", buf, "Fred"); > const char *argv4[] = {prgname, > - "--file-prefix=" memtest, "--socket-mem=2,2,Fred"}; > + "--file-prefix=" memtest, arg4_socket_mem}; Idem argv3. > > /* invalid (with numeric value as last character) --socket-mem flag */ > + char arg5_socket_mem[SOCKET_MEM_STRLEN]; > + populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf)); > + snprintf(arg5_socket_mem, sizeof(arg5_socket_mem), > + "%s%s", buf, "Fred0"); > const char *argv5[] = {prgname, > - "--file-prefix=" memtest, "--socket-mem=2,2,Fred0"}; > + "--file-prefix=" memtest, arg5_socket_mem}; Idem argv3. > > /* invalid (with empty socket) --socket-mem flag */ > + char arg6_socket_mem[SOCKET_MEM_STRLEN]; > + populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf)); > + snprintf(arg6_socket_mem, sizeof(arg6_socket_mem), "%s%s", buf, ",,"); > const char *argv6[] = {prgname, > - "--file-prefix=" memtest, "--socket-mem=2,,2"}; > + "--file-prefix=" memtest, arg6_socket_mem}; Here, this test requires at least 3 sockets so that we can test at minimum 2,,2. > > /* invalid (null) --socket-mem flag */ > const char *argv7[] = {prgname, > "--file-prefix=" memtest, "--socket-mem="}; > > /* valid --socket-mem specified together with -m flag */ > + char arg8_socket_mem[SOCKET_MEM_STRLEN]; > + populate_socket_mem_param(num_sockets - 1, "2", buf, sizeof(buf)); > + snprintf(arg8_socket_mem, sizeof(arg8_socket_mem), "%s%s", buf, "2"); > const char *argv8[] = {prgname, > - "--file-prefix=" memtest, "-m", DEFAULT_MEM_SIZE, "--socket-mem=2,2"}; > + "--file-prefix=" memtest, "-m", > + DEFAULT_MEM_SIZE, arg8_socket_mem}; > > /* construct an invalid socket mask with 2 megs on each socket plus > * extra 2 megs on socket that doesn't exist on current system */ > char invalid_socket_mem[SOCKET_MEM_STRLEN]; > - char buf[SOCKET_MEM_STRLEN]; /* to avoid copying string onto itself */ > - > -#ifdef RTE_EXEC_ENV_FREEBSD > - int i, num_sockets = 1; > -#else > - int i, num_sockets = RTE_MIN(get_number_of_sockets(), > - RTE_MAX_NUMA_NODES); > -#endif > - > - if (num_sockets <= 0) { > - printf("Error - cannot get number of sockets!\n"); > - return -1; > - } > - > - snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), "--socket-mem="); > - > - /* add one extra socket */ > - for (i = 0; i < num_sockets + 1; i++) { > - snprintf(buf, sizeof(buf), "%s%s", invalid_socket_mem, DEFAULT_MEM_SIZE); > - strlcpy(invalid_socket_mem, buf, sizeof(invalid_socket_mem)); > - > - if (num_sockets + 1 - i > 1) { > - snprintf(buf, sizeof(buf), "%s,", invalid_socket_mem); > - strlcpy(invalid_socket_mem, buf, > - sizeof(invalid_socket_mem)); > - } > - } > - > - /* construct a valid socket mask with 2 megs on each existing socket */ > - char valid_socket_mem[SOCKET_MEM_STRLEN]; > - > - snprintf(valid_socket_mem, sizeof(valid_socket_mem), "--socket-mem="); > - > - /* add one extra socket */ > - for (i = 0; i < num_sockets; i++) { > - snprintf(buf, sizeof(buf), "%s%s", valid_socket_mem, DEFAULT_MEM_SIZE); > - strlcpy(valid_socket_mem, buf, sizeof(valid_socket_mem)); > - > - if (num_sockets - i > 1) { > - snprintf(buf, sizeof(buf), "%s,", valid_socket_mem); > - strlcpy(valid_socket_mem, buf, > - sizeof(valid_socket_mem)); > - } > - } > + populate_socket_mem_param(num_sockets, DEFAULT_MEM_SIZE, > + buf, sizeof(buf)); > + snprintf(invalid_socket_mem, sizeof(invalid_socket_mem), > + "%s%s", buf, DEFAULT_MEM_SIZE); The comment says you want to generate a string with 2M per socket, not DEFAULT_MEM_SIZE. > > /* invalid --socket-mem flag (with extra socket) */ > const char *argv9[] = {prgname, > "--file-prefix=" memtest, invalid_socket_mem}; > > + /* construct a valid socket mask with 2 megs on each existing socket */ > + char valid_socket_mem[SOCKET_MEM_STRLEN]; > + populate_socket_mem_param(num_sockets - 1, DEFAULT_MEM_SIZE, > + buf, sizeof(buf)); > + snprintf(valid_socket_mem, sizeof(valid_socket_mem), > + "%s%s", buf, DEFAULT_MEM_SIZE); > + Idem 2M, not DEFAULT_MEM_SIZE. > /* valid --socket-mem flag */ > const char *argv10[] = {prgname, > "--file-prefix=" memtest, valid_socket_mem}; > -- > 2.8.4 > I prepared a patch with my comments addressed, can you have a look ? https://github.com/david-marchand/dpdk/commit/a78b7572a402579d775f1d0622d1454eb36e0ad4 Thanks. -- David Marchand