netdev - Re: [PATCH net-next v18 12/14] net: replace page_frag with page_frag

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <15d963c2-1854-4fb7-a7a4-d8ed8cafead3@huawei.com>
Date: Wed, 11 Sep 2024 19:31:16 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Paolo Abeni <pabeni@...hat.com>, <davem@...emloft.net>, <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Alexander Duyck
	<alexander.duyck@...il.com>, Ayush Sawal <ayush.sawal@...lsio.com>, Eric
 Dumazet <edumazet@...gle.com>, Willem de Bruijn
	<willemdebruijn.kernel@...il.com>, Jason Wang <jasowang@...hat.com>, Ingo
 Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>, Juri Lelli
	<juri.lelli@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>, Steven Rostedt
	<rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, Mel Gorman
	<mgorman@...e.de>, Valentin Schneider <vschneid@...hat.com>, John Fastabend
	<john.fastabend@...il.com>, Jakub Sitnicki <jakub@...udflare.com>, David
 Ahern <dsahern@...nel.org>, Matthieu Baerts <matttbe@...nel.org>, Mat
 Martineau <martineau@...nel.org>, Geliang Tang <geliang@...nel.org>, Jamal
 Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>, Jiri
 Pirko <jiri@...nulli.us>, Boris Pismenny <borisp@...dia.com>,
	<bpf@...r.kernel.org>, <mptcp@...ts.linux.dev>
Subject: Re: [PATCH net-next v18 12/14] net: replace page_frag with
 page_frag_cache

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
> 
>