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@xxxxxxxxx>
Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
---
.../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
