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@xxxxxxxxx> 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@xxxxxxxxx/
https://patchwork.ozlabs.org/project/intel-wired-lan/patch/
20220919180949.388785-2-anirudh.venkataramanan@xxxxxxxxx/
[2]
https://lore.kernel.org/lkml/5d667258-b58b-3d28-3609-e7914c99b31b@xxxxxxxxx/
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
