Message-ID: <01fa5b0f-d209-6eb5-9911-b7522eb491d6@intel.com>
Date:   Fri, 23 Sep 2022 10:59:35 -0700
From:   Anirudh Venkataramanan <anirudh.venkataramanan@...el.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
        Ira Weiny <ira.weiny@...el.com>
CC:     Alexander Duyck <alexander.duyck@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Netdev <netdev@...r.kernel.org>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        Alexander Duyck <alexanderduyck@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Alexei Starovoitov <ast@...nel.org>, bpf <bpf@...r.kernel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Tony Nguyen <anthony.l.nguyen@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH] ixgbe: Use kmap_local_page in
 ixgbe_check_lbtest_frame()

On 9/23/2022 8:05 AM, Fabio M. De Francesco wrote:
> Hi Anirudh,
> 
> On Friday, September 23, 2022 12:38:02 AM CEST Anirudh Venkataramanan wrote:
>> On 9/22/2022 1:58 PM, Alexander Duyck wrote:
>>> On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
>>> <anirudh.venkataramanan@...el.com> wrote:
>>>>
>>>>
>>>> Following Fabio's patches, I made similar changes for e1000/e1000e and
>>>> submitted them to IWL [1].
> 
> I saw your patches and they look good to me. I might comment and probably
> review them, however I prefer to wait for Ira to do that. Furthermore, looking
> again at your patches made me recall that I need to talk with him about
> something that is only indirectly related with you work.
> 
> Please don't rely on older patches of mine as models for your next patches. In
> the last months I changed many things in the way I handle the removal of
> kmap() in favour of a plain page_address() or decide to convert to
> kmap_local_page(). Obviously I'm talking about pages which cannot come from
> ZONE_HIGHMEM.
> 
>>>> Yesterday, Ira Weiny pointed me to some feedback from Dave Hansen on the
>>>> use of page_address() [2]. My understanding of this feedback is that
>>>> it's safer to use kmap_local_page() instead of page_address(), because
>>>> you don't always know how the underlying page was allocated.
> 
> Your understanding of Dave's message is absolutely correct.
> 
>>>> This approach (of using kmap_local_page() instead of page_address())
>>>> makes sense to me. Any reason not to go this way?
> 
>>>> [1]
>>>>
>>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/
> 20220919180949.388785-1-anirudh.venkataramanan@...el.com/
>>>>
>>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/
> 20220919180949.388785-2-anirudh.venkataramanan@...el.com/
>>>>
>>>> [2]
>>>> https://lore.kernel.org/lkml/5d667258-b58b-3d28-3609-e7914c99b31b@intel.com/
>>>>
>>>> Ani
>>>
>>> For the two patches you referenced the driver is the one allocating
>>> the pages. So in such a case the page_address should be acceptable.
>>> Specifically we are falling into alloc_page(GFP_ATOMIC) which should
>>> fall into the first case that Dave Hansen called out.
>>
>> Right. However, I did run into a case in the chelsio inline crypto
>> driver where it seems like the pages are allocated outside the driver.
>> In such cases, kmap_local_page() would be the right approach, as the
>> driver can't make assumptions on how the page was allocated.
> 
> The mere fact that we are still discussing this particular topic is my only
> fault. I mean that the guidelines about what to do with ZONE_NORMAL or lower
> pages is not enough clear. I'll have to improve that paragraph.
> 
> For now let me tell you what I'm doing whenever I have to decide between a
> conversion  from kmap{,_atomic}() to kmap_local_page() or a kmap() removal  in
> favour of page_address() use.
> 
>> ... and this makes me wonder why not just use kmap_local_page() even in
>> cases where the page allocation was done in the driver. IMO, this is
>> simpler because
>>
>> a) you don't have to care how a page was allocated. kmap_local_page()
>> will create a temporary mapping if required, if not it just becomes a
>> wrapper to page_address().
>>
>> b) should a future patch change the allocation to be from highmem, you
>> don't have to change a bunch of page_address() calls to be
>> kmap_local_page().
> 
> "a" and "b" are good arguments with sound logic. However there are a couple of
> cases that you are not yet considering.
> 
> As my main rule I prefer the use of kmap_local_page() whenever tracking if
> pages can't come from Highmem is complex, especially when allocation is
> performed in other translation units of the same driver or, worse, pages come
> from different subsystems.
> 
> Instead, I don't like to use kmap_local_page() when the allocation is in the
> same function and you see immediately that it cannot come from ZONE_HIGHMEM.
> 
> Sometimes it's so clear that using kmap_local_page() looks silly to me :-)
> For example...
> 
> void *silly_alloc_and_map() {
>           	struct *page;
> 	
> 	page = alloc_page(GFP_KERNEL);
> 	return kmap_local_page(page);
> }
> 
> In this case you know without any effort that the page cannot come from
> ZONE_HIGHMEM. Therefore, why bother with mapping and unmapping (and perhaps
> write a function for unmapping).

That's fair. When I suggested using kmap_local_page() even for 
driver-local page allocations, I was thinking of situations where the 
page allocation and mapping/access happen in different functions in the 
same driver. Not that these are impossible to trace, just takes some 
more time and effort.

> 
> While working on the removals or the conversions of kmap(), I noticed that
> people tend to forget to call kunmap(). We have a limited amount of kmap()
> slots. If the mapping space is fully utilized we'll have the next slot
> available only after reboot or unloading and reloading a module.
> 
> If I recall correctly, with kmap_local_page() we can map a maximum of 16 pages
> per task_struct. Therefore, limits are everywhere and people tend to leak
> resources.
> 
> To summarize: whenever allocation is easily trackable, and pages cannot come
> from ZONE_HIGHMEM, I prefer page_address().

How would you define "easily track-able"? Does it make more sense to 
instead say "if page allocation is module-local and can't come from 
highmem, then use page_address()".

> 
> Honestly, if code is well designed I don't care whether or not within 5 days
> or 10 years decide to change the allocation. I think it's like to refrain from
> deleting unreachable code, variables, partially implemented functions, and so
> on just because one day someone may think to make something useful from those
> things.

(a) is the primary reason to use kmap_local_page(). (b) is a co-traveler.

> 
> Greg K-H taught me that I must see the code as is now and don't speculate
> about possible future scenarios. I agree with him in full :-)
> 
> Very different case where I _need_ page_address() are due to the strict rules
> of nesting mapping and unmapping-mapping. I recall that I spent days on a
> function in Btrfs because I could not map and unmap with the usual Last In -
> First Out (LIFO) rule.

Right, so maybe instead of me saying "use kmap_local_page() everywhere" 
I should have said "kmap_local_page() should be preferred where possible".

To summarize, how about this for a guideline?

- For module-local page allocations that can't come from highmem, using 
page_address() is acceptable.

- For pages that are allocated outside the module but passed to the 
module, use the appropriate kmap() function.

Ani

Powered by blists - more mailing lists