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 B031E1B3BA for ; Tue, 3 Oct 2017 19:54:33 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Oct 2017 10:54:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,474,1500966000"; d="scan'208";a="906316152" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.224.245]) ([10.241.224.245]) by FMSMGA003.fm.intel.com with ESMTP; 03 Oct 2017 10:54:32 -0700 To: Wei Zhao , dev@dpdk.org References: <1506672718-39160-1-git-send-email-wei.zhao1@intel.com> <1506676584-41030-1-git-send-email-wei.zhao1@intel.com> <1506676584-41030-2-git-send-email-wei.zhao1@intel.com> From: Ferruh Yigit Message-ID: Date: Tue, 3 Oct 2017 18:54:31 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <1506676584-41030-2-git-send-email-wei.zhao1@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v7 1/2] net/i40e: queue region set and flush 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: , X-List-Received-Date: Tue, 03 Oct 2017 17:54:34 -0000 On 9/29/2017 10:16 AM, Wei Zhao wrote: > This feature enable queue regions configuration for RSS in PF, > so that different traffic classes or different packet > classification types can be separated to different queues in > different queue regions.This patch can set queue region range, > it include queue number in a region and the index of first queue. > This patch enable mapping between different priorities (UP) and > different traffic classes.It also enable mapping between a region > index and a sepcific flowtype(PCTYPE).It also provide the solution > of flush all configuration about queue region the above described. > > Signed-off-by: Wei Zhao > --- <...> > @@ -541,6 +547,34 @@ struct i40e_ethertype_rule { > struct rte_hash *hash_table; > }; > > +/* queue region info */ > +struct i40e_region_info { > + /* the region id for this configuration */ > + uint8_t region_id; > + /* the start queue index for this region */ > + uint8_t queue_start_index; > + /* the total queue number of this queue region */ > + uint8_t queue_num; > + /* the total number of user priority for this region */ > + uint8_t user_priority_num; > + /* the packet's user priority for this region */ > + uint8_t user_priority[I40E_MAX_USER_PRIORITY]; > + /* the total number of flowtype for this region */ > + uint8_t flowtype_num; > + /** > + * the pctype or hardware flowtype of packet, > + * the specific index for each type has been defined > + * in file i40e_type.h as enum i40e_filter_pctype. > + */ > + uint8_t hw_flowtype[I40E_FILTER_PCTYPE_MAX]; > +}; > + > +struct i40e_queue_region_info { What do you think renaming: i40e_region_info -> i40e_queue_region_info i40e_queue_region_info -> i40e_queue_regions > + /* the total number of queue region for this port */ > + uint16_t queue_region_number; > + struct i40e_region_info region[I40E_REGION_MAX_INDEX + 1]; > +}; > + > /* Tunnel filter number HW supports */ > #define I40E_MAX_TUNNEL_FILTER_NUM 400 > <...> > +static int > +i40e_queue_region_set_flowtype(struct i40e_pf *pf, > + struct rte_i40e_rss_region_conf *rss_region_conf) > +{ > + int32_t ret = -EINVAL; > + struct i40e_queue_region_info *info = &pf->queue_region; > + uint16_t i, j; > + uint16_t region_index, flowtype_index; > + > + /** This is doxygen sytle comment, only nedeed for public headers. > + * For the pctype or hardware flowtype of packet, > + * the specific index for each type has been defined > + * in file i40e_type.h as enum i40e_filter_pctype. > + */ > + <...> > + } > + } > +} Missing line. > +static int > +i40e_queue_region_set_user_priority(struct i40e_pf *pf, > + struct rte_i40e_rss_region_conf *rss_region_conf) > +{ > + struct i40e_queue_region_info *info = &pf->queue_region; > + int32_t ret = -EINVAL; > + uint16_t i, j, region_index; > + <...> > +static int > +i40e_queue_region_display_all_info(struct i40e_pf *pf, uint16_t port_id) > +{ > + uint16_t i, j; > + struct i40e_queue_region_info *info = &pf->queue_region; > + static const char *queue_region_info_stats_border = "-------"; > + > + if (!info->queue_region_number) > + PMD_DRV_LOG(ERR, "there is no has been region set before"); > + > + printf("\n %s All queue region info for port=%2d %s", > + queue_region_info_stats_border, port_id, > + queue_region_info_stats_border); printf ? Please use driver logging functions. > + printf("\n queue_region_number: %-14u \n", info->queue_region_number); > + > + for (i = 0; i < info->queue_region_number; i++) { > + printf("\n region_id: %-14u queue_number: %-14u " > + "queue_start_index: %-14u \n", > + info->region[i].region_id, > + info->region[i].queue_num, > + info->region[i].queue_start_index); > + > + printf(" user_priority_num is %-14u :", > + info->region[i].user_priority_num); > + for (j = 0; j < info->region[i].user_priority_num; j++) > + printf(" %-14u ", info->region[i].user_priority[j]); > + > + printf("\n flowtype_num is %-14u :", > + info->region[i].flowtype_num); > + for (j = 0; j < info->region[i].flowtype_num; j++) > + printf(" %-14u ", info->region[i].hw_flowtype[j]); > + } > + > + printf("\n\n"); > + return 0; > +} > + > +int rte_pmd_i40e_rss_queue_region_conf(uint16_t port_id, > + struct rte_i40e_rss_region_conf *rss_region_conf) > +{ > + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > + struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > + struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); > + enum rte_pmd_i40e_queue_region_op op_type = rss_region_conf->op; > + int32_t ret; > + > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > + > + if (!is_i40e_supported(dev)) > + return -ENOTSUP; > + > + if (!(!i40e_queue_region_pf_check_rss(pf))) > + return -ENOTSUP; > + > + /** > + * This queue region feature only support pf by now. It should > + * be called after dev_start, and will be clear after dev_stop. > + * "RTE_PMD_I40E_RSS_QUEUE_REGION_ALL_FLUSH_ON" > + * is just an enable function which server for other configuration, > + * it is for all configuration about queue region from up layer, > + * at first will only keep in DPDK softwarestored in driver, > + * only after "FLUSH_ON", it commit all configuration to HW. > + * Because PMD had to set hardware configuration at a time, so > + * it will record all up layer command at first. > + * "RTE_PMD_I40E_RSS_QUEUE_REGION_ALL_FLUSH_OFF" is > + * just clean all configuration about queue region just now, > + * and restore all to DPDK i40e driver default > + * config when start up. > + */ > + > + switch (op_type) { > + case RTE_PMD_I40E_QUEUE_REGION_SET: > + ret = i40e_queue_region_set_region(pf, rss_region_conf); > + break; > + case RTE_PMD_I40E_REGION_FLOWTYPE_SET: > + ret = i40e_queue_region_set_flowtype(pf, rss_region_conf); > + break; > + case RTE_PMD_I40E_USER_PRIORITY_REGION_SET: > + ret = i40e_queue_region_set_user_priority(pf, rss_region_conf); > + break; > + case RTE_PMD_I40E_RSS_QUEUE_REGION_ALL_FLUSH_ON: > + ret = i40e_flush_queue_region_all_conf(dev, hw, pf, 1); > + break; > + case RTE_PMD_I40E_RSS_QUEUE_REGION_ALL_FLUSH_OFF: > + ret = i40e_flush_queue_region_all_conf(dev, hw, pf, 0); > + break; > + case RTE_PMD_I40E_RSS_QUEUE_REGION_INFO_GET: > + ret = i40e_queue_region_display_all_info(pf, port_id); Displaying is not always very useful in API level. What do you think get fills the rss_region_conf with current device settings and return back to the caller? > + break; > + default: > + PMD_DRV_LOG(WARNING, "op type (%d) not supported", > + op_type); > + ret = -EINVAL; > + } > + > + I40E_WRITE_FLUSH(hw); > + > + return ret; > +} > diff --git a/drivers/net/i40e/rte_pmd_i40e.h b/drivers/net/i40e/rte_pmd_i40e.h > index 155b7e8..2219318 100644 > --- a/drivers/net/i40e/rte_pmd_i40e.h > +++ b/drivers/net/i40e/rte_pmd_i40e.h > @@ -91,6 +91,33 @@ enum rte_pmd_i40e_package_info { > RTE_PMD_I40E_PKG_INFO_MAX = 0xFFFFFFFF > }; > > +/** > + * Option types of queue region. > + */ > +enum rte_pmd_i40e_queue_region_op { > + RTE_PMD_I40E_REGION_UNDEFINED, > + RTE_PMD_I40E_QUEUE_REGION_SET, /**< add queue region set */ > + RTE_PMD_I40E_REGION_FLOWTYPE_SET, /**< add pf region pctype set */ > + /*** add queue region user priority set */ > + RTE_PMD_I40E_USER_PRIORITY_REGION_SET, > + /** > + * ALL configuration about queue region from up layer > + * at first will only keep in DPDK softwarestored in driver, > + * only after " FLUSH_ON ", it commit all configuration to HW. > + * Because PMD had to set hardware configuration at a time, so > + * it will record all up layer command at first. > + */ > + RTE_PMD_I40E_RSS_QUEUE_REGION_ALL_FLUSH_ON, > + /** > + * "FLUSH_OFF " is just clean all configuration about queue > + * region just now, and restore all to DPDK i40e driver default > + * config when start up. > + */ > + RTE_PMD_I40E_RSS_QUEUE_REGION_ALL_FLUSH_OFF, > + RTE_PMD_I40E_RSS_QUEUE_REGION_INFO_GET, > + RTE_PMD_I40E_QUEUE_REGION_OP_MAX These are in public header now. Can you please use same namespace for all ops. It can be "RTE_PMD_I40E_RSS_QUEUE_REGION_" > +}; > + > #define RTE_PMD_I40E_DDP_NAME_SIZE 32 > > /** > @@ -146,6 +173,27 @@ struct rte_pmd_i40e_ptype_mapping { > }; > > /** > + * Queue region related information. > + */ > +struct rte_i40e_rss_region_conf { Can you please be consistant in using public names. There are: rss_region queue_region rss_queue_region Also there is confusion between using: rte_i40e_ rte_pmd_i40e_ > + /*** the region id for this configuration */ > + uint8_t region_id; > + /** the pctype or hardware flowtype of packet, > + * the specific index for each type has been defined > + * in file i40e_type.h as enum i40e_filter_pctype. > + */ > + uint8_t hw_flowtype; > + /*** the start queue index for this region */ > + uint8_t queue_start_index; > + /*** the total queue number of this queue region */ > + uint8_t queue_num; > + /*** the packet's user priority for this region */ > + uint8_t user_priority; > + /*** Option types of queue region */ > + enum rte_pmd_i40e_queue_region_op op; > +}; > + > +/** > * Notify VF when PF link status changes. > * > * @param port > @@ -657,4 +705,16 @@ int rte_pmd_i40e_ptype_mapping_replace(uint8_t port, > int rte_pmd_i40e_add_vf_mac_addr(uint8_t port, uint16_t vf_id, > struct ether_addr *mac_addr); > > +/** > + * Do RSS queue region configuration for that port as > + * the command option type > + * > + * @param port > + * pointer id for that port device port id ? > + * @param conf_ptr > + * pointer to the struct that contain all the > + * region configuration parameters > + */ > +int rte_pmd_i40e_rss_queue_region_conf(uint16_t port_id, By defining port_id uint16_t, this patches becomes dependent to the port_id update patchset, please clarify this in cover letter. > + struct rte_i40e_rss_region_conf *rss_region_conf); > #endif /* _PMD_I40E_H_ */ > diff --git a/drivers/net/i40e/rte_pmd_i40e_version.map b/drivers/net/i40e/rte_pmd_i40e_version.map > index ef8882b..29d6b74 100644 > --- a/drivers/net/i40e/rte_pmd_i40e_version.map > +++ b/drivers/net/i40e/rte_pmd_i40e_version.map > @@ -50,5 +50,6 @@ DPDK_17.11 { > global: > > rte_pmd_i40e_add_vf_mac_addr; > + rte_pmd_i40e_rss_queue_region_conf; > > } DPDK_17.08; >