From: Mat Martineau <martineau@kernel.org>
To: Yunsheng Lin <linyunsheng@huawei.com>
Cc: davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Alexander Duyck <alexander.duyck@gmail.com>,
Ayush Sawal <ayush.sawal@chelsio.com>,
Eric Dumazet <edumazet@google.com>,
Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
Jason Wang <jasowang@redhat.com>, Ingo Molnar <mingo@redhat.com>,
Peter Zijlstra <peterz@infradead.org>,
Juri Lelli <juri.lelli@redhat.com>,
Vincent Guittot <vincent.guittot@linaro.org>,
Dietmar Eggemann <dietmar.eggemann@arm.com>,
Steven Rostedt <rostedt@goodmis.org>,
Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
Daniel Bristot de Oliveira <bristot@redhat.com>,
Valentin Schneider <vschneid@redhat.com>,
John Fastabend <john.fastabend@gmail.com>,
Jakub Sitnicki <jakub@cloudflare.com>,
David Ahern <dsahern@kernel.org>,
Matthieu Baerts <matttbe@kernel.org>,
Geliang Tang <geliang@kernel.org>,
Jamal Hadi Salim <jhs@mojatatu.com>,
Cong Wang <xiyou.wangcong@gmail.com>,
Jiri Pirko <jiri@resnulli.us>,
Boris Pismenny <borisp@nvidia.com>,
bpf@vger.kernel.org, mptcp@lists.linux.dev
Subject: Re: [PATCH net-next v3 11/13] net: replace page_frag with page_frag_cache
Date: Mon, 13 May 2024 16:44:18 -0700 (PDT) [thread overview]
Message-ID: <bcd2a227-9d0e-6d81-2439-2b7f1922bccb@kernel.org> (raw)
In-Reply-To: <444d0349-476b-a04b-f6f1-d59ee57e2054@huawei.com>
On Mon, 13 May 2024, Yunsheng Lin wrote:
> On 2024/5/11 1:29, Mat Martineau wrote:
>> On Fri, 10 May 2024, Yunsheng Lin wrote:
>>
>>> On 2024/5/10 0:22, Mat Martineau wrote:
>>>> On Wed, 8 May 2024, Yunsheng Lin wrote:
>>>>
>>>>> Use the newly introduced prepare/probe/commit API to
>>>>> replace page_frag with page_frag_cache for sk_page_frag().
>>>>>
>>>>> CC: Alexander Duyck <alexander.duyck@gmail.com>
>>>>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>>>>> ---
>>>>> .../chelsio/inline_crypto/chtls/chtls.h | 3 -
>>>>> .../chelsio/inline_crypto/chtls/chtls_io.c | 100 ++++---------
>>>>> .../chelsio/inline_crypto/chtls/chtls_main.c | 3 -
>>>>> drivers/net/tun.c | 28 ++--
>>>>> include/linux/sched.h | 4 +-
>>>>> include/net/sock.h | 14 +-
>>>>> kernel/exit.c | 3 +-
>>>>> kernel/fork.c | 3 +-
>>>>> net/core/skbuff.c | 32 ++--
>>>>> net/core/skmsg.c | 22 +--
>>>>> net/core/sock.c | 46 ++++--
>>>>> net/ipv4/ip_output.c | 33 +++--
>>>>> net/ipv4/tcp.c | 35 ++---
>>>>> net/ipv4/tcp_output.c | 28 ++--
>>>>> net/ipv6/ip6_output.c | 33 +++--
>>>>> net/kcm/kcmsock.c | 30 ++--
>>>>> net/mptcp/protocol.c | 70 +++++----
>>>>> net/sched/em_meta.c | 2 +-
>>>>> net/tls/tls_device.c | 139 ++++++++++--------
>>>>> 19 files changed, 331 insertions(+), 297 deletions(-)
>>>>>
>>>>
>>>> <snip>
>>>>
>>>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>>>> index bb8f96f2b86f..ab844011d442 100644
>>>>> --- a/net/mptcp/protocol.c
>>>>> +++ b/net/mptcp/protocol.c
>>>>> @@ -960,17 +960,18 @@ static bool mptcp_skb_can_collapse_to(u64 write_seq,
>>>>> }
>>>>>
>>>>> /* we can append data to the given data frag if:
>>>>> - * - there is space available in the backing page_frag
>>>>> - * - the data frag tail matches the current page_frag free offset
>>>>> + * - there is space available for the current page
>>>>> + * - the data frag tail matches the current page and offset
>>>>> * - the data frag end sequence number matches the current write seq
>>>>> */
>>>>> static bool mptcp_frag_can_collapse_to(const struct mptcp_sock *msk,
>>>>> - const struct page_frag *pfrag,
>>>>> + const struct page *page,
>>>>> + const unsigned int offset,
>>>>> + const unsigned int size,
>>>>
>>>> Hi Yunsheng -
>>>>
>>>> Why add the 'size' parameter here? It's checked to be a nonzero value, but it can only be 0 if page is also NULL. In this case "page == df->page" will be false, so the function will return false even without checking 'size'.
>>>
>>> Is it possible that the pfrag->page is also NULL, which may cause
>>> mptcp_frag_can_collapse_to() to return true?
>>
>> Not sure. But I do know that df->page will never be NULL, so "page == df->page" will always be false when page == NULL.
>>
>>>
>>> I just found out that the 'size' is not set to zero when return
>>> NULL for the implementation of probe API for the current version.
>>> Perhaps it makes more sense to expect the API caller to make sure
>>> the the returned 'page' not being NULL before using the 'offset',
>>> 'size' and 'va', like below:
>>>
>>> df && page && page == df->page
>>>
>>
>> Given that df->page is never NULL, I don't think the extra "&& page" is needed.
>
> Not checking the extra "&& page" seems to cause the below warning, it seems we
> have the below options:
> 1. ignore the warning.
> 2. set offset to zero if there is no enough space when probe API asks for a specific
> amount of available space as you suggested.
> 3. add the "&& page" in mptcp_frag_can_collapse_to()
>
> what is your favour option? or any other better option?
>
> net-mptcp-protocol.c:warning:variable-offset-is-used-uninitialized-whenever-if-condition-is-false
>
Hi Yunsheng -
That static analyzer is correct that "offset" is *passed* uninitialized in
that scenario, but it doesn't recognize that "offset" is never compared
when page == NULL. So, it's a false positive in a way.
I don't think implementing fix #2 in the page_frag_alloc_probe() macro is
best, since the warning is specific to the MPTCP code and other
page_frag_cache users may have reasons to choose nonzero default values
for the offset. I suggest initializing offset = 0 where it is declared in
mptcp_sendmsg().
- Mat
next prev parent reply other threads:[~2024-05-13 23:44 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 13:33 [PATCH net-next v3 00/13] First try to replace page_frag with page_frag_cache Yunsheng Lin
2024-05-08 13:33 ` [PATCH net-next v3 01/13] mm: page_frag: add a test module for page_frag Yunsheng Lin
2024-05-08 13:33 ` [PATCH net-next v3 02/13] xtensa: remove the get_order() implementation Yunsheng Lin
2024-05-08 13:33 ` [PATCH net-next v3 03/13] mm: page_frag: use free_unref_page() to free page fragment Yunsheng Lin
2024-05-08 13:33 ` [PATCH net-next v3 04/13] mm: move the page fragment allocator from page_alloc into its own file Yunsheng Lin
2024-05-08 13:34 ` [PATCH net-next v3 05/13] mm: page_frag: use initial zero offset for page_frag_alloc_align() Yunsheng Lin
2024-05-08 13:34 ` [PATCH net-next v3 06/13] mm: page_frag: add '_va' suffix to page_frag API Yunsheng Lin
2024-05-08 13:34 ` [PATCH net-next v3 07/13] mm: page_frag: avoid caller accessing 'page_frag_cache' directly Yunsheng Lin
2024-05-08 13:34 ` [PATCH net-next v3 08/13] mm: page_frag: reuse existing space for 'size' and 'pfmemalloc' Yunsheng Lin
2024-05-08 13:34 ` [PATCH net-next v3 09/13] net: introduce the skb_copy_to_va_nocache() helper Yunsheng Lin
2024-05-08 13:34 ` [PATCH net-next v3 10/13] mm: page_frag: introduce prepare/probe/commit API Yunsheng Lin
2024-05-10 17:38 ` Mat Martineau
2024-05-08 13:34 ` [PATCH net-next v3 11/13] net: replace page_frag with page_frag_cache Yunsheng Lin
2024-05-09 16:22 ` Mat Martineau
2024-05-10 9:48 ` Yunsheng Lin
2024-05-10 17:29 ` Mat Martineau
2024-05-13 11:53 ` Yunsheng Lin
2024-05-13 23:44 ` Mat Martineau [this message]
2024-05-08 13:34 ` [PATCH net-next v3 12/13] mm: page_frag: update documentation for page_frag Yunsheng Lin
2024-05-09 0:44 ` Randy Dunlap
2024-05-10 9:48 ` Yunsheng Lin
2024-05-10 12:32 ` Yunsheng Lin
2024-05-10 18:30 ` Randy Dunlap
2024-05-09 16:58 ` Mat Martineau
2024-05-10 9:48 ` Yunsheng Lin
2024-05-08 13:34 ` [PATCH net-next v3 13/13] mm: page_frag: add a entry in MAINTAINERS " Yunsheng Lin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bcd2a227-9d0e-6d81-2439-2b7f1922bccb@kernel.org \
--to=martineau@kernel.org \
--cc=alexander.duyck@gmail.com \
--cc=ayush.sawal@chelsio.com \
--cc=borisp@nvidia.com \
--cc=bpf@vger.kernel.org \
--cc=bristot@redhat.com \
--cc=bsegall@google.com \
--cc=davem@davemloft.net \
--cc=dietmar.eggemann@arm.com \
--cc=dsahern@kernel.org \
--cc=edumazet@google.com \
--cc=geliang@kernel.org \
--cc=jakub@cloudflare.com \
--cc=jasowang@redhat.com \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=john.fastabend@gmail.com \
--cc=juri.lelli@redhat.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linyunsheng@huawei.com \
--cc=matttbe@kernel.org \
--cc=mgorman@suse.de \
--cc=mingo@redhat.com \
--cc=mptcp@lists.linux.dev \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=vincent.guittot@linaro.org \
--cc=vschneid@redhat.com \
--cc=willemdebruijn.kernel@gmail.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).