From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:48820 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751901AbeCWSPS (ORCPT ); Fri, 23 Mar 2018 14:15:18 -0400 Date: Fri, 23 Mar 2018 19:15:09 +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 V5 PATCH 10/15] xdp: rhashtable with allocator ID to pointer mapping Message-ID: <20180323191509.68b62451@redhat.com> In-Reply-To: References: <152180742196.20167.5168801400337773178.stgit@firesoul> <152180752969.20167.10855856167552164450.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 Fri, 23 Mar 2018 09:56:50 -0700 Alexander Duyck wrote: > On Fri, Mar 23, 2018 at 5:18 AM, Jesper Dangaard Brouer > wrote: > > Use the IDA infrastructure for getting a cyclic increasing ID number, > > that is used for keeping track of each registered allocator per > > RX-queue xdp_rxq_info. Instead of using the IDR infrastructure, which > > uses a radix tree, use a dynamic rhashtable, for creating ID to > > pointer lookup table, because this is faster. > > > > The problem that is being solved here is that, the xdp_rxq_info > > pointer (stored in xdp_buff) cannot be used directly, as the > > guaranteed lifetime is too short. The info is needed on a > > (potentially) remote CPU during DMA-TX completion time . In an > > xdp_frame the xdp_mem_info is stored, when it got converted from an > > xdp_buff, which is sufficient for the simple page refcnt based recycle > > schemes. > > > > For more advanced allocators there is a need to store a pointer to the > > registered allocator. Thus, there is a need to guard the lifetime or > > validity of the allocator pointer, which is done through this > > rhashtable ID map to pointer. The removal and validity of of the > > allocator and helper struct xdp_mem_allocator is guarded by RCU. The > > allocator will be created by the driver, and registered with > > xdp_rxq_info_reg_mem_model(). > > > > It is up-to debate who is responsible for freeing the allocator > > pointer or invoking the allocator destructor function. In any case, > > this must happen via RCU freeing. > > > > Use the IDA infrastructure for getting a cyclic increasing ID number, > > that is used for keeping track of each registered allocator per > > RX-queue xdp_rxq_info. > > > > V4: Per req of Jason Wang > > - Use xdp_rxq_info_reg_mem_model() in all drivers implementing > > XDP_REDIRECT, even-though it's not strictly necessary when > > allocator==NULL for type MEM_TYPE_PAGE_SHARED (given it's zero). > > > > Signed-off-by: Jesper Dangaard Brouer > > --- > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 9 + > > drivers/net/tun.c | 6 + > > drivers/net/virtio_net.c | 7 + > > include/net/xdp.h | 15 -- > > net/core/xdp.c | 230 ++++++++++++++++++++++++- > > 5 files changed, 248 insertions(+), 19 deletions(-) > > [...] > > int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq, > > enum mem_type type, void *allocator) > > { > > + struct xdp_mem_allocator *xdp_alloc; > > + gfp_t gfp = GFP_KERNEL; > > + int id, errno, ret; > > + void *ptr; > > + > > + if (xdp_rxq->reg_state != REG_STATE_REGISTERED) { > > + WARN(1, "Missing register, driver bug"); > > + return -EFAULT; > > + } > > + > > if (type >= MEM_TYPE_MAX) > > return -EINVAL; > > > > xdp_rxq->mem.type = type; > > > > - if (allocator) > > - return -EOPNOTSUPP; > > + if (!allocator) > > + return 0; > > + > > + /* Delay init of rhashtable to save memory if feature isn't used */ > > + if (!mem_id_init) { > > + mutex_lock(&mem_id_lock); > > + ret = __mem_id_init_hash_table(); > > + mutex_unlock(&mem_id_lock); > > + if (ret < 0) { > > + WARN_ON(1); > > + return ret; > > + } > > + } > > + > > + xdp_alloc = kzalloc(sizeof(*xdp_alloc), gfp); > > + if (!xdp_alloc) > > + return -ENOMEM; > > + > > + mutex_lock(&mem_id_lock); > > + id = __mem_id_cyclic_get(gfp); > > + if (id < 0) { > > + errno = id; > > + goto err; > > + } > > + xdp_rxq->mem.id = id; > > + xdp_alloc->mem = xdp_rxq->mem; > > + xdp_alloc->allocator = allocator; > > + > > + /* Insert allocator into ID lookup table */ > > + ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node); > > + if (IS_ERR(ptr)) { > > + errno = PTR_ERR(ptr); > > + goto err; > > + } > > + > > + mutex_unlock(&mem_id_lock); > > > > - /* TODO: Allocate an ID that maps to allocator pointer > > - * See: https://www.kernel.org/doc/html/latest/core-api/idr.html > > - */ > > return 0; > > +err: > > + mutex_unlock(&mem_id_lock); > > + kfree(xdp_alloc); > > + return errno; > > } > > EXPORT_SYMBOL_GPL(xdp_rxq_info_reg_mem_model); > > + > > +void xdp_return_frame(void *data, struct xdp_mem_info *mem) > > +{ > > + struct xdp_mem_allocator *xa; > > + > > + rcu_read_lock(); > > + if (mem->id) > > + xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params); > > + rcu_read_unlock(); > > + > > + if (mem->type == MEM_TYPE_PAGE_SHARED) { > > + page_frag_free(data); > > + return; > > + } > > + > > + if (mem->type == MEM_TYPE_PAGE_ORDER0) { > > + struct page *page = virt_to_page(data); /* Assumes order0 page*/ > > + > > + put_page(page); > > + } > > +} > > +EXPORT_SYMBOL_GPL(xdp_return_frame); > > > > I'm not sure what the point is of getting the xa value if it is not > going to be used. Also I would assume there are types that won't even > need the hash table lookup. I would prefer to see this bit held off on > until you have something that actually needs it. I think, you misread the patch. The lookup is NOT going to be performed when mem->id is zero, which is the case that you are interested in for your ixgbe driver. -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer