On Tue, Jan 11, 2022 at 02:05:23PM +0100, Lorenzo Bianconi wrote:

quoted

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?

Lorenzo,

Thanks for the explanation; I do agree.  I was thinking about this last night
and suspected this was the case.  My current implementation doesn't use
build_skb to reuse the DMA buffer; it allocates a new skb->data area for
passing up the stack.  This is why I needed the skb_frag_set_page calls and you
did not.  That may change, but the first implementation will probably continue
on that path.

Overall this set looks pretty solid to me.  Thanks to you and other
contributors for all the work on this!

-andy

quoted

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

quoted

-out:
      return skb;
 }

--
2.33.1