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

quoted

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...)

quoted

+       /* 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.

[...]

quoted

+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