Re: [PATCH v8 bpf-next 09/14] bpd: add multi-buffer support to xdp copy helpers

[Eelco Chaudron]

On 8 Apr 2021, at 23:04, Vladimir Oltean wrote:

On Thu, Apr 08, 2021 at 02:51:01PM +0200, Lorenzo Bianconi wrote:
From: Eelco Chaudron <echau...@redhat.com>

This patch adds support for multi-buffer for the following helpers:
  - bpf_xdp_output()
  - bpf_perf_event_output()

Signed-off-by: Eelco Chaudron <echau...@redhat.com>
Signed-off-by: Lorenzo Bianconi <lore...@kernel.org>
---

Also there is a typo in the commit message: bpd -> bpf.

ACK, will fix in next version.

 net/core/filter.c                             |  63 ++++++++-
 .../selftests/bpf/prog_tests/xdp_bpf2bpf.c    | 127 
++++++++++++------
 .../selftests/bpf/progs/test_xdp_bpf2bpf.c    |   3 +-
 3 files changed, 149 insertions(+), 44 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index c4eb1392f88e..c00f52ab2532 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4549,10 +4549,56 @@ static const struct bpf_func_proto 
bpf_sk_ancestor_cgroup_id_proto = {
 };
 #endif

-static unsigned long bpf_xdp_copy(void *dst_buff, const void 
*src_buff,
+static unsigned long bpf_xdp_copy(void *dst_buff, const void *ctx,
                                  unsigned long off, unsigned long len)
 {
-       memcpy(dst_buff, src_buff + off, len);
+       struct xdp_buff *xdp = (struct xdp_buff *)ctx;

There is no need to cast a void pointer in C.

I added this as the void pointer is a const. However, looking at it 
again, we should probably change xdp_get_shared_info_from_buff() to also 
take a const pointer, i.e.:

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

What do you think Lorenzo?

+       struct xdp_shared_info *xdp_sinfo;
+       unsigned long base_len;
+
+       if (likely(!xdp->mb)) {
+               memcpy(dst_buff, xdp->data + off, len);
+               return 0;
+       }
+
+       base_len = xdp->data_end - xdp->data;

Would a static inline int xdp_buff_head_len() be useful?

Guess everybody is using the xdp->data_end - xdp->data, in there code. 
But I guess we can add a static inline and change all code, but I 
don’t think we should do it as part of this patchset. I would also 
call it something like xdp_buff_data_len()?

+       xdp_sinfo = xdp_get_shared_info_from_buff(xdp);
+       do {
+               const void *src_buff = NULL;
+               unsigned long copy_len = 0;
+
+               if (off < base_len) {
+                       src_buff = xdp->data + off;
+                       copy_len = min(len, base_len - off);
+               } else {
+                       unsigned long frag_off_total = base_len;
+                       int i;
+
+                       for (i = 0; i < xdp_sinfo->nr_frags; i++) {
+                               skb_frag_t *frag = &xdp_sinfo->frags[i];
+                               unsigned long frag_len, frag_off;
+
+                               frag_len = xdp_get_frag_size(frag);
+                               frag_off = off - frag_off_total;
+                               if (frag_off < frag_len) {
+                                       src_buff = xdp_get_frag_address(frag) +
+                                                  frag_off;
+                                       copy_len = min(len,
+                                                      frag_len - frag_off);
+                                       break;
+                               }
+                               frag_off_total += frag_len;
+                       }
+               }
+               if (!src_buff)
+                       break;
+
+               memcpy(dst_buff, src_buff, copy_len);
+               off += copy_len;
+               len -= copy_len;
+               dst_buff += copy_len;
+       } while (len);
+
        return 0;
 }