On Mon, 2020-12-07 at 17:32 +0100, Lorenzo Bianconi wrote:

quoted

Introduce xdp_shared_info data structure to contain info about
"non-linear" xdp frame. xdp_shared_info will alias skb_shared_info
allowing to keep most of the frags in the same cache-line.
Introduce some xdp_shared_info helpers aligned to skb_frag* ones

is there or will be a more general purpose use to this xdp_shared_info
? other than hosting frags ?

I do not have other use-cases at the moment other than multi-buff but in
theory it is possible I guess.
The reason we introduced it is to have most of the frags in the first
shared_info cache-line to avoid cache-misses.

quoted

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/marvell/mvneta.c | 62 +++++++++++++++--------
----
 include/net/xdp.h                     | 52 ++++++++++++++++++++--
 2 files changed, 82 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c

b/drivers/net/ethernet/marvell/mvneta.c
index 1e5b5c69685a..d635463609ad 100644

--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c

@@ -2033,14 +2033,17 @@ int mvneta_rx_refill_queue(struct mvneta_port

*pp, struct mvneta_rx_queue *rxq)
 

[...]

quoted

 static void

@@ -2278,7 +2281,7 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port

*pp,
 			    struct mvneta_rx_desc *rx_desc,
 			    struct mvneta_rx_queue *rxq,
 			    struct xdp_buff *xdp, int *size,
-			    struct skb_shared_info *xdp_sinfo,
+			    struct xdp_shared_info *xdp_sinfo,
 			    struct page *page)
 {
 	struct net_device *dev = pp->dev;

@@ -2301,13 +2304,13 @@ mvneta_swbm_add_rx_fragment(struct

mvneta_port *pp,
 	if (data_len > 0 && xdp_sinfo->nr_frags < MAX_SKB_FRAGS) {
 		skb_frag_t *frag = &xdp_sinfo->frags[xdp_sinfo-

quoted

nr_frags++];

 
-		skb_frag_off_set(frag, pp->rx_offset_correction);
-		skb_frag_size_set(frag, data_len);
-		__skb_frag_set_page(frag, page);
+		xdp_set_frag_offset(frag, pp->rx_offset_correction);
+		xdp_set_frag_size(frag, data_len);
+		xdp_set_frag_page(frag, page);
 

why three separate setters ? why not just one 
xdp_set_frag(page, offset, size) ?

to be aligned with skb_frags helpers, but I guess we can have a single helper,
I do not have a strong opinion on it

quoted

 		/* last fragment */
 		if (len == *size) {
-			struct skb_shared_info *sinfo;
+			struct xdp_shared_info *sinfo;
 
 			sinfo = xdp_get_shared_info_from_buff(xdp);
 			sinfo->nr_frags = xdp_sinfo->nr_frags;

@@ -2324,10 +2327,13 @@ static struct sk_buff *
 mvneta_swbm_build_skb(struct mvneta_port *pp, struct mvneta_rx_queue

*rxq,
 		      struct xdp_buff *xdp, u32 desc_status)
 {

[...]

quoted

 
-static inline struct skb_shared_info *
+struct xdp_shared_info {

xdp_shared_info is a bad name, we need this to have a specific purpose 
xdp_frags should the proper name, so people will think twice before
adding weird bits to this so called shared_info.

I named the struct xdp_shared_info to recall skb_shared_info but I guess
xdp_frags is fine too. Agree?

quoted

+	u16 nr_frags;
+	u16 data_length; /* paged area length */
+	skb_frag_t frags[MAX_SKB_FRAGS];

why MAX_SKB_FRAGS ? just use a flexible array member 
skb_frag_t frags[]; 

and enforce size via the n_frags and on the construction of the
tailroom preserved buffer, which is already being done.

this is waste of unnecessary space, at lease by definition of the
struct, in your use case you do:
memcpy(frag_list, xdp_sinfo->frags, sizeof(skb_frag_t) * num_frags);
And the tailroom space was already preserved for a full skb_shinfo.
so i don't see why you need this array to be of a fixed MAX_SKB_FRAGS
size.

In order to avoid cache-misses, xdp_shared info is built as a variable
on mvneta_rx_swbm() stack and it is written to "shared_info" area only on the
last fragment in mvneta_swbm_add_rx_fragment(). I used MAX_SKB_FRAGS to be
aligned with skb_shared_info struct but probably we can use even a smaller value.
Another approach would be to define two different struct, e.g.

stuct xdp_frag_metadata {
	u16 nr_frags;
	u16 data_length; /* paged area length */
};

struct xdp_frags {
	skb_frag_t frags[MAX_SKB_FRAGS];
};

and then define xdp_shared_info as

struct xdp_shared_info {
	stuct xdp_frag_metadata meta;
	skb_frag_t frags[];
};

In this way we can probably optimize the space. What do you think?

quoted

+};
+
+static inline struct xdp_shared_info *
 xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
 {
-	return (struct skb_shared_info *)xdp_data_hard_end(xdp);
+	BUILD_BUG_ON(sizeof(struct xdp_shared_info) >
+		     sizeof(struct skb_shared_info));
+	return (struct xdp_shared_info *)xdp_data_hard_end(xdp);
+}
+

Back to my first comment, do we have plans to use this tail room buffer
for other than frag_list use cases ? what will be the buffer format
then ? should we push all new fields to the end of the xdp_shared_info
struct ? or deal with this tailroom buffer as a stack ? 
my main concern is that for drivers that don't support frag list and
still want to utilize the tailroom buffer for other usecases they will
have to skip the first sizeof(xdp_shared_info) so they won't break the
stack.

for the moment I do not know if this area is used for other purposes.
Do you think there are other use-cases for it?

quoted

+static inline struct page *xdp_get_frag_page(const skb_frag_t *frag)
+{
+	return frag->bv_page;
+}
+
+static inline unsigned int xdp_get_frag_offset(const skb_frag_t
*frag)
+{
+	return frag->bv_offset;
+}
+
+static inline unsigned int xdp_get_frag_size(const skb_frag_t *frag)
+{
+	return frag->bv_len;
+}
+
+static inline void *xdp_get_frag_address(const skb_frag_t *frag)
+{
+	return page_address(xdp_get_frag_page(frag)) +
+	       xdp_get_frag_offset(frag);
+}
+
+static inline void xdp_set_frag_page(skb_frag_t *frag, struct page
*page)
+{
+	frag->bv_page = page;
+}
+
+static inline void xdp_set_frag_offset(skb_frag_t *frag, u32 offset)
+{
+	frag->bv_offset = offset;
+}
+
+static inline void xdp_set_frag_size(skb_frag_t *frag, u32 size)
+{
+	frag->bv_len = size;
 }
 
 struct xdp_frame {

@@ -120,12 +164,12 @@ static __always_inline void

xdp_frame_bulk_init(struct xdp_frame_bulk *bq)
 	bq->xa = NULL;
 }
 
-static inline struct skb_shared_info *
+static inline struct xdp_shared_info *
 xdp_get_shared_info_from_frame(struct xdp_frame *frame)
 {
 	void *data_hard_start = frame->data - frame->headroom -
sizeof(*frame);
 
-	return (struct skb_shared_info *)(data_hard_start + frame-

quoted

frame_sz -

+	return (struct xdp_shared_info *)(data_hard_start + frame-

quoted

frame_sz -

 				SKB_DATA_ALIGN(sizeof(struct
skb_shared_info)));
 }
 

need a comment here why we preserve the size of skb_shared_info, yet
the usable buffer is of type xdp_shared_info.

ack, I will add it in v6.

Regards,
Lorenzo