From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 7A4761D90 for ; Thu, 23 Nov 2017 00:15:23 +0100 (CET) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Nov 2017 15:15:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,438,1505804400"; d="scan'208";a="179492134" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.224.250]) ([10.241.224.250]) by fmsmga006.fm.intel.com with ESMTP; 22 Nov 2017 15:15:20 -0800 To: Stephen Hemminger Cc: Jingjing Wu , dev@dpdk.org, wenzhuo.lu@intel.com References: <1508488012-82704-1-git-send-email-jingjing.wu@intel.com> <1508488012-82704-5-git-send-email-jingjing.wu@intel.com> <69d5b2a3-542b-0f01-f712-3f2a87e591e2@intel.com> <20171121165740.50686126@xeon-e3> From: Ferruh Yigit Message-ID: <3f0ad309-2393-93ce-2622-55165a029f5e@intel.com> Date: Wed, 22 Nov 2017 15:15:20 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171121165740.50686126@xeon-e3> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [RFC 4/9] net/avf: enable basic Rx Tx func 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: Wed, 22 Nov 2017 23:15:23 -0000 On 11/21/2017 4:57 PM, Stephen Hemminger wrote: > On Tue, 21 Nov 2017 16:06:24 -0800 > Ferruh Yigit wrote: > >> On 10/20/2017 1:26 AM, Jingjing Wu wrote: >>> Signed-off-by: Wenzhuo Lu >> >> <...> >> >>> @@ -214,6 +214,9 @@ CONFIG_RTE_LIBRTE_FM10K_INC_VECTOR=y >>> # Compile burst-oriented AVF PMD driver >>> # >>> CONFIG_RTE_LIBRTE_AVF_PMD=y >>> +CONFIG_RTE_LIBRTE_AVF_RX_DUMP=n >>> +CONFIG_RTE_LIBRTE_AVF_TX_DUMP=n >> >> Are these config options used? >> >> <...> >> >>> @@ -49,4 +49,18 @@ extern int avf_logtype_driver; >>> PMD_DRV_LOG_RAW(level, fmt "\n", ## args) >>> #define PMD_DRV_FUNC_TRACE() PMD_DRV_LOG(DEBUG, " >>") >>> >>> +#ifdef RTE_LIBRTE_AVF_DEBUG_TX >> >> Is this defined anywhere? >> >>> +#define PMD_TX_LOG(level, fmt, args...) \ >>> + RTE_LOG(level, PMD, "%s(): " fmt "\n", __func__, ## args) >> >> Instead should RTE_LOG_DP used? >> And since other macros uses dynamic log functions, why here use static method, >> what do you think using new method for data path logs as well? >> >> <...> >> >>> +static inline void >>> +avf_rxd_to_vlan_tci(struct rte_mbuf *mb, volatile union avf_rx_desc *rxdp) >>> +{ >>> + if (rte_le_to_cpu_64(rxdp->wb.qword1.status_error_len) & >>> + (1 << AVF_RX_DESC_STATUS_L2TAG1P_SHIFT)) { >>> + mb->ol_flags |= PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED; >> >> Please new flag instead of PKT_RX_VLAN_PKT and please be sure flag is correctly >> used with its new meaning. >> >> <...> >> >>> +/* TX prep functions */ >>> +uint16_t >>> +avf_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts, >>> + uint16_t nb_pkts) >>> +{ >>> + int i, ret; >>> + uint64_t ol_flags; >>> + struct rte_mbuf *m; >>> + >>> + for (i = 0; i < nb_pkts; i++) { >>> + m = tx_pkts[i]; >>> + ol_flags = m->ol_flags; >>> + >>> + /* m->nb_segs is uint8_t, so nb_segs is always less than >>> + * AVF_TX_MAX_SEG. >>> + * We check only a condition for nb_segs > AVF_TX_MAX_MTU_SEG. >>> + */ >> >> This is wrong, nb_segs is 16bits now, this check has been updated in i40e already. >> >> <...> > > Most drivers base code of one of the legacy Intel drivers. > Why not fix ixgbe (or similar) to be a "follow this model" reference? > > It is unreasonable to expect new drivers to follow a different pattern. You are right, updating existing drivers will increase the chance of new drivers being correct at first time. After above said, it is harder to get community driven updates for existing drivers, but easier to ask new drivers to comply with latest libraries since there is already a resource working on developing the driver.