Re: [Intel-wired-lan] [PATCH] ixgbe: Use kmap_local_page in ixgbe_check_lbtest_frame()

[Alexander Duyck]

On Thu, Sep 22, 2022 at 3:38 PM Anirudh Venkataramanan
<anirudh.venkataramanan@xxxxxxxxx> 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].
> >>
> >> 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.
> >>
> >> 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.

Right, but that is comparing apples and oranges. As I said for Tx it
would make sense, but since we are doing the allocations for Rx that
isn't the case so we don't need it.

> ... 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().
>
> Is using page_address() directly beneficial in some way?

By that argument why don't we just leave the code alone and keep using
kmap? I am pretty certain that is the logic that had us using kmap in
the first place since it also dumps us with page_address in most cases
and we didn't care much about the other architectures. If you look at
the kmap_local_page() it just adds an extra step or two to calling
page_address(). In this case it is adding extra complication to
something that isn't needed which is the reason why we are going
through this in the first place. If we are going to pull the bandage I
suggest we might as well just go all the way and not take a half-step
since we don't actually need kmap or its related calls for this.