From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:46348 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752059AbeCHPam (ORCPT ); Thu, 8 Mar 2018 10:30:42 -0500 Date: Thu, 8 Mar 2018 16:30:30 +0100 From: Jesper Dangaard Brouer To: Alexander Duyck Cc: Netdev , =?UTF-8?B?QmrDtnJuVMO2cGVs?= , "Karlsson, Magnus" , Eugenia Emantayev , Jason Wang , John Fastabend , Eran Ben Elisha , Saeed Mahameed , Gal Pressman , Daniel Borkmann , Alexei Starovoitov , Tariq Toukan , brouer@redhat.com Subject: Re: [bpf-next V2 PATCH 00/15] XDP redirect memory return API Message-ID: <20180308163030.5a955cb8@redhat.com> In-Reply-To: References: <152051439383.7018.11827926732878918934.stgit@firesoul> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: On Thu, 8 Mar 2018 07:03:07 -0800 Alexander Duyck wrote: > On Thu, Mar 8, 2018 at 5:07 AM, Jesper Dangaard Brouer > wrote: > > This patchset works towards supporting different XDP RX-ring memory > > allocators. As this will be needed by the AF_XDP zero-copy mode. > > > > The patchset uses mlx5 as the sample driver, which gets implemented > > XDP_REDIRECT RX-mode, but not ndo_xdp_xmit (as this API is subject to > > change thought the patchset). > > > > A new struct xdp_frame is introduced (modeled after cpumap xdp_pkt). > > And both ndo_xdp_xmit and the new xdp_return_frame end-up using this. > > > > Support for a driver supplied allocator is implemented, and a > > refurbished version of page_pool is the first return allocator type > > introduced. This will be a integration point for AF_XDP zero-copy. > > > > The mlx5 driver evolve into using the page_pool, and see a performance > > increase (with ndo_xdp_xmit out ixgbe driver) from 6Mpps to 12Mpps. > > So one question I would have is what effect does your patch set have > on packets moving in the opposite direction, or ixgbe to ixgbe. My > concern is how much of a hit we are taking in ixgbe to support a more > generic solution. The overhead for ixgbe is basically close to zero. Notice that the MEM_TYPE_PAGE_SHARED type (like ixgbe) does not get allocated an ID, thus it avoids doing any hash lookup. And basically just calls page_frag_free(). if (mem->type == MEM_TYPE_PAGE_SHARED) { page_frag_free(data); return; } > > The patchset stop at the 15 patches limit, but more API changes are > > planned. Specifically extending ndo_xdp_xmit and xdp_return_frame > > APIs to support bulking. As this will address some known limits. > > > > V2: Updated according to Tariq's feedback > > I am assuming you are submitting this as an RFC aren't you? If not you > have some general quality issues to work on. I saw c99 style "//" > comments in patch 2, for example: > +struct xdp_mem_info { > + u32 type; /* enum mem_type, but known size type */ > + u32 id; // Needed later (to lookup struct xdp_rxq_info) > +}; > + > > You should probably just go with the standard "/* */" style comments > and be consistent. This comment style, is basically a reminder to myself (and it gets removed later). I guess, I should have removed it completely before I submitted. Note, I've already done 3 versions of pre-RFC patchset (offlist), so I hope this patchset can be soon be considered non-RFC material. > There are also "FIXME" comments in patch 6, either drop them entirely > so you don't need to remove them in patch 15, or address the issue of > explaining why you needed the call. This were actually a question to Jason Wang... pointed it out explicitly. -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer