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 C380AA04DB; Fri, 16 Oct 2020 19:00:46 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 88DB39AEB; Fri, 16 Oct 2020 19:00:44 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 442699A89 for ; Fri, 16 Oct 2020 19:00:41 +0200 (CEST) IronPort-SDR: VowBSRubem3GgdoEP6c2AEGaDe/O9vv5daj9XPE+PFPZU09qm7VzYw6lB9QwV5YRDAK/r0FR8Y jWCF0WtvIN5w== X-IronPort-AV: E=McAfee;i="6000,8403,9776"; a="145941500" X-IronPort-AV: E=Sophos;i="5.77,383,1596524400"; d="scan'208";a="145941500" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2020 08:57:21 -0700 IronPort-SDR: ADkNvLe/L6C/YO6OOylaLkPv4EaUPD6et2Sx7AY8aSkUi9TalP1us9hRZhvq8hnLiXqc3y/gfO Tov/glfe5k+A== X-IronPort-AV: E=Sophos;i="5.77,383,1596524400"; d="scan'208";a="531767192" 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:57:18 -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> <53e6ccf1-afbf-c297-82fe-dd5557049b21@intel.com> From: Ferruh Yigit Message-ID: <432371f7-25dc-20f9-e783-437190059d6f@intel.com> Date: Fri, 16 Oct 2020 16:57:17 +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:55 PM, Slava Ovsiienko wrote: >> -----Original Message----- >> From: Ferruh Yigit >> Sent: Friday, October 16, 2020 18:52 >> 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: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"? > It is place in the pool structure, so it is an object name > and tightly coupled with the pool. OK, let's try "mb_pool" ? > sounds good