Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220531081412.22db88cc@kernel.org> <1654011382-2453-1-git-send-email-chen45464546@163.com>
 <20220531084704.480133fa@kernel.org>
In-Reply-To: <20220531084704.480133fa@kernel.org>
From:   Alexander Duyck <alexander.duyck@gmail.com>
Date:   Tue, 31 May 2022 11:28:59 -0700
Message-ID: <CAKgT0UfQsbAzsJ1e__irHY2xBRevpB9m=FBYDis3C1fMua+Zag@mail.gmail.com>
Subject: Re: [PATCH v2] mm: page_frag: Warn_on when frag_alloc size is bigger
 than PAGE_SIZE
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     Chen Lin <chen45464546@163.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-mm <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Netdev <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, May 31, 2022 at 8:47 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 31 May 2022 23:36:22 +0800 Chen Lin wrote:
> > At 2022-05-31 22:14:12, "Jakub Kicinski" <kuba@kernel.org> wrote:
> > >On Tue, 31 May 2022 22:41:12 +0800 Chen Lin wrote:
> > >> The sample code above cannot completely solve the current problem.
> > >> For example, when fragsz is greater than PAGE_FRAG_CACHE_MAX_SIZE(32768),
> > >> __page_frag_cache_refill will return a memory of only 32768 bytes, so
> > >> should we continue to expand the PAGE_FRAG_CACHE_MAX_SIZE? Maybe more
> > >> work needs to be done
> > >
> > >Right, but I can think of two drivers off the top of my head which will
> > >allocate <=32k frags but none which will allocate more.
> >
> > In fact, it is rare to apply for more than one page, so is it necessary to
> > change it to support?
>
> I don't really care if it's supported TBH, but I dislike adding
> a branch to the fast path just to catch one or two esoteric bad
> callers.
>
> Maybe you can wrap the check with some debug CONFIG_ so it won't
> run on production builds?

Also the example used here to define what is triggering the behavior
is seriously flawed. The code itself is meant to allow for order0 page
reuse, and the 32K page was just an optimization. So the assumption
that you could request more than 4k is a bad assumption in the driver
that is making this call.

So I am in agreement with Kuba. We shouldn't be needing to add code in
the fast path to tell users not to shoot themselves in the foot.

We already have code in place in __netdev_alloc_skb that is calling
the slab allocator if "len > SKB_WITH_OVERHEAD(PAGE_SIZE)". We could
probably just add a DEBUG wrapped BUG_ON to capture those cases where
a driver is making that mistake with __netdev_alloc_frag_align.