Re: [linux-next:master] [mm, slab] 6431f06eec: WARNING:at_include/linux/mm.h:#skb_append_pagefrags

[Vlastimil Babka <vbabka@suse.cz>]

On 5/28/25 18:52, Bernard Metzler wrote:
> 
> 
>> -----Original Message-----
>> From: Vlastimil Babka <vbabka@suse.cz>
>> Sent: Wednesday, May 28, 2025 11:46 AM
>> To: kernel test robot <oliver.sang@intel.com>; David S. Miller
>> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
>> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Bernard Metzler
>> <BMT@zurich.ibm.com>; Jason Gunthorpe <jgg@ziepe.ca>; Leon Romanovsky
>> <leon@kernel.org>
>> Cc: oe-lkp@lists.linux.dev; lkp@intel.com; linux-mm@kvack.org; Matthew
>> Wilcox <willy@infradead.org>; Simon Horman <horms@kernel.org>;
>> netdev@vger.kernel.org; linux-rdma@vger.kernel.org
>> Subject: [EXTERNAL] Re: [linux-next:master] [mm, slab] 6431f06eec:
>> WARNING:at_include/linux/mm.h:#skb_append_pagefrags
>> 
>> On 5/22/25 06:54, kernel test robot wrote:
>> >
>> >
>> > Hello,
>> >
>> >
>> > we noticed the WARN added in this commit is hit in our tests. not sure if
>> it's
>> > expected. below full report FYI.
>> 
>> It's expected in the sense that if somebody is doing the wrong thing, there
>> would be a report. So it seems that has now happened :)
>> 
>> > kernel test robot noticed
>> "WARNING:at_include/linux/mm.h:#skb_append_pagefrags" on:
>> >
>> > commit: 6431f06eecf44e7b8d42237cb0e166a456f491ad ("mm, slab: warn when
>> increasing refcount on large kmalloc page")
>> > https% 
>> 3A__git.kernel.org_cgit_linux_kernel_git_next_linux-
>> 2Dnext.git&d=DwIFaQ&c=BSDicqBQBDjDI9RkVyTcHQ&r=4ynb4Sj_4MUcZXbhvovE4tYSbqxy
>> OwdSiLedP4yO55g&m=RVwvTs1_4tv9VS6aBY_BN1Nb5XX4P80N9RLHq1UND_lRCm9uja6lYFlrt
>> 1eIP6xD&s=gDBCZ9iGPTCbRHLz-aCIQx6RpCDBnybfljVjh1G_MjQ&e=  master
>> 
>> FYI that's
>> https% 
>> 3A__git.kernel.org_pub_scm_linux_kernel_git_vbabka_slab.git_commit_-3Fh-
>> 3Dslab_for-2D6.16_testing-26id-
>> 3D6431f06eecf44e7b8d42237cb0e166a456f491ad&d=DwIFaQ&c=BSDicqBQBDjDI9RkVyTcH
>> Q&r=4ynb4Sj_4MUcZXbhvovE4tYSbqxyOwdSiLedP4yO55g&m=RVwvTs1_4tv9VS6aBY_BN1Nb5
>> XX4P80N9RLHq1UND_lRCm9uja6lYFlrt1eIP6xD&s=HgoQP2p-
>> qH0YF0Pxt2hT1lLJjQI4CaEArKRz-4OZCUA&e=
>> 
>> so we have skb_splice_from_iter() calling skb_append_pagefrags() and that
>> does a get_page(). But this warning means one of the pages is a kmalloc()
>> with size >8kb that is using the page allocator and not slab caches. But
>> that 8kb threshold is an implementation detail, so we want all kmalloc()
>> allocated buffers to behave the same and use frozen pages and thus not
>> allow
>> get_page().
>> 
>> Note skb_splice_from_iter() has above the call to skb_append_pagefrags():
>> 
>> if (WARN_ON_ONCE(!sendpage_ok(page)))
>>     goto out;
>> 
>> and sendpage_ok() is:
>> 
>> return !PageSlab(page) && page_count(page) >= 1;
>> 
>> Thus if we went ahead with frozen pages for large kmalloc(), sendpage_ok()
>> would start marking them as not ok, which seems like the right thing. But
>> this callsite would still produce a WARN_ON_ONCE(), so that suggests it's
>> not really expected to for kmalloc() pages to reach this code.
>> 
>> It's possible that some other code using sendpage_ok() elsewhere prevents
>> this from happening, and this WARN_ON_ONCE() is just a safety double check.
>> Or not, and something (siw?) needs to be fixed to e.g. stop using kmalloc()
> 
> siw ckecks every page before sending via sendpage_ok(). It unsets
> MSG_SPLICE_PAGES flag for the current message if one page to be
> handed to tcp_sendmsg_locked() fails that test. Wouldn't that
> be sufficient?

So that's indeed the "some other code using sendpage_ok() elsewhere prevents
this from happening" case. Excellent, thanks. I thought I was being careful
by introducing the warning in -next without actually making the pages frozen
at the same time, but Matthew was of course rightfully doubting that
approach. I will adjust it to do both then and then this case will be resolved.

Thanks,
Vlastimil

> Thanks,
> Bernard.
>