[PATCH] ixgbe: Use kmap_local_page in ixgbe_check_lbtest_frame()

[PATCH] ixgbe: Use kmap_local_page in ixgbe_check_lbtest_frame()

[Fabio M. De Francesco]

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 <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 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

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

[Fabio M. De Francesco]

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 <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 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

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [PATCH] ixgbe: Use kmap_local_page in ixgbe_check_lbtest_frame()

[Maciej Fijalkowski]

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 <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  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
> 

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

[Maciej Fijalkowski]

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 <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  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
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Alexander Duyck]

On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> 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

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

[Alexander Duyck]

On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> 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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Maciej Fijalkowski]

On Thu, Jun 30, 2022 at 08:17:24AM -0700, Alexander Duyck wrote:
> On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> 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

I knew the second part but not the first, thanks.
So basically it is advised to convert the page_address() usage in similar
loopback testing code that other Intel drivers have, I'll do that later.

> 
> Thanks,
> 
> - Alex

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

[Maciej Fijalkowski]

On Thu, Jun 30, 2022 at 08:17:24AM -0700, Alexander Duyck wrote:
> On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> 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

I knew the second part but not the first, thanks.
So basically it is advised to convert the page_address() usage in similar
loopback testing code that other Intel drivers have, I'll do that later.

> 
> Thanks,
> 
> - Alex
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Eric Dumazet]

On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> 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.

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

[Eric Dumazet]

On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> 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.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Alexander Duyck]

On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
> >
> > On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> > <maciej.fijalkowski@intel.com> 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

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

[Alexander Duyck]

On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
> >
> > On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> > <maciej.fijalkowski@intel.com> 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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Fabio M. De Francesco]

On giovedì 30 giugno 2022 18:09:18 CEST Alexander Duyck wrote:
> On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
> > <alexander.duyck@gmail.com> wrote:
> > >
> > > On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> > > <maciej.fijalkowski@intel.com> 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

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

[Fabio M. De Francesco]

On giovedì 30 giugno 2022 18:09:18 CEST Alexander Duyck wrote:
> On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
> > <alexander.duyck@gmail.com> wrote:
> > >
> > > On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> > > <maciej.fijalkowski@intel.com> 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



_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Alexander Duyck]

On Thu, Jun 30, 2022 at 11:18 AM Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> On giovedì 30 giugno 2022 18:09:18 CEST Alexander Duyck wrote:
> > On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
> > > <alexander.duyck@gmail.com> wrote:
> > > >
> > > > On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> > > > <maciej.fijalkowski@intel.com> 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

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

[Alexander Duyck]

On Thu, Jun 30, 2022 at 11:18 AM Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> On giovedì 30 giugno 2022 18:09:18 CEST Alexander Duyck wrote:
> > On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
> > > <alexander.duyck@gmail.com> wrote:
> > > >
> > > > On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> > > > <maciej.fijalkowski@intel.com> 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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Fabio M. De Francesco]

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
> <fmdefrancesco@gmail.com> wrote:
> >
> > On giovedì 30 giugno 2022 18:09:18 CEST Alexander Duyck wrote:
> > > On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <edumazet@google.com> 
wrote:
> > > >
> > > > On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
> > > > <alexander.duyck@gmail.com> wrote:
> > > > >
> > > > > On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> > > > > <maciej.fijalkowski@intel.com> 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().

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

[Fabio M. De Francesco]

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
> <fmdefrancesco@gmail.com> wrote:
> >
> > On giovedì 30 giugno 2022 18:09:18 CEST Alexander Duyck wrote:
> > > On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <edumazet@google.com> 
wrote:
> > > >
> > > > On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
> > > > <alexander.duyck@gmail.com> wrote:
> > > > >
> > > > > On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> > > > > <maciej.fijalkowski@intel.com> 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().



_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Anirudh Venkataramanan]

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
>> <fmdefrancesco@gmail.com> wrote:
>>>
>>> On giovedì 30 giugno 2022 18:09:18 CEST Alexander Duyck wrote:
>>>> On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <edumazet@google.com>
> wrote:
>>>>>
>>>>> On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
>>>>> <alexander.duyck@gmail.com> wrote:
>>>>>>
>>>>>> On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
>>>>>> <maciej.fijalkowski@intel.com> 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/20220919180949.388785-1-anirudh.venkataramanan@intel.com/

https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220919180949.388785-2-anirudh.venkataramanan@intel.com/

[2] 
https://lore.kernel.org/lkml/5d667258-b58b-3d28-3609-e7914c99b31b@intel.com/

Ani

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

[Anirudh Venkataramanan]

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
>> <fmdefrancesco@gmail.com> wrote:
>>>
>>> On giovedì 30 giugno 2022 18:09:18 CEST Alexander Duyck wrote:
>>>> On Thu, Jun 30, 2022 at 8:25 AM Eric Dumazet <edumazet@google.com>
> wrote:
>>>>>
>>>>> On Thu, Jun 30, 2022 at 5:17 PM Alexander Duyck
>>>>> <alexander.duyck@gmail.com> wrote:
>>>>>>
>>>>>> On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
>>>>>> <maciej.fijalkowski@intel.com> 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/20220919180949.388785-1-anirudh.venkataramanan@intel.com/

https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220919180949.388785-2-anirudh.venkataramanan@intel.com/

[2] 
https://lore.kernel.org/lkml/5d667258-b58b-3d28-3609-e7914c99b31b@intel.com/

Ani

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Alexander Duyck]

On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
<anirudh.venkataramanan@intel.com> 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@intel.com/
>
> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220919180949.388785-2-anirudh.venkataramanan@intel.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.

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.

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

[Alexander Duyck]

On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
<anirudh.venkataramanan@intel.com> 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@intel.com/
>
> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220919180949.388785-2-anirudh.venkataramanan@intel.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.

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.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Anirudh Venkataramanan]

On 9/22/2022 1:58 PM, Alexander Duyck wrote:
> On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
> <anirudh.venkataramanan@intel.com> 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@intel.com/
>>
>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220919180949.388785-2-anirudh.venkataramanan@intel.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.

... 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

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

[Anirudh Venkataramanan]

On 9/22/2022 1:58 PM, Alexander Duyck wrote:
> On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
> <anirudh.venkataramanan@intel.com> 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@intel.com/
>>
>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220919180949.388785-2-anirudh.venkataramanan@intel.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.

... 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

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Fabio M. De Francesco]

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@intel.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@intel.com/
> >>
> >> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/
20220919180949.388785-2-anirudh.venkataramanan@intel.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).

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
> 
> 

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

[Fabio M. De Francesco]

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@intel.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@intel.com/
> >>
> >> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/
20220919180949.388785-2-anirudh.venkataramanan@intel.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).

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
> 
> 




_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Anirudh Venkataramanan]

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@intel.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@intel.com/
>>>>
>>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/
> 20220919180949.388785-2-anirudh.venkataramanan@intel.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

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

[Anirudh Venkataramanan]

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@intel.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@intel.com/
>>>>
>>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/
> 20220919180949.388785-2-anirudh.venkataramanan@intel.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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Fabio M. De Francesco]

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
> > > <anirudh.venkataramanan@intel.com> 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

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

[Fabio M. De Francesco]

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
> > > <anirudh.venkataramanan@intel.com> 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



_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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@intel.com> wrote:
>
> On 9/22/2022 1:58 PM, Alexander Duyck wrote:
> > On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
> > <anirudh.venkataramanan@intel.com> 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@intel.com/
> >>
> >> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220919180949.388785-2-anirudh.venkataramanan@intel.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.

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.

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@intel.com> wrote:
>
> On 9/22/2022 1:58 PM, Alexander Duyck wrote:
> > On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
> > <anirudh.venkataramanan@intel.com> 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@intel.com/
> >>
> >> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220919180949.388785-2-anirudh.venkataramanan@intel.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.

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.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Anirudh Venkataramanan]

On 9/23/2022 8:31 AM, Alexander Duyck wrote:
> On Thu, Sep 22, 2022 at 3:38 PM Anirudh Venkataramanan
> <anirudh.venkataramanan@intel.com> wrote:
>>
>> On 9/22/2022 1:58 PM, Alexander Duyck wrote:
>>> On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
>>> <anirudh.venkataramanan@intel.com> 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@intel.com/
>>>>
>>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220919180949.388785-2-anirudh.venkataramanan@intel.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.
> 
> 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

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

[Anirudh Venkataramanan]

On 9/23/2022 8:31 AM, Alexander Duyck wrote:
> On Thu, Sep 22, 2022 at 3:38 PM Anirudh Venkataramanan
> <anirudh.venkataramanan@intel.com> wrote:
>>
>> On 9/22/2022 1:58 PM, Alexander Duyck wrote:
>>> On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
>>> <anirudh.venkataramanan@intel.com> 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@intel.com/
>>>>
>>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220919180949.388785-2-anirudh.venkataramanan@intel.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.
> 
> 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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Alexander Duyck]

On Fri, Sep 23, 2022 at 11:51 AM Anirudh Venkataramanan
<anirudh.venkataramanan@intel.com> wrote:
>
> On 9/23/2022 8:31 AM, Alexander Duyck wrote:
> > On Thu, Sep 22, 2022 at 3:38 PM Anirudh Venkataramanan
> > <anirudh.venkataramanan@intel.com> wrote:
> >>
> >> On 9/22/2022 1:58 PM, Alexander Duyck wrote:
> >>> On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
> >>> <anirudh.venkataramanan@intel.com> 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@intel.com/
> >>>>
> >>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220919180949.388785-2-anirudh.venkataramanan@intel.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.
> >
> > 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
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Alexander Duyck]

On Fri, Sep 23, 2022 at 11:51 AM Anirudh Venkataramanan
<anirudh.venkataramanan@intel.com> wrote:
>
> On 9/23/2022 8:31 AM, Alexander Duyck wrote:
> > On Thu, Sep 22, 2022 at 3:38 PM Anirudh Venkataramanan
> > <anirudh.venkataramanan@intel.com> wrote:
> >>
> >> On 9/22/2022 1:58 PM, Alexander Duyck wrote:
> >>> On Thu, Sep 22, 2022 at 1:07 PM Anirudh Venkataramanan
> >>> <anirudh.venkataramanan@intel.com> 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@intel.com/
> >>>>
> >>>> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220919180949.388785-2-anirudh.venkataramanan@intel.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.
> >
> > 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

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

[Fabio M. De Francesco]

On giovedì 30 giugno 2022 17:17:24 CEST Alexander Duyck wrote:
> On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> 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
> 

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

[Fabio M. De Francesco]

On giovedì 30 giugno 2022 17:17:24 CEST Alexander Duyck wrote:
> On Thu, Jun 30, 2022 at 3:10 AM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> 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
> 




_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[G, GurucharanX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Fabio M. De Francesco
> Sent: Wednesday, June 29, 2022 2:29 PM
> To: Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>;
> Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Alexei Starovoitov <ast@kernel.org>;
> Daniel Borkmann <daniel@iogearbox.net>; Jesper Dangaard Brouer
> <hawk@kernel.org>; John Fastabend <john.fastabend@gmail.com>; intel-
> wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; bpf@vger.kernel.org
> Cc: Weiny, Ira <ira.weiny@intel.com>; Fabio M. De Francesco
> <fmdefrancesco@gmail.com>
> 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 <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)

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

[G, GurucharanX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Fabio M. De Francesco
> Sent: Wednesday, June 29, 2022 2:29 PM
> To: Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>;
> Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>;
> Paolo Abeni <pabeni@redhat.com>; Alexei Starovoitov <ast@kernel.org>;
> Daniel Borkmann <daniel@iogearbox.net>; Jesper Dangaard Brouer
> <hawk@kernel.org>; John Fastabend <john.fastabend@gmail.com>; intel-
> wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; bpf@vger.kernel.org
> Cc: Weiny, Ira <ira.weiny@intel.com>; Fabio M. De Francesco
> <fmdefrancesco@gmail.com>
> 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 <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan