On 2024/9/10 22:04, Paolo Abeni wrote:
> On 9/6/24 09:36, Yunsheng Lin wrote:
>> 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
>
>
