| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 23 Mar 2018 11:22:14 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Jesper Dangaard Brouer <brouer@...hat.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>
Subject: Re: [bpf-next V5 PATCH 10/15] xdp: rhashtable with allocator ID to
pointer mapping
On Fri, Mar 23, 2018 at 11:15 AM, Jesper Dangaard Brouer
<brouer@...hat.com> wrote:
> 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.
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
Powered by blists - more mailing lists