Re: [bpf-next V5 PATCH 02/15] xdp: introduce xdp_return_frame API and use in cpumap

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Fri, 23 Mar 2018 09:35:47 -0700
Alexander Duyck <alexander.duyck@gmail.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