The use of kmap() is being deprecated in favor of kmap_local_page().
With kmap_local_page(), the mapping is per thread, CPU local and not
globally visible. Furthermore, the mapping can be acquired from any context
(including interrupts).
Therefore, use kmap_local_page() in ixgbe_check_lbtest_frame() because
this mapping is per thread, CPU local, and not globally visible.
Suggested-by: Ira Weiny <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 628d0eb0599f..e64d40482bfd 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1966,14 +1966,14 @@ static bool ixgbe_check_lbtest_frame(struct ixgbe_rx_buffer *rx_buffer,
frame_size >>= 1;
- data = kmap(rx_buffer->page) + rx_buffer->page_offset;
+ data = kmap_local_page(rx_buffer->page) + rx_buffer->page_offset;
if (data[3] != 0xFF ||
data[frame_size + 10] != 0xBE ||
data[frame_size + 12] != 0xAF)
match = false;
- kunmap(rx_buffer->page);
+ kunmap_local(data);
return match;
}
--
2.36.1
On Wed, Jun 29, 2022 at 10:58:36AM +0200, Fabio M. De Francesco wrote:
> The use of kmap() is being deprecated in favor of kmap_local_page().
>
> With kmap_local_page(), the mapping is per thread, CPU local and not
> globally visible. Furthermore, the mapping can be acquired from any context
> (including interrupts).
>
> Therefore, use kmap_local_page() in ixgbe_check_lbtest_frame() because
> this mapping is per thread, CPU local, and not globally visible.
Hi,
I'd like to ask why kmap was there in the first place and not plain
page_address() ?
Alex?
>
> Suggested-by: Ira Weiny <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> index 628d0eb0599f..e64d40482bfd 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> @@ -1966,14 +1966,14 @@ static bool ixgbe_check_lbtest_frame(struct ixgbe_rx_buffer *rx_buffer,
>
> frame_size >>= 1;
>
> - data = kmap(rx_buffer->page) + rx_buffer->page_offset;
> + data = kmap_local_page(rx_buffer->page) + rx_buffer->page_offset;
>
> if (data[3] != 0xFF ||
> data[frame_size + 10] != 0xBE ||
> data[frame_size + 12] != 0xAF)
> match = false;
>
> - kunmap(rx_buffer->page);
> + kunmap_local(data);
>
> return match;
> }
> --
> 2.36.1
>
On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
<[email protected]> wrote:
>
> On Wed, Jun 29, 2022 at 10:58:36AM +0200, Fabio M. De Francesco wrote:
> > The use of kmap() is being deprecated in favor of kmap_local_page().
> >
> > With kmap_local_page(), the mapping is per thread, CPU local and not
> > globally visible. Furthermore, the mapping can be acquired from any context
> > (including interrupts).
> >
> > Therefore, use kmap_local_page() in ixgbe_check_lbtest_frame() because
> > this mapping is per thread, CPU local, and not globally visible.
>
> Hi,
>
> I'd like to ask why kmap was there in the first place and not plain
> page_address() ?
>
> Alex?
The page_address function only works on architectures that have access
to all of physical memory via virtual memory addresses. The kmap
function is meant to take care of highmem which will need to be mapped
before it can be accessed.
For non-highmem pages kmap just calls the page_address function.
https://elixir.bootlin.com/linux/latest/source/include/linux/highmem-internal.h#L40
Thanks,
- Alex
On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
<[email protected]> wrote:
>
> On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> <[email protected]> wrote:
> >
> > On Wed, Jun 29, 2022 at 10:58:36AM +0200, Fabio M. De Francesco wrote:
> > > The use of kmap() is being deprecated in favor of kmap_local_page().
> > >
> > > With kmap_local_page(), the mapping is per thread, CPU local and not
> > > globally visible. Furthermore, the mapping can be acquired from any context
> > > (including interrupts).
> > >
> > > Therefore, use kmap_local_page() in ixgbe_check_lbtest_frame() because
> > > this mapping is per thread, CPU local, and not globally visible.
> >
> > Hi,
> >
> > I'd like to ask why kmap was there in the first place and not plain
> > page_address() ?
> >
> > Alex?
>
> The page_address function only works on architectures that have access
> to all of physical memory via virtual memory addresses. The kmap
> function is meant to take care of highmem which will need to be mapped
> before it can be accessed.
>
> For non-highmem pages kmap just calls the page_address function.
> https://elixir.bootlin.com/linux/latest/source/include/linux/highmem-internal.h#L40
Sure, but drivers/net/ethernet/intel/ixgbe/ixgbe_main.c is allocating
pages that are not highmem ?
This kmap() does not seem needed.
On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <[email protected]> wrote:
>
> On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
> <[email protected]> wrote:
> >
> > On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> > <[email protected]> wrote:
> > >
> > > On Wed, Jun 29, 2022 at 10:58:36AM +0200, Fabio M. De Francesco wrote:
> > > > The use of kmap() is being deprecated in favor of kmap_local_page().
> > > >
> > > > With kmap_local_page(), the mapping is per thread, CPU local and not
> > > > globally visible. Furthermore, the mapping can be acquired from any context
> > > > (including interrupts).
> > > >
> > > > Therefore, use kmap_local_page() in ixgbe_check_lbtest_frame() because
> > > > this mapping is per thread, CPU local, and not globally visible.
> > >
> > > Hi,
> > >
> > > I'd like to ask why kmap was there in the first place and not plain
> > > page_address() ?
> > >
> > > Alex?
> >
> > The page_address function only works on architectures that have access
> > to all of physical memory via virtual memory addresses. The kmap
> > function is meant to take care of highmem which will need to be mapped
> > before it can be accessed.
> >
> > For non-highmem pages kmap just calls the page_address function.
> > https://elixir.bootlin.com/linux/latest/source/include/linux/highmem-internal.h#L40
>
>
> Sure, but drivers/net/ethernet/intel/ixgbe/ixgbe_main.c is allocating
> pages that are not highmem ?
>
> This kmap() does not seem needed.
Good point. So odds are page_address is fine to use. Actually there is
a note to that effect in ixgbe_pull_tail.
As such we could probably go through and update igb, and several of
the other Intel drivers as well.
- Alex
On giovedì 30 giugno 2022 17:17:24 CEST Alexander Duyck wrote:
> On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> <[email protected]> wrote:
> >
> > On Wed, Jun 29, 2022 at 10:58:36AM +0200, Fabio M. De Francesco wrote:
> > > The use of kmap() is being deprecated in favor of kmap_local_page().
> > >
> > > With kmap_local_page(), the mapping is per thread, CPU local and not
> > > globally visible. Furthermore, the mapping can be acquired from any
context
> > > (including interrupts).
> > >
> > > Therefore, use kmap_local_page() in ixgbe_check_lbtest_frame()
because
> > > this mapping is per thread, CPU local, and not globally visible.
> >
> > Hi,
> >
> > I'd like to ask why kmap was there in the first place and not plain
> > page_address() ?
> >
> > Alex?
>
> The page_address function only works on architectures that have access
> to all of physical memory via virtual memory addresses. The kmap
> function is meant to take care of highmem which will need to be mapped
> before it can be accessed.
>
> For non-highmem pages kmap just calls the page_address function.
> https://elixir.bootlin.com/linux/latest/source/include/linux/highmem-internal.h#L40
Please take a look at documentation (highmem.rst). I've recently reworked
it and added information about kmap_local_page()
Thanks,
Fabio
>
> Thanks,
>
> - Alex
>
On giovedì 30 giugno 2022 18:09:18 CEST Alexander Duyck wrote:
> On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <[email protected]> wrote:
> >
> > On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
> > <[email protected]> wrote:
> > >
> > > On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> > > <[email protected]> wrote:
> > > >
> > > > On Wed, Jun 29, 2022 at 10:58:36AM +0200, Fabio M. De Francesco
wrote:
> > > > > The use of kmap() is being deprecated in favor of
kmap_local_page().
> > > > >
> > > > > With kmap_local_page(), the mapping is per thread, CPU local and
not
> > > > > globally visible. Furthermore, the mapping can be acquired from
any context
> > > > > (including interrupts).
> > > > >
> > > > > Therefore, use kmap_local_page() in ixgbe_check_lbtest_frame()
because
> > > > > this mapping is per thread, CPU local, and not globally visible.
> > > >
> > > > Hi,
> > > >
> > > > I'd like to ask why kmap was there in the first place and not plain
> > > > page_address() ?
> > > >
> > > > Alex?
> > >
> > > The page_address function only works on architectures that have
access
> > > to all of physical memory via virtual memory addresses. The kmap
> > > function is meant to take care of highmem which will need to be
mapped
> > > before it can be accessed.
> > >
> > > For non-highmem pages kmap just calls the page_address function.
> > > https://elixir.bootlin.com/linux/latest/source/include/linux/highmem-internal.h#L40
> >
> >
> > Sure, but drivers/net/ethernet/intel/ixgbe/ixgbe_main.c is allocating
> > pages that are not highmem ?
> >
> > This kmap() does not seem needed.
>
> Good point. So odds are page_address is fine to use. Actually there is
> a note to that effect in ixgbe_pull_tail.
>
> As such we could probably go through and update igb, and several of
> the other Intel drivers as well.
>
> - Alex
>
I don't know this code, however I know kmap*().
I assumed that, if author used kmap(), there was possibility that the page
came from highmem.
In that case kmap_local_page() looks correct here.
However, now I read that that page _cannot_ come from highmem. Therefore,
page_address() would suffice.
If you all want I can replace kmap() / kunmap() with a "plain"
page_address(). Please let me know.
Thanks,
Fabio
On Thu, Jun 30, 2022 at 11:18 AM Fabio M. De Francesco
<[email protected]> wrote:
>
> On giovedì 30 giugno 2022 18:09:18 CEST Alexander Duyck wrote:
> > On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <[email protected]> wrote:
> > >
> > > On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
> > > <[email protected]> wrote:
> > > >
> > > > On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Wed, Jun 29, 2022 at 10:58:36AM +0200, Fabio M. De Francesco
> wrote:
> > > > > > The use of kmap() is being deprecated in favor of
> kmap_local_page().
> > > > > >
> > > > > > With kmap_local_page(), the mapping is per thread, CPU local and
> not
> > > > > > globally visible. Furthermore, the mapping can be acquired from
> any context
> > > > > > (including interrupts).
> > > > > >
> > > > > > Therefore, use kmap_local_page() in ixgbe_check_lbtest_frame()
> because
> > > > > > this mapping is per thread, CPU local, and not globally visible.
> > > > >
> > > > > Hi,
> > > > >
> > > > > I'd like to ask why kmap was there in the first place and not plain
> > > > > page_address() ?
> > > > >
> > > > > Alex?
> > > >
> > > > The page_address function only works on architectures that have
> access
> > > > to all of physical memory via virtual memory addresses. The kmap
> > > > function is meant to take care of highmem which will need to be
> mapped
> > > > before it can be accessed.
> > > >
> > > > For non-highmem pages kmap just calls the page_address function.
> > > > https://elixir.bootlin.com/linux/latest/source/include/linux/highmem-internal.h#L40
> > >
> > >
> > > Sure, but drivers/net/ethernet/intel/ixgbe/ixgbe_main.c is allocating
> > > pages that are not highmem ?
> > >
> > > This kmap() does not seem needed.
> >
> > Good point. So odds are page_address is fine to use. Actually there is
> > a note to that effect in ixgbe_pull_tail.
> >
> > As such we could probably go through and update igb, and several of
> > the other Intel drivers as well.
> >
> > - Alex
> >
> I don't know this code, however I know kmap*().
>
> I assumed that, if author used kmap(), there was possibility that the page
> came from highmem.
>
> In that case kmap_local_page() looks correct here.
>
> However, now I read that that page _cannot_ come from highmem. Therefore,
> page_address() would suffice.
>
> If you all want I can replace kmap() / kunmap() with a "plain"
> page_address(). Please let me know.
>
> Thanks,
>
> Fabio
Replacing it with just page_address() should be fine. Back when I
wrote the code I didn't realize that GFP_ATOMIC pages weren't
allocated from highmem so I suspect I just used kmap since it was the
way to cover all the bases.
Thanks,
- Alex
On giovedì 30 giugno 2022 23:59:23 CEST Alexander Duyck wrote:
> On Thu, Jun 30, 2022 at 11:18 AM Fabio M. De Francesco
> <[email protected]> wrote:
> >
> > On giovedì 30 giugno 2022 18:09:18 CEST Alexander Duyck wrote:
> > > On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <[email protected]>
wrote:
> > > >
> > > > On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, Jun 29, 2022 at 10:58:36AM +0200, Fabio M. De Francesco
> > wrote:
> > > > > > > The use of kmap() is being deprecated in favor of
> > kmap_local_page().
> > > > > > >
> > > > > > > With kmap_local_page(), the mapping is per thread, CPU local
and
> > not
> > > > > > > globally visible. Furthermore, the mapping can be acquired
from
> > any context
> > > > > > > (including interrupts).
> > > > > > >
> > > > > > > Therefore, use kmap_local_page() in
ixgbe_check_lbtest_frame()
> > because
> > > > > > > this mapping is per thread, CPU local, and not globally
visible.
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > I'd like to ask why kmap was there in the first place and not
plain
> > > > > > page_address() ?
> > > > > >
> > > > > > Alex?
> > > > >
> > > > > The page_address function only works on architectures that have
> > access
> > > > > to all of physical memory via virtual memory addresses. The kmap
> > > > > function is meant to take care of highmem which will need to be
> > mapped
> > > > > before it can be accessed.
> > > > >
> > > > > For non-highmem pages kmap just calls the page_address function.
> > > > > https://elixir.bootlin.com/linux/latest/source/include/linux/
highmem-internal.h#L40
> > > >
> > > >
> > > > Sure, but drivers/net/ethernet/intel/ixgbe/ixgbe_main.c is
allocating
> > > > pages that are not highmem ?
> > > >
> > > > This kmap() does not seem needed.
> > >
> > > Good point. So odds are page_address is fine to use. Actually there
is
> > > a note to that effect in ixgbe_pull_tail.
> > >
> > > As such we could probably go through and update igb, and several of
> > > the other Intel drivers as well.
> > >
> > > - Alex
> > >
> > I don't know this code, however I know kmap*().
> >
> > I assumed that, if author used kmap(), there was possibility that the
page
> > came from highmem.
> >
> > In that case kmap_local_page() looks correct here.
> >
> > However, now I read that that page _cannot_ come from highmem.
Therefore,
> > page_address() would suffice.
> >
> > If you all want I can replace kmap() / kunmap() with a "plain"
> > page_address(). Please let me know.
> >
> > Thanks,
> >
> > Fabio
>
> Replacing it with just page_address() should be fine. Back when I
> wrote the code I didn't realize that GFP_ATOMIC pages weren't
> allocated from highmem so I suspect I just used kmap since it was the
> way to cover all the bases.
>
> Thanks,
>
> - Alex
>
OK, I'm about to prepare another patch with page_address() (obviously, this
should be discarded).
Last thing... Is that page allocated with dma_pool_alloc() at
ixgbe/ixgbe_fcoe.c:196? Somewhere else?
Thanks,
Fabio
P.S.: Can you say something about how pages are allocated in intel/e1000
and in intel/e1000e? I see that those drivers use kmap_atomic().
> -----Original Message-----
> From: Intel-wired-lan <[email protected]> On Behalf Of
> Fabio M. De Francesco
> Sent: Wednesday, June 29, 2022 2:29 PM
> To: Brandeburg, Jesse <[email protected]>; Nguyen, Anthony L
> <[email protected]>; David S. Miller <[email protected]>;
> Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>;
> Paolo Abeni <[email protected]>; Alexei Starovoitov <[email protected]>;
> Daniel Borkmann <[email protected]>; Jesper Dangaard Brouer
> <[email protected]>; John Fastabend <[email protected]>; intel-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Cc: Weiny, Ira <[email protected]>; Fabio M. De Francesco
> <[email protected]>
> Subject: [Intel-wired-lan] [PATCH] ixgbe: Use kmap_local_page in
> ixgbe_check_lbtest_frame()
>
> The use of kmap() is being deprecated in favor of kmap_local_page().
>
> With kmap_local_page(), the mapping is per thread, CPU local and not
> globally visible. Furthermore, the mapping can be acquired from any context
> (including interrupts).
>
> Therefore, use kmap_local_page() in ixgbe_check_lbtest_frame() because
> this mapping is per thread, CPU local, and not globally visible.
>
> Suggested-by: Ira Weiny <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Tested-by: Gurucharan <[email protected]> (A Contingent worker at Intel)
On 7/1/2022 8:36 AM, Fabio M. De Francesco wrote:
> On giovedì 30 giugno 2022 23:59:23 CEST Alexander Duyck wrote:
>> On Thu, Jun 30, 2022 at 11:18 AM Fabio M. De Francesco
>> <[email protected]> wrote:
>>>
>>> On giovedì 30 giugno 2022 18:09:18 CEST Alexander Duyck wrote:
>>>> On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <[email protected]>
> wrote:
>>>>>
>>>>> On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>> On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>> On Wed, Jun 29, 2022 at 10:58:36AM +0200, Fabio M. De Francesco
>>> wrote:
>>>>>>>> The use of kmap() is being deprecated in favor of
>>> kmap_local_page().
>>>>>>>>
>>>>>>>> With kmap_local_page(), the mapping is per thread, CPU local
> and
>>> not
>>>>>>>> globally visible. Furthermore, the mapping can be acquired
> from
>>> any context
>>>>>>>> (including interrupts).
>>>>>>>>
>>>>>>>> Therefore, use kmap_local_page() in
> ixgbe_check_lbtest_frame()
>>> because
>>>>>>>> this mapping is per thread, CPU local, and not globally
> visible.
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> I'd like to ask why kmap was there in the first place and not
> plain
>>>>>>> page_address() ?
>>>>>>>
>>>>>>> Alex?
>>>>>>
>>>>>> The page_address function only works on architectures that have
>>> access
>>>>>> to all of physical memory via virtual memory addresses. The kmap
>>>>>> function is meant to take care of highmem which will need to be
>>> mapped
>>>>>> before it can be accessed.
>>>>>>
>>>>>> For non-highmem pages kmap just calls the page_address function.
>>>>>> https://elixir.bootlin.com/linux/latest/source/include/linux/
> highmem-internal.h#L40
>>>>>
>>>>>
>>>>> Sure, but drivers/net/ethernet/intel/ixgbe/ixgbe_main.c is
> allocating
>>>>> pages that are not highmem ?
>>>>>
>>>>> This kmap() does not seem needed.
>>>>
>>>> Good point. So odds are page_address is fine to use. Actually there
> is
>>>> a note to that effect in ixgbe_pull_tail.
>>>>
>>>> As such we could probably go through and update igb, and several of
>>>> the other Intel drivers as well.
>>>>
>>>> - Alex
>>>>
>>> I don't know this code, however I know kmap*().
>>>
>>> I assumed that, if author used kmap(), there was possibility that the
> page
>>> came from highmem.
>>>
>>> In that case kmap_local_page() looks correct here.
>>>
>>> However, now I read that that page _cannot_ come from highmem.
> Therefore,
>>> page_address() would suffice.
>>>
>>> If you all want I can replace kmap() / kunmap() with a "plain"
>>> page_address(). Please let me know.
>>>
>>> Thanks,
>>>
>>> Fabio
>>
>> Replacing it with just page_address() should be fine. Back when I
>> wrote the code I didn't realize that GFP_ATOMIC pages weren't
>> allocated from highmem so I suspect I just used kmap since it was the
>> way to cover all the bases.
>>
>> Thanks,
>>
>> - Alex
>>
>
> OK, I'm about to prepare another patch with page_address() (obviously, this
> should be discarded).
>
> Last thing... Is that page allocated with dma_pool_alloc() at
> ixgbe/ixgbe_fcoe.c:196? Somewhere else?
>
> Thanks,
>
> Fabio
>
> P.S.: Can you say something about how pages are allocated in intel/e1000
> and in intel/e1000e? I see that those drivers use kmap_atomic().
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/[email protected]/
https://patchwork.ozlabs.org/project/intel-wired-lan/patch/[email protected]/
[2]
https://lore.kernel.org/lkml/[email protected]/
Ani
On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
<[email protected]> 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/[email protected]/
>
> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/[email protected]/
>
> [2]
> https://lore.kernel.org/lkml/[email protected]/
>
> 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.
If it was the Tx path that would be another matter, however these are
Rx only pages so they are allocated by the driver directly and won't
be allocated from HIGHMEM.
On 9/22/2022 1:58 PM, Alexander Duyck wrote:
> On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
> <[email protected]> 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/[email protected]/
>>
>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/[email protected]/
>>
>> [2]
>> https://lore.kernel.org/lkml/[email protected]/
>>
>> 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.
... 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?
Ani
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
> > <[email protected]> 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/
[email protected]/
> >>
> >> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/
[email protected]/
> >>
> >> [2]
> >> https://lore.kernel.org/lkml/[email protected]/
> >>
> >> 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).
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().
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.
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.
A function was so complex and convoluted that nobody could know in advance the
order of execution of the mappings of two pages. Lots of goto, breaks, loops
made impossible to unmap in the correct order at the "clean and exit" label.
I made a first attempt using a two element array as a stack which registered
the mappings and then I used it to unmap in the correct order at exit.
It was intentionally a means to draw the attention of the maintainers. One of
them proposed to split that very complex function in several helpers, and
isolate the mappings one by one. It was OK to me.
After weeks, David Sterba noticed that he knew that one of the pages came from
the page cache and we had to map it, but the second page was allocated inside
Btrfs with GFP_KERNEL. Therefore, the better suited solution was to use
kmap_local_page() for the first and page address() for the second.
My stack based solution was working but nobody should write such an ugly code
just to enforce local mapping :-)
> Is using page_address() directly beneficial in some way?
A possible call chain on 32 bits kernels is the following:
kmap_local_page() ->
__kmap_local_page_prot() {
if (!IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) && |
PageHighMem(page))
return page_address(page);
....
}
How many instructions can you save calling page_address() directly?
If you don't know, look at the assembly.
Thanks,
Fabio
> Ani
>
>
On Thu, Sep 22, 2022 at 3:38 PM Anirudh Venkataramanan
<[email protected]> wrote:
>
> On 9/22/2022 1:58 PM, Alexander Duyck wrote:
> > On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
> > <[email protected]> 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/[email protected]/
> >>
> >> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/[email protected]/
> >>
> >> [2]
> >> https://lore.kernel.org/lkml/[email protected]/
> >>
> >> 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.
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
>>> <[email protected]> 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/
> [email protected]/
>>>>
>>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/
> [email protected]/
>>>>
>>>> [2]
>>>> https://lore.kernel.org/lkml/[email protected]/
>>>>
>>>> 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
On 9/23/2022 8:31 AM, Alexander Duyck wrote:
> On Thu, Sep 22, 2022 at 3:38 PM Anirudh Venkataramanan
> <[email protected]> wrote:
>>
>> On 9/22/2022 1:58 PM, Alexander Duyck wrote:
>>> On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
>>> <[email protected]> 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/[email protected]/
>>>>
>>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/[email protected]/
>>>>
>>>> [2]
>>>> https://lore.kernel.org/lkml/[email protected]/
>>>>
>>>> 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.
Well, my understanding is that kmap_local_page() doesn't have the
overheads kmap() has, and that alone is reason enough to replace kmap()
and kmap_atomic() with kmap_local_page() where possible.
> 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.
I don't really see this as "pulling the kmap() bandage", but a "use a
more appropriate kmap function if you can" type situation.
FWIW, I am not against using page_address(). Just wanted to hash this
out and get to a conclusion before I made new changes.
Ani
On Fri, Sep 23, 2022 at 11:51 AM Anirudh Venkataramanan
<[email protected]> wrote:
>
> On 9/23/2022 8:31 AM, Alexander Duyck wrote:
> > On Thu, Sep 22, 2022 at 3:38 PM Anirudh Venkataramanan
> > <[email protected]> wrote:
> >>
> >> On 9/22/2022 1:58 PM, Alexander Duyck wrote:
> >>> On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
> >>> <[email protected]> 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/[email protected]/
> >>>>
> >>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/[email protected]/
> >>>>
> >>>> [2]
> >>>> https://lore.kernel.org/lkml/[email protected]/
> >>>>
> >>>> 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.
>
> Well, my understanding is that kmap_local_page() doesn't have the
> overheads kmap() has, and that alone is reason enough to replace kmap()
> and kmap_atomic() with kmap_local_page() where possible.
It has less overhead, but there is still some pretty significant code
involved. Basically in the cases where it can't bail out and just call
page_address it will call __kmap_local_page_prot(),
https://elixir.bootlin.com/linux/v6.0-rc4/source/mm/highmem.c#L517.
> > 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.
>
> I don't really see this as "pulling the kmap() bandage", but a "use a
> more appropriate kmap function if you can" type situation.
My concern is that it is more of a half step in the case of the
e1000/e1000e drivers. We likely should have fixed this some time ago
when I had rewritten the Rx path for the igb and ixgbe drivers, but I
just didn't get around to it because if I messed with other areas it
would have required more validation. I'd rather not carry around the
extra code or function calls if we don't need it.
> FWIW, I am not against using page_address(). Just wanted to hash this
> out and get to a conclusion before I made new changes.
>
> Ani
I gathered as much based on your other conversation. This is
essentially the module-local case you had referred to in which the
page is allocated and used within the module so there is no need to be
concerned about it possibly being a highmem page.
Thanks,
- Alex
On Friday, September 23, 2022 5:05:43 PM CEST 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
> > > <[email protected]> wrote:
[snip]
> > Is using page_address() directly beneficial in some way?
>
> A possible call chain on 32 bits kernels is the following:
>
> kmap_local_page() ->
> __kmap_local_page_prot() {
> if (!IS_ENABLED(CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP) && |
> PageHighMem(page))
> return page_address(page);
>
> ....
> }
>
> How many instructions can you save calling page_address() directly?
> If you don't know, look at the assembly.
I just realized that perhaps you were expecting something like either "No, it
is not directly beneficial because []" or "Yes, it is directly beneficial
because []".
Instead, I used a rhetoric question that might not have been so clear as I
thought. This kind of construct is so largely used in my native language, that
nobody might misunderstand. I'm not so sure if it is the same in English.
I mean, are those dozen "unnecessary" further assembly instructions too many
or too few to care about? I _think_ that they are too many.
Therefore, by showing a possible call chain in 32 bits architectures, I
indirectly responded "no, I can't see any direct benefit", at least because....
1) Whatever the architecture, if pages can't come from Highmem, code always
ends up calling page_address(). In 32 bits archs they waste precious kernel
stack space (a scarce resources) only to build two stack frames (one per each
called functions).
2) Developers adds further work to the CPU and force the kernel to run
unnecessary code.
I'll always use page_address() when I can "prove" that the allocation cannot
come from ZONE_HIGHMEM.
Thanks,
Fabio