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