From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f47.google.com (mail-wm0-f47.google.com [74.125.82.47]) by dpdk.org (Postfix) with ESMTP id 996D8594B for ; Fri, 15 Jan 2016 16:33:24 +0100 (CET) Received: by mail-wm0-f47.google.com with SMTP id f206so30192475wmf.0 for ; Fri, 15 Jan 2016 07:33:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-type:content-disposition:in-reply-to; bh=CkpS9BjheT9JrNwqZGEgr1KQOMYN/FpjtfTln88DMcE=; b=QcujhgUebYGTdfIIoMhI9DIeSmRf+nMURIONYRpjrigKk2u4glaeaobfMf8rYPXgLr dUsoAPS1gQYtAvAIhE/WqWeiC3+pYJAbDzUSSiSfEn/T7JFv9+tfmlIiYUKQgFKFYy4n 55PyJR/HBZFZEfPGG+2DAJJTGsVdBtwq/YqDMjjz6pvdyICXjLS4jpsuEY0Uodo/aY/U Mf7tqEAyvBg5gE3++1ifMlj2ssrbX/xTPo3ZbODu0P2zAgLFIw6K0bB3fW/PQOfHMw2v dqR840XueG8FF9xsJD63KEau/1Icxgn9FI+sySDpGkPSDvkIJmpJZ4dq7EKxUu4QUL9i xlDg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-type :content-disposition:in-reply-to; bh=CkpS9BjheT9JrNwqZGEgr1KQOMYN/FpjtfTln88DMcE=; b=ASeUOI5ZRr8xjwcWicIMjhTtybBY3RFEOZLNSwnZTC7io2ZOwJiimfYTSep41zaPkJ 35GDLoAKrvXcqiEeE4LlnPYKlkk8+LwQbuiBt6CI5yBG9T1bHMxzNrpX0YOnPjaTGEJu 4jF3OO4ofZTpp40Az0KpWtREMMr2MKtFPHzCcgoReO0IaUw3W52acO/P889oEgtQzv/r E6HdCzZw6c69SMkrEQ72g69STTw54AHIUWlEFeTWvgdQnWMXuNS5t+Li1hkRf4In+m8J s+SGzqZJ9rt0rDOKThoBtpsOHEuulXYCs1BpW0nV34jYnPfyIfj3Etv6h9ZK9eee2FLm ZX7A== X-Gm-Message-State: ALoCoQlWB60HXLJM842k68OErs99D625F7ByMebkUJUsnnoFi60L4KdERgtrDmgOduu51iavsNP/8KBk9/HjtqjTw4NaKF4ZNg== X-Received: by 10.194.201.134 with SMTP id ka6mr10697916wjc.116.1452872004205; Fri, 15 Jan 2016 07:33:24 -0800 (PST) Received: from 6wind.com (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by smtp.gmail.com with ESMTPSA id qs1sm11107512wjc.2.2016.01.15.07.33.18 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 15 Jan 2016 07:33:20 -0800 (PST) Date: Fri, 15 Jan 2016 16:33:01 +0100 From: Adrien Mazarguil To: "Ananyev, Konstantin" Message-ID: <20160115153301.GR12095@6wind.com> Mail-Followup-To: "Ananyev, Konstantin" , "Tan, Jianfeng" , "dev@dpdk.org" References: <1451544799-70776-1-git-send-email-jianfeng.tan@intel.com> <1452836759-63540-1-git-send-email-jianfeng.tan@intel.com> <1452836759-63540-2-git-send-email-jianfeng.tan@intel.com> <20160115135846.GO12095@6wind.com> <2601191342CEEE43887BDE71AB97725836AE619F@irsmsx105.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2601191342CEEE43887BDE71AB97725836AE619F@irsmsx105.ger.corp.intel.com> Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2 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, 15 Jan 2016 15:33:24 -0000 On Fri, Jan 15, 2016 at 03:11:18PM +0000, Ananyev, Konstantin wrote: > Hi lads, > > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil > > Sent: Friday, January 15, 2016 1:59 PM > > To: Tan, Jianfeng > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v2 01/12] ethdev: add API to query packet type filling info > > > > Hi Jianfeng, a few comments below. > > > > On Fri, Jan 15, 2016 at 01:45:48PM +0800, Jianfeng Tan wrote: > > > Add a new API rte_eth_dev_get_ptype_info to query wether/what packet type will > > > be filled by given pmd rx burst function. > > > > > > Signed-off-by: Jianfeng Tan > > > --- > > > lib/librte_ether/rte_ethdev.c | 20 ++++++++++++++++++++ > > > lib/librte_ether/rte_ethdev.h | 27 +++++++++++++++++++++++++++ > > > lib/librte_mbuf/rte_mbuf.h | 6 ++++++ > > > 3 files changed, 53 insertions(+) > > > > > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c > > > index ed971b4..cd34f46 100644 > > > --- a/lib/librte_ether/rte_ethdev.c > > > +++ b/lib/librte_ether/rte_ethdev.c > > > @@ -1614,6 +1614,26 @@ 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 ptypes[], int num) > > > +{ > > > + int ret, i, j; > > > + struct rte_eth_dev *dev; > > > + uint32_t all_ptypes[RTE_PTYPE_MAX_NUM]; > > > + > > > + 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); > > > + ret = (*dev->dev_ops->dev_ptype_info_get)(dev, all_ptypes); > > > + > > > + for (i = 0, j = 0; i < ret && j < num; ++i) > > > + if (all_ptypes[i] & ptype_mask) > > > + ptypes[j++] = all_ptypes[i]; > > > + > > > + return ret; > > > +} > > > > It's a good thing that the size of ptypes[] can be provided, but I think num > > should be passed to the dev_ptype_info_get() callback as well. > > > > If you really do not want to pass the size, you have to force the array type > > size onto callbacks using a pointer to the array otherwise they look unsafe > > (and are actually unsafe when not called from the rte_eth_dev wrapper), > > something like this: > > > > int (*dev_ptype_info_get)(uint8_t port_id, uint32_t (*ptypes)[RTE_PTYPE_MAX_NUM]); > > > > In which case you might as well drop the num argument from > > rte_eth_dev_get_ptype_info() to use the same syntax. That way there is no > > need to allocate a ptypes array on the stack twice. > > > > But since people usually do not like this syntax, I think passing num and > > letting callbacks check for overflow themselves on the user-provided ptypes > > array directly is better. They have to return the total number of packet > > types supported even when num is 0 (ptypes may be NULL in that case). > > > > I understand the result needs to be temporarily stored somewhere for > > filtering and for that purpose the entire size must be known in advance, > > hence my previous suggestion for rte_eth_dev_get_ptype_info() to return > > the total number of ptypes and providing a separate function for filtering > > a ptypes array for applications that need it: > > > > /* Return remaining number entries in ptypes[] after filtering it > > * according to ptype_mask. */ > > int rte_eth_dev_ptypes_filter(uint32_t ptype_mask, uint32_t ptypes[], int num); > > > > Usage would be like: > > > > int ptypes_num = rte_eth_dev_get_ptype_info(42, NULL, 0); > > > > if (ptypes_num <= 0) > > goto err; > > > > uint32_t ptypes[ptypes_num]; > > > > rte_eth_dev_get_ptype_info(42, ptypes, ptypes_num); > > ptypes_num = rte_eth_dev_ptypes_filter(RTE_PTYPE_INNER_L4_MASK, ptypes, ptypes_num); > > > > if (ptypes_num <= 0) > > goto err; > > > > /* process ptypes... */ > > > > What about this? > > Actually while thinking about it, we probably can make: > const uint32_t * (*dev_ptype_info_get)(uint8_t port_id); > So PMD return to ethdev layer a pointer to a constant array of supported ptypes, > terminated by RTE_PTYPE_UNKNOWN. > > Then rte_eth_dev_get_ptype_info() will iterate over it, and fill array provided by the user. > > all_pytpes = (*dev->dev_ops->dev_ptype_info_get)(dev); > for (i = 0; all_ptypes[i] != RTE_PTYPE_UNKNOWN; i++) { > if (all_ptypes[i] & ptype_mask) { > if (j < num) > ptypes[j] = all_ptypes[i]; > j++; > } > return j; Looks like a much simpler and better approach, that's the implementation we need! (ignore my overengineered blob above) -- Adrien Mazarguil 6WIND