netdev - Re: [bpf-next V5 PATCH 02/15] xdp: introduce xdp_return

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 26 Mar 2018 21:06:48 +0200
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 02/15] xdp: introduce xdp_return_frame API
 and use in cpumap

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

> > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > index b2362ddfa694..15b546325e31 100644
> > --- a/include/net/xdp.h
> > +++ b/include/net/xdp.h
> > @@ -33,16 +33,44 @@
> >   * also mandatory during RX-ring setup.
> >   */
> >
> > +enum mem_type {
> > +       MEM_TYPE_PAGE_SHARED = 0, /* Split-page refcnt based model */
> > +       MEM_TYPE_PAGE_ORDER0,     /* Orig XDP full page model */
> > +       MEM_TYPE_MAX,
> > +};
> > +
> > +struct xdp_mem_info {
> > +       u32 type; /* enum mem_type, but known size type */  
> 
> Do you really need to make t his a full u32 value for something that
> is likely never going to exceed a single digit value?

Could we please save these kind of optimizations for later, please?

I do have a plan for state compression later.  If you noticed, later
I'll add an 'id' member.   And in the ID patch, I'm actually
limiting the IDs to max 16-bit.  Thus, the plan is to allow for storing
type+id in e.g. a u32.  BUT I see no reason do this now and complicate
the patchset further. (When we do this state compression, I actually
want to run some perf tests, that kind of work can cause unfortunate
compiler and assembler level constructs what are slower...)

> > +       /* u32 id; will be added later in this patchset */  
> 
> Wouldn't it be better to just hold off and add it then instead of
> adding it as a comment?

I was trying to help the reviewer see where this was going.  The line
will be updated within the patchset, like the comment promise, so there
is not real danger of having it here.

[...]
> > +static inline
> > +void xdp_return_frame(void *data, struct xdp_mem_info *mem)
> > +{
> > +       if (mem->type == MEM_TYPE_PAGE_SHARED)
> > +               page_frag_free(data);
> > +
> > +       if (mem->type == MEM_TYPE_PAGE_ORDER0) {
> > +               struct page *page = virt_to_page(data); /* Assumes order0 page*/
> > +
> > +               put_page(page);
> > +       }  
> 
> Actually page_frag_free would probably work for either one.  Also is it
> safe to assume that the page is order 0? Are the only users of
> compound pages that support XDP also only supporting the page fragment
> setup?
> 
> Also you probably don't need put_page. It might be better to use
> __free_page if you are certain the pages are coming from the Rx path
> of drivers and don't have any special destructors associated with
> them.

Can we also keep these kind of optimization for later, please? (p.s. I
do know this, e.g. I put a comment in page_pool of using __free_pages()).


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer