From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 432C42BD8 for ; Fri, 26 Feb 2016 02:42:22 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 25 Feb 2016 17:42:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,498,1449561600"; d="scan'208";a="753470378" Received: from shwdeisgchi083.ccr.corp.intel.com (HELO [10.239.67.193]) ([10.239.67.193]) by orsmga003.jf.intel.com with ESMTP; 25 Feb 2016 17:42:19 -0800 To: "Ananyev, Konstantin" , "dev@dpdk.org" References: <1451544799-70776-1-git-send-email-jianfeng.tan@intel.com> <1456386842-112571-1-git-send-email-jianfeng.tan@intel.com> <1456386842-112571-2-git-send-email-jianfeng.tan@intel.com> <2601191342CEEE43887BDE71AB97725836B0B4F8@irsmsx105.ger.corp.intel.com> <56CF2D81.2090904@intel.com> <2601191342CEEE43887BDE71AB97725836B0B5DE@irsmsx105.ger.corp.intel.com> From: "Tan, Jianfeng" Message-ID: <56CFAD7B.8030300@intel.com> Date: Fri, 26 Feb 2016 09:42:19 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <2601191342CEEE43887BDE71AB97725836B0B5DE@irsmsx105.ger.corp.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 01/12] ethdev: add API to query packet type filling info X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 26 Feb 2016 01:42:22 -0000 Hi Konstantin, On 2/26/2016 1:16 AM, Ananyev, Konstantin wrote: > >> -----Original Message----- >> From: Tan, Jianfeng >> Sent: Thursday, February 25, 2016 4:36 PM >> To: Ananyev, Konstantin; dev@dpdk.org >> Cc: Zhang, Helin; nelio.laranjeiro@6wind.com; adrien.mazarguil@6wind.com; rahul.lakkireddy@chelsio.com >> Subject: Re: [PATCH v3 01/12] ethdev: add API to query packet type filling info >> >> Hi Konstantin, >> >> On 2/25/2016 11:46 PM, Ananyev, Konstantin wrote: >>> Hi Jainfeng, >>> >>>> Add a new API rte_eth_dev_get_ptype_info to query whether/what packet >>>> type can be filled by given pmd rx burst function. >>>> >>>> Signed-off-by: Jianfeng Tan >>>> --- >>>> lib/librte_ether/rte_ethdev.c | 32 ++++++++++++++++++++++++++++++++ >>>> lib/librte_ether/rte_ethdev.h | 23 +++++++++++++++++++++++ >>>> 2 files changed, 55 insertions(+) >>>> >>>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c >>>> index 1257965..b52555b 100644 >>>> --- a/lib/librte_ether/rte_ethdev.c >>>> +++ b/lib/librte_ether/rte_ethdev.c >>>> @@ -1576,6 +1576,38 @@ rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info) >>>> dev_info->driver_name = dev->data->drv_name; >>>> } >>>> >>>> +int >>>> +rte_eth_dev_get_ptype_info(uint8_t port_id, uint32_t ptype_mask, >>>> + uint32_t **p_ptypes) >>>> +{ >>>> + int i, j, ret; >>>> + struct rte_eth_dev *dev; >>>> + const uint32_t *all_ptypes; >>>> + >>>> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >>>> + dev = &rte_eth_devices[port_id]; >>>> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_ptype_info_get, -ENOTSUP); >>>> + all_ptypes = (*dev->dev_ops->dev_ptype_info_get)(dev); >>>> + >>>> + if (!all_ptypes) >>>> + return 0; >>>> + >>>> + for (i = 0, ret = 0; all_ptypes[i] != RTE_PTYPE_UNKNOWN; ++i) >>>> + if (all_ptypes[i] & ptype_mask) >>>> + ret++; >>>> + if (ret == 0) >>>> + return 0; >>>> + >>>> + *p_ptypes = (uint32_t *)malloc(sizeof(uint32_t) * ret); >>>> + if (*p_ptypes == NULL) >>>> + return -ENOMEM; >>> I thought we all agreed to follow snprintf()-like logic and avoid any memory allocations inside that function. >>> So why malloc() again? >>> Konstantin >>> >> Sorry for the setback. I still have concerns that snprintf()-like needs >> to be called twice by an application to get the ptype info. So I write >> this implementation for you to comment. >> >> So what's the reason why we should avoid memory allocations inside that >> function? > By the same reason none of other rte_ethdev get_info() style functions > (rte_eth_dev_info_get, rte_eth_rx_queue_info_get, rte_eth_xstats_get, > rte_eth_dev_rss_reta_query, etc) allocate space themselves. > It is a good practice to let user to decide himself how/where to allocate/free a space for that date: > on a stack, inside a data segment (global variable), on heap (malloc), > at hugepages via rte_malloc, somewhere else. > > BTW, if you had concerns about that approach, why didn't you provide any arguments > when it was discussed/agreed? > Instead you came up a month later with same old approach that voids my and other > reviewers comments and even v2 of your own patch. > Do you have any good reason for that? > > Konstantin > That makes sense for me now. Thanks for explanation. Will send out v4 as previous discussion result. I'd like to say sorry for this delayed and wrong way to raise concern. Sorry for waste of your time. Appreciate your comments on this. Thanks, Jianfeng