On Fri, 23 Mar 2018 09:56:50 -0700
Alexander Duyck <alexander.du...@gmail.com> wrote:
> On Fri, Mar 23, 2018 at 5:18 AM, Jesper Dangaard Brouer
> <bro...@redhat.com> 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 <bro...@redhat.com>
> > ---
> > 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
