Date:   Fri, 23 Mar 2018 19:15:09 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        BjörnTöpel <bjorn.topel@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Eugenia Emantayev <eugenia@...lanox.com>,
        Jason Wang <jasowang@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Eran Ben Elisha <eranbe@...lanox.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Gal Pressman <galp@...lanox.com>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Tariq Toukan <tariqt@...lanox.com>, brouer@...hat.com
Subject: Re: [bpf-next V5 PATCH 10/15] xdp: rhashtable with allocator ID to
 pointer mapping

On Fri, 23 Mar 2018 09:56:50 -0700
Alexander Duyck <alexander.duyck@...il.com> wrote:

> On Fri, Mar 23, 2018 at 5:18 AM, Jesper Dangaard Brouer
> <brouer@...hat.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 <brouer@...hat.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

Powered by blists - more mailing lists