On Sat, Jan 08, 2022 at 12:53:09PM +0100, Lorenzo Bianconi wrote:

quoted

Rely on xdp_update_skb_shared_info routine in order to avoid
resetting frags array in skb_shared_info structure building
the skb in mvneta_swbm_build_skb(). Frags array is expected to
be initialized by the receiving driver building the xdp_buff
and here we just need to update memory metadata.

Acked-by: Toke Hoiland-Jorgensen <redacted>
Acked-by: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/marvell/mvneta.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 775ffd91b741..267a306d9c75 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c

@@ -2332,8 +2332,12 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
              skb_frag_size_set(frag, data_len);
              __skb_frag_set_page(frag, page);

-             if (!xdp_buff_is_mb(xdp))
+             if (!xdp_buff_is_mb(xdp)) {
+                     sinfo->xdp_frags_size = *size;
                      xdp_buff_set_mb(xdp);
+             }
+             if (page_is_pfmemalloc(page))
+                     xdp_buff_set_frag_pfmemalloc(xdp);
      } else {
              page_pool_put_full_page(rxq->page_pool, page, true);
      }

@@ -2347,7 +2351,6 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
      struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
      struct sk_buff *skb;
      u8 num_frags;
-     int i;

      if (unlikely(xdp_buff_is_mb(xdp)))
              num_frags = sinfo->nr_frags;

@@ -2362,18 +2365,12 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
      skb_put(skb, xdp->data_end - xdp->data);
      skb->ip_summed = mvneta_rx_csum(pp, desc_status);

-     if (likely(!xdp_buff_is_mb(xdp)))
-             goto out;
-
-     for (i = 0; i < num_frags; i++) {
-             skb_frag_t *frag = &sinfo->frags[i];
-
-             skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
-                             skb_frag_page(frag), skb_frag_off(frag),
-                             skb_frag_size(frag), PAGE_SIZE);

Maybe I'm missing something but I'm not sure you have a suitable
replacement for the 3 lines above this in your proposed change.

Hi Andy,

mvneta_swbm_add_rx_fragment() initializes frags array in
skb_shared_info for xdp whenever we receive a multi-descriptors frame.
Since frags array is at the same offset for the xdp_buff and for the
new skb and build_skb() in mvneta_swbm_build_skb() does not overwrite
it, we do not need to initialize it again allocating the skb, just
account metadata info running xdp_update_skb_shared_info(). Agree?

quoted

-     }
+     if (unlikely(xdp_buff_is_mb(xdp)))
+             xdp_update_skb_shared_info(skb, num_frags,
+                                        sinfo->xdp_frags_size,
+                                        num_frags * xdp->frame_sz,
+                                        xdp_buff_is_frag_pfmemalloc(xdp));

When I did an implementation of this on a different driver I also needed
to add:

        for (i = 0; i < num_frags; i++)
                skb_frag_set_page(skb, i, skb_frag_page(&sinfo->frags[i]));

to make sure that frames that were given XDP_PASS were formatted
correctly so they could be handled by the stack.  Don't you need
something similar to make sure frags are properly set?

Thanks,

-andy

P.S.  Sorry for noticing this so late in the process; I realize this version
was just a rebase of v20 and this would have been useful information
earlier if I'm correct.

no worries :)

Regards,
Lorenzo

quoted

-out:
      return skb;
 }

--
2.33.1