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 3D3DDA04DB; Fri, 16 Oct 2020 17:52:46 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 81C841EBB5; Fri, 16 Oct 2020 17:52:28 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id D188C1EBAD for ; Fri, 16 Oct 2020 17:52:26 +0200 (CEST) IronPort-SDR: jNKwSMKN1JV0WyxMsiqGJfsT/RnSeguvUhdocHrdbUTS0Nl6lPwFYfhFNfmPqpoaCIkxoyr1rn Zqhre3WuDUQw== X-IronPort-AV: E=McAfee;i="6000,8403,9776"; a="251328854" X-IronPort-AV: E=Sophos;i="5.77,383,1596524400"; d="scan'208";a="251328854" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2020 08:52:24 -0700 IronPort-SDR: PrgfIJXbkNUVAO1ji59uvuifWBIa4BQWhAUJxut7CYdbTpkCsRQBDe51BGcNseCigQ0dh8/A3e P4zRoq+lCpoQ== X-IronPort-AV: E=Sophos;i="5.77,383,1596524400"; d="scan'208";a="531763886" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.252.19.66]) ([10.252.19.66]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2020 08:52:21 -0700 To: Slava Ovsiienko , "dev@dpdk.org" Cc: NBU-Contact-Thomas Monjalon , "stephen@networkplumber.org" , "olivier.matz@6wind.com" , "jerinjacobk@gmail.com" , "maxime.coquelin@redhat.com" , "david.marchand@redhat.com" , "arybchenko@solarflare.com" References: <1602855582-15847-1-git-send-email-viacheslavo@nvidia.com> <1602855582-15847-3-git-send-email-viacheslavo@nvidia.com> <5c7edb1c-2fe0-b98e-b0d0-a12052b59f09@intel.com> <11dd71b9-0e5f-6885-c8b1-abe0bfd28315@intel.com> From: Ferruh Yigit Message-ID: <53e6ccf1-afbf-c297-82fe-dd5557049b21@intel.com> Date: Fri, 16 Oct 2020 16:52:18 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v11 2/6] app/testpmd: add multiple pools per core creation 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 10/16/2020 4:48 PM, Slava Ovsiienko wrote: >> -----Original Message----- >> From: Ferruh Yigit >> Sent: Friday, October 16, 2020 18:39 >> To: Slava Ovsiienko ; dev@dpdk.org >> Cc: NBU-Contact-Thomas Monjalon ; >> stephen@networkplumber.org; olivier.matz@6wind.com; >> jerinjacobk@gmail.com; maxime.coquelin@redhat.com; >> david.marchand@redhat.com; arybchenko@solarflare.com >> Subject: Re: [dpdk-dev] [PATCH v11 2/6] app/testpmd: add multiple pools per >> core creation >> >> On 10/16/2020 4:05 PM, Ferruh Yigit wrote: >>> On 10/16/2020 2:39 PM, Viacheslav Ovsiienko wrote: >>>> The command line parameter --mbuf-size is updated, it can handle the >>>> multiple values like the following: >>>> >>>> --mbuf-size=2176,512,768,4096 >>>> >>>> specifying the creation the extra memory pools with the requested >>>> mbuf data buffer sizes. If some buffer split feature is engaged the >>>> extra memory pools can be used to configure the Rx queues with >>>> rte_the_dev_rx_queue_setup_ex(). >>>> >>>> The extra pools are created with requested sizes, and pool names are >>>> assigned with appended index: mbuf_pool_socket_%socket_%index. >>>> Index zero is used to specify the first mandatory pool to maintain >>>> compatibility with existing code. >>>> >>>> Signed-off-by: Viacheslav Ovsiienko >>> >>> <...> >>> >>>>   /* Mbuf Pools */ >>>>   static inline void >>>> -mbuf_poolname_build(unsigned int sock_id, char* mp_name, int >>>> name_size) >>>> +mbuf_poolname_build(unsigned int sock_id, char *mp_name, >>>> +            int name_size, unsigned int idx) >>>>   { >>>> -    snprintf(mp_name, name_size, "mbuf_pool_socket_%u", sock_id); >>>> +    if (!idx) >>>> +        snprintf(mp_name, name_size, "mbuf_pool_socket_%u", >>>> +sock_id); >>>> +    else >>>> +        snprintf(mp_name, name_size, "mbuf_pool_socket_%u_%u", >>>> +             sock_id, idx); >>> >>> 'mp_name' can theoretically overflow and gives a compiler warning, >>> although not sure if this truncation is a problem in practice. >>> >>> ../app/test-pmd/testpmd.c: In function ‘rx_queue_setup’: >>> ../app/test-pmd/testpmd.h:666:53: error: ‘%u’ directive output may be >>> truncated writing between 1 and 10 bytes into a region of size between >>> 0 and 7 [-Werror=format-truncation=] >>>  666 |   snprintf(mp_name, name_size, "mbuf_pool_socket_%u_%u", >>>      |                                                     ^~ >>> ../app/test-pmd/testpmd.h:666:32: note: directive argument in the >>> range [1, 4294967295] >>>   666 |   snprintf(mp_name, name_size, "mbuf_pool_socket_%u_%u", >>>       |                                ^~~~~~~~~~~~~~~~~~~~~~~~ >>> ../app/test-pmd/testpmd.h:666:3: note: ‘snprintf’ output between 21 >>> and 39 bytes into a destination of size 26 >>>   666 |   snprintf(mp_name, name_size, "mbuf_pool_socket_%u_%u", >>>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>   667 |     sock_id, idx); >>>       |     ~~~~~~~~~~~~~ >>> >>> Any suggestion for fix? Can we shorten the string above, is it used >>> somewhere else? Or casting variables to a smaller size may work too.. >> >> What do you think to following: >> >> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h >> index 4cd0f967f0..4ac1c1f86e 100644 >> --- a/app/test-pmd/testpmd.h >> +++ b/app/test-pmd/testpmd.h >> @@ -661,10 +661,11 @@ mbuf_poolname_build(unsigned int sock_id, char >> *mp_name, >> int name_size, unsigned int idx) >> { >> if (!idx) >> - snprintf(mp_name, name_size, "mbuf_pool_socket_%u", sock_id); >> + snprintf(mp_name, name_size, "mbuf_pool_s%u", >> + (uint16_t)sock_id); >> else >> - snprintf(mp_name, name_size, "mbuf_pool_socket_%u_%u", >> - sock_id, idx); >> + snprintf(mp_name, name_size, "mbuf_pool_s%u_%u", >> + (uint16_t)sock_id, (uint16_t)idx); >> } >> >> static inline struct rte_mempool * > > Testpmd build mbuf pool names with mbuf_poolname_build() routine only > (invoked from mbuf_pool_find/mbuf_pool_create). I suppose it is safe > to replace "mbuf_pool_socket_" prefix with shorter one. > > Say something like this: #define TESTPMD_MBUF_POOL_PFX "mbuf" > Just 'mbuf' may not give enough context in the logs, what about "mbuf_pool_%u_%u"? > Truncating idx to uint16_t is OK, let me update the types of routines. > sock_id seems to be OK either. If you agree on both - please, let me update the patch. > If variables are not truncated, it leaves very small place for prefix, 3 chars if I calculate correctly, so it seems we need to truncate it anyway.