On 2024/9/10 22:04, Paolo Abeni wrote:

On 9/6/24 09:36, Yunsheng Lin wrote:

quoted

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 49811c9281d4..6190d9bfd618 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c

@@ -953,7 +953,7 @@ static int __ip_append_data(struct sock *sk,

                  struct flowi4 *fl4,
                  struct sk_buff_head *queue,
                  struct inet_cork *cork,
-                struct page_frag *pfrag,
+                struct page_frag_cache *nc,
                  int getfrag(void *from, char *to, int offset,
                      int len, int odd, struct sk_buff *skb),
                  void *from, int length, int transhdrlen,

@@ -1228,13 +1228,19 @@ static int __ip_append_data(struct sock *sk,

              copy = err;
              wmem_alloc_delta += copy;
          } else if (!zc) {
+            struct page_frag page_frag, *pfrag;
              int i = skb_shinfo(skb)->nr_frags;
+            void *va;
                err = -ENOMEM;
-            if (!sk_page_frag_refill(sk, pfrag))
+            pfrag = &page_frag;
+            va = sk_page_frag_alloc_refill_prepare(sk, nc, pfrag);
+            if (!va)
                  goto error;
                skb_zcopy_downgrade_managed(skb);
+            copy = min_t(int, copy, pfrag->size);
+
              if (!skb_can_coalesce(skb, i, pfrag->page,
                            pfrag->offset)) {
                  err = -EMSGSIZE;

@@ -1242,18 +1248,18 @@ static int __ip_append_data(struct sock *sk,

                      goto error;
                    __skb_fill_page_desc(skb, i, pfrag->page,
-                             pfrag->offset, 0);
+                             pfrag->offset, copy);
                  skb_shinfo(skb)->nr_frags = ++i;
-                get_page(pfrag->page);
+                page_frag_commit(nc, pfrag, copy);
+            } else {
+                skb_frag_size_add(&skb_shinfo(skb)->frags[i - 1],
+                          copy);
+                page_frag_commit_noref(nc, pfrag, copy);
              }
-            copy = min_t(int, copy, pfrag->size - pfrag->offset);
-            if (getfrag(from,
-                    page_address(pfrag->page) + pfrag->offset,
-                    offset, copy, skb->len, skb) < 0)
+
+            if (getfrag(from, va, offset, copy, skb->len, skb) < 0)
                  goto error_efault;

Should the 'commit' happen only when 'getfrag' is successful?

Similar question in the ipv6 code.

Good question.
First of all, even it fails, the 'goto' will handle it correctly as the
skb_shinfo(skb)->nr_frags is setup correctly.
And then 'getfrag' being failure is an unlikely case, right?

I thought about moving it when coding, but decided not to mainly get_page()
is also called before 'getfrag' as above, and I guess 'getfrag' might
involve memcpy'ing causing the cache eviction for the above data, so it
might make sense to do the likely case before 'getfrag'.

Thanks,

Paolo