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 D74B8223 for ; Wed, 22 Nov 2017 23:38:37 +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 14:38:36 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,438,1505804400"; d="scan'208";a="179483011" 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 14:38:36 -0800 To: "Wu, Jingjing" , "dev@dpdk.org" Cc: "Lu, Wenzhuo" 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> <9BB6961774997848B5B42BEC655768F810EC3889@SHSMSX103.ccr.corp.intel.com> From: Ferruh Yigit Message-ID: Date: Wed, 22 Nov 2017 14:38:36 -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: <9BB6961774997848B5B42BEC655768F810EC3889@SHSMSX103.ccr.corp.intel.com> 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 22:38:38 -0000 On 11/21/2017 11:55 PM, Wu, Jingjing wrote: > > >> -----Original Message----- >> From: Yigit, Ferruh >> Sent: Wednesday, November 22, 2017 8:06 AM >> To: Wu, Jingjing ; dev@dpdk.org >> Cc: Lu, Wenzhuo >> Subject: Re: [dpdk-dev] [RFC 4/9] net/avf: enable basic Rx Tx func >> >> 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? >> > Yes, some macros are defined in avf_rxtx.h for dump descriptors. Will merge them with AVF_DEBUG_TX/RX. > >> <...> >> >>> @@ -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? > Will merge it with AVF_TX_DUMP. > >> >>> +#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? >> > This is used for fast path debug, so static macro will benefit performance. How it will benefit? The PMD_TX_LOG macro controlled by a specific compile time option, RTE_LIBRTE_AVF_DEBUG_TX. If this config is disabled the logging won't be part of all binary at all. When that config option enabled, what is the difference with macro and dynamic debug call? Eventually both are rte_log calls. Only macro has dependency to RTE_LOGTYPE_xxx static definitions. > >> <...> >> >>> +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. Just reminder of this one, new flag is "PKT_RX_VLAN" which means mbuf contains vlan information. >> >> <...> >> >>> +/* 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. >> > Will change, Thanks > >> <...>