From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 84533B4FD for ; Fri, 13 Feb 2015 17:45:09 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga102.fm.intel.com with ESMTP; 13 Feb 2015 08:45:07 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.09,571,1418112000"; d="scan'208";a="527201855" Received: from ecsmtp.pdx.intel.com (HELO plxs0284.pdx.intel.com) ([10.25.97.128]) by orsmga003.jf.intel.com with ESMTP; 13 Feb 2015 08:36:50 -0800 Received: from plxv1142.pdx.intel.com (plxv1142.pdx.intel.com [10.25.98.49]) by plxs0284.pdx.intel.com with ESMTP id t1DGj7pT011858; Fri, 13 Feb 2015 08:45:08 -0800 Received: from plxv1142.pdx.intel.com (localhost [127.0.0.1]) by plxv1142.pdx.intel.com with ESMTP id t1DGj7pR011572; Fri, 13 Feb 2015 08:45:07 -0800 Received: (from jbshaw@localhost) by plxv1142.pdx.intel.com with œ id t1DGj3IO010486; Fri, 13 Feb 2015 08:45:04 -0800 Date: Fri, 13 Feb 2015 08:45:03 -0800 From: Jeff Shaw To: David Marchand Message-ID: <20150213164503.GA9768@plxv1142.pdx.intel.com> References: <1423618298-2933-2-git-send-email-jing.d.chen@intel.com> <1423815597-17819-1-git-send-email-jing.d.chen@intel.com> <1423815597-17819-9-git-send-email-jing.d.chen@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v5 08/17] fm10k: add RX/TX single queue start/stop function 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, 13 Feb 2015 16:45:10 -0000 Hi David, thanks for the review. On Fri, Feb 13, 2015 at 12:31:16PM +0100, David Marchand wrote: > Hello, > > On Fri, Feb 13, 2015 at 9:19 AM, Chen Jing D(Mark) > wrote: > > [snip] > > +/* > > + * Verify Rx packet buffer alignment is valid. > > + * > > + * Hardware requires specific alignment for Rx packet buffers. At > > + * least one of the following two conditions must be satisfied. > > + * 1. Address is 512B aligned > > + * 2. Address is 8B aligned and buffer does not cross 4K boundary. > > + * > > + * Return 1 if buffer alignment satisfies at least one condition, > > + * otherwise return 0. > > + * > > + * Note: Alignment is checked by the driver when the Rx queue is reset. It > > + * is assumed that if an entire descriptor ring can be filled with > > + * buffers containing valid alignment, then all buffers in that > > mempool > > + * have valid address alignment. It is the responsibility of the > > user > > + * to ensure all buffers have valid alignment, as it is the user who > > + * creates the mempool. > > + * Note: It is assumed the buffer needs only to store a maximum size > > Ethernet > > + * frame. > > + */ > > +static inline int > > +fm10k_addr_alignment_valid(struct rte_mbuf *mb) > > +{ > > + uint64_t addr = MBUF_DMA_ADDR_DEFAULT(mb); > > + uint64_t boundary1, boundary2; > > + > > + /* 512B aligned? */ > > + if (RTE_ALIGN(addr, 512) == addr) > > + return 1; > > + > > + /* 8B aligned, and max Ethernet frame would not cross a 4KB > > boundary? */ > > + if (RTE_ALIGN(addr, 8) == addr) { > > + boundary1 = RTE_ALIGN_FLOOR(addr, 4096); > > + boundary2 = RTE_ALIGN_FLOOR(addr + > > ETHER_MAX_VLAN_FRAME_LEN, > > + 4096); > > + if (boundary1 == boundary2) > > + return 1; > > + } > > + > > + /* use RTE_LOG directly to make sure this error is seen */ > > + RTE_LOG(ERR, PMD, "%s(): Error: Invalid buffer alignment\n", > > __func__); > > + > > + return 0; > > +} > > > > Same comment as before, do not directly use RTE_LOG. > This is init stuff, you have a PMD_INIT_LOG macro. Agreed, the comment should be fixed. > > By the way, I need to dig deeper into this, but I can see multiple patches > ensuring buffer alignment. > Do we really need to validate this alignment here, if we already validated > this constraint at the mempool level ? > This is really a sanity check. The buffer alignment needs to be checked at runtime becuase a user could modify the alignment. We provide a check here to be extra safe, and hopefully to fail at init time rather than later. There are two ways to satisfy the alignment requirements for the hardware. Currently the driver implements the 512B alignment, but it is possible somebody may want to the other 8B alignment w/o crossing a 4K page boundary. This sanity check would help catch any possible issues in the future related to buffer alignment. -Jeff