On Fri, Mar 23, 2018 at 11:15 AM, Jesper Dangaard Brouer
[off-list ref] wrote:

On Fri, 23 Mar 2018 09:56:50 -0700
Alexander Duyck [off-list ref] wrote:

quoted

On Fri, Mar 23, 2018 at 5:18 AM, Jesper Dangaard Brouer
[off-list ref] wrote:

quoted

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 <redacted>
---
 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(-)

[...]

quoted

quoted

 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.

Sorry, to clarify. Why do I have to take rcu_read_lock and
rcu_read_unlock if i am not doing an rcu read? Why even bother doing a
conditional check for mem->id if the lookup using it is not used?

Basically if I am not using it why should I take any of the overhead
for it. I would much rather have this code reduced to be as small and
fast as possible instead of wasting cycles on the RCU acquire/release,
reading mem->id, testing mem->id, and then jumping over the code that
is not used in either case even if mem->id isn't 0.

What I don't get is why you aren't getting warnings about variables
being assigned but never used in the case of xa.

- Alex