On Mon, 07 Dec 2020 22:49:55 -0800
Saeed Mahameed [off-list ref] wrote:

On Mon, 2020-12-07 at 19:16 -0800, Alexander Duyck wrote:

quoted

On Mon, Dec 7, 2020 at 3:03 PM Saeed Mahameed [off-list ref]
wrote:  

quoted

On Mon, 2020-12-07 at 13:16 -0800, Alexander Duyck wrote:  

quoted

On Mon, Dec 7, 2020 at 8:36 AM Lorenzo Bianconi <  
lorenzo@kernel.org>  
wrote:  

quoted

Introduce multi-buffer bit (mb) in xdp_frame/xdp_buffer data
structure
in order to specify if this is a linear buffer (mb = 0) or a
multi-
buffer
frame (mb = 1). In the latter case the shared_info area at the
end
of the
first buffer is been properly initialized to link together
subsequent
buffers.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 include/net/xdp.h | 8 ++++++--
 net/core/xdp.c    | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/net/xdp.h b/include/net/xdp.h
index 700ad5db7f5d..70559720ff44 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h

@@ -73,7 +73,8 @@ struct xdp_buff {
        void *data_hard_start;
        struct xdp_rxq_info *rxq;
        struct xdp_txq_info *txq;
-       u32 frame_sz; /* frame size to deduce

data_hard_end/reserved tailroom*/
+       u32 frame_sz:31; /* frame size to deduce
data_hard_end/reserved tailroom*/
+       u32 mb:1; /* xdp non-linear buffer */
 };
  

If we are really going to do something like this I say we should
just
rip a swath of bits out instead of just grabbing one. We are
already
cutting the size down then we should just decide on the minimum
size
that is acceptable and just jump to that instead of just stealing
one
bit at a time. It looks like we already have differences between
the
size here and frame_size in xdp_frame.
  

+1
  

quoted

If we have to steal a bit why not look at something like one of
the
lower 2/3 bits in rxq? You could then do the same thing using
dev_rx
in a similar fashion instead of stealing from a bit that is
likely to
be used in multiple spots and modifying like this adds extra
overhead
to?
  

What do you mean in rxq ? from the pointer ?  

Yeah, the pointers have a few bits that are guaranteed 0 and in my
mind reusing the lower bits from a 4 or 8 byte aligned pointer would
make more sense then stealing the upper bits from the size of the
frame.  

Ha, i can't imagine how accessing that pointer would look like ..
is possible to define the pointer as a bit-field and just access it
normally ? or do we need to fix it up every time we need to access it ?
will gcc/static checkers complain about wrong pointer type ?

This is a pattern that is used all over the kernel.  Yes, it needs to
be fixed it up every time we access it.  In this case, we don't want to
to deploy this trick.  For two reason, (1) rxq is accessed by BPF
byte-code rewrite (which would also need to handle masking out the
bit), (2) this optimization is trading CPU cycles for saving space.

IIRC Alexei have already pointed out that the change to struct xdp_buff
looks suboptimal.  Why don't you simply add a u8 with the info.

The general point is that struct xdp_buff layout is for fast access,
and struct xdp_frame is a state compressed version of xdp_buff.  (Still
room in xdp_buff is limited to 64 bytes - one cacheline, which is
rather close according to pahole)

Thus, it is more okay to do these bit tricks in struct xdp_frame.  For
xdp_frame, it might be better to take some room/space from the member
'mem' (struct xdp_mem_info).  (Would it help later that multi-buffer
bit is officially part of struct xdp_mem_info, when later freeing the
memory backing the frame?)

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

$ pahole -C xdp_buff
struct xdp_buff {
	void *                     data;                 /*     0     8 */
	void *                     data_end;             /*     8     8 */
	void *                     data_meta;            /*    16     8 */
	void *                     data_hard_start;      /*    24     8 */
	struct xdp_rxq_info *      rxq;                  /*    32     8 */
	struct xdp_txq_info *      txq;                  /*    40     8 */
	u32                        frame_sz;             /*    48     4 */

	/* size: 56, cachelines: 1, members: 7 */
	/* padding: 4 */
	/* last cacheline: 56 bytes */
};

$ pahole -C xdp_frame
struct xdp_frame {
	void *                     data;                 /*     0     8 */
	u16                        len;                  /*     8     2 */
	u16                        headroom;             /*    10     2 */
	u32                        metasize:8;           /*    12: 0  4 */
	u32                        frame_sz:24;          /*    12: 8  4 */
	struct xdp_mem_info        mem;                  /*    16     8 */
	struct net_device *        dev_rx;               /*    24     8 */

	/* size: 32, cachelines: 1, members: 7 */
	/* last cacheline: 32 bytes */
};

$ pahole -C xdp_mem_info
struct xdp_mem_info {
	u32                        type;                 /*     0     4 */
	u32                        id;                   /*     4     4 */

	/* size: 8, cachelines: 1, members: 2 */
	/* last cacheline: 8 bytes */
};