* [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
@ 2022-04-16 11:14 ` Alaa Mohamed
0 siblings, 0 replies; 19+ messages in thread
From: Alaa Mohamed @ 2022-04-16 11:14 UTC (permalink / raw)
To: outreachy
Cc: jesse.brandeburg, anthony.l.nguyen, davem, kuba, pabeni,
intel-wired-lan, netdev, linux-kernel, ira.weiny,
eng.alaamohamedsoliman.am
Convert kmap() to kmap_local_page()
With kmap_local_page(), the mapping is per thread, CPU local and not
globally visible.
Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
---
changes in V2:
fix kunmap_local path value to take address of the mapped page.
---
changes in V3:
edit commit message to be clearer
---
drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 2a5782063f4c..c14fc871dd41 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct igb_rx_buffer *rx_buffer,
frame_size >>= 1;
- data = kmap(rx_buffer->page);
+ data = kmap_local_page(rx_buffer->page);
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.35.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Intel-wired-lan] [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
@ 2022-04-16 11:14 ` Alaa Mohamed
0 siblings, 0 replies; 19+ messages in thread
From: Alaa Mohamed @ 2022-04-16 11:14 UTC (permalink / raw)
To: intel-wired-lan
Convert kmap() to kmap_local_page()
With kmap_local_page(), the mapping is per thread, CPU local and not
globally visible.
Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
---
changes in V2:
fix kunmap_local path value to take address of the mapped page.
---
changes in V3:
edit commit message to be clearer
---
drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 2a5782063f4c..c14fc871dd41 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct igb_rx_buffer *rx_buffer,
frame_size >>= 1;
- data = kmap(rx_buffer->page);
+ data = kmap_local_page(rx_buffer->page);
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.35.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
2022-04-16 11:14 ` [Intel-wired-lan] " Alaa Mohamed
@ 2022-04-16 11:31 ` Julia Lawall
-1 siblings, 0 replies; 19+ messages in thread
From: Julia Lawall @ 2022-04-16 11:31 UTC (permalink / raw)
To: Alaa Mohamed
Cc: outreachy, jesse.brandeburg, anthony.l.nguyen, davem, kuba,
pabeni, intel-wired-lan, netdev, linux-kernel, ira.weiny
On Sat, 16 Apr 2022, Alaa Mohamed wrote:
> Convert kmap() to kmap_local_page()
>
> With kmap_local_page(), the mapping is per thread, CPU local and not
> globally visible.
It's not clearer. This is a general statement about the function. You
need to explain why it is appropriate to use it here. Unless it is the
case that all calls to kmap should be converted to call kmap_local_page.
julia
>
> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
> ---
> changes in V2:
> fix kunmap_local path value to take address of the mapped page.
> ---
> changes in V3:
> edit commit message to be clearer
> ---
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 2a5782063f4c..c14fc871dd41 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct igb_rx_buffer *rx_buffer,
>
> frame_size >>= 1;
>
> - data = kmap(rx_buffer->page);
> + data = kmap_local_page(rx_buffer->page);
>
> 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.35.2
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-wired-lan] [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
@ 2022-04-16 11:31 ` Julia Lawall
0 siblings, 0 replies; 19+ messages in thread
From: Julia Lawall @ 2022-04-16 11:31 UTC (permalink / raw)
To: intel-wired-lan
On Sat, 16 Apr 2022, Alaa Mohamed wrote:
> Convert kmap() to kmap_local_page()
>
> With kmap_local_page(), the mapping is per thread, CPU local and not
> globally visible.
It's not clearer. This is a general statement about the function. You
need to explain why it is appropriate to use it here. Unless it is the
case that all calls to kmap should be converted to call kmap_local_page.
julia
>
> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
> ---
> changes in V2:
> fix kunmap_local path value to take address of the mapped page.
> ---
> changes in V3:
> edit commit message to be clearer
> ---
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 2a5782063f4c..c14fc871dd41 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct igb_rx_buffer *rx_buffer,
>
> frame_size >>= 1;
>
> - data = kmap(rx_buffer->page);
> + data = kmap_local_page(rx_buffer->page);
>
> 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.35.2
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-wired-lan] [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
2022-04-16 11:31 ` [Intel-wired-lan] " Julia Lawall
(?)
@ 2022-04-16 13:14 ` Alaa Mohamed
2022-04-18 22:19 ` [Intel-wired-lan] " Ira Weiny
-1 siblings, 1 reply; 19+ messages in thread
From: Alaa Mohamed @ 2022-04-16 13:14 UTC (permalink / raw)
To: intel-wired-lan
On ???/??/???? ??:??, Julia Lawall wrote:
>
> On Sat, 16 Apr 2022, Alaa Mohamed wrote:
>
>> Convert kmap() to kmap_local_page()
>>
>> With kmap_local_page(), the mapping is per thread, CPU local and not
>> globally visible.
> It's not clearer.
I mean this " fix kunmap_local path value to take address of the mapped
page" be more clearer
> This is a general statement about the function. You
> need to explain why it is appropriate to use it here. Unless it is the
> case that all calls to kmap should be converted to call kmap_local_page.
It's required to convert all calls kmap to kmap_local_page. So, I don't
what should the commit message be?
Is this will be good :
"kmap_local_page() was recently developed as a replacement for kmap().? The
kmap_local_page() creates a mapping which is restricted to local use by a
single thread of execution. "
>
> julia
>
>> Signed-off-by: Alaa Mohamed<eng.alaamohamedsoliman.am@gmail.com>
>> ---
>> changes in V2:
>> fix kunmap_local path value to take address of the mapped page.
>> ---
>> changes in V3:
>> edit commit message to be clearer
>> ---
>> drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> index 2a5782063f4c..c14fc871dd41 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> @@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct igb_rx_buffer *rx_buffer,
>>
>> frame_size >>= 1;
>>
>> - data = kmap(rx_buffer->page);
>> + data = kmap_local_page(rx_buffer->page);
>>
>> 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.35.2
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20220416/173e7199/attachment-0001.html>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
2022-04-16 13:14 ` Alaa Mohamed
@ 2022-04-18 22:19 ` Ira Weiny
0 siblings, 0 replies; 19+ messages in thread
From: Ira Weiny @ 2022-04-18 22:19 UTC (permalink / raw)
To: Alaa Mohamed
Cc: Julia Lawall, outreachy, jesse.brandeburg, anthony.l.nguyen,
davem, kuba, pabeni, intel-wired-lan, netdev, linux-kernel
On Sat, Apr 16, 2022 at 03:14:57PM +0200, Alaa Mohamed wrote:
> On ١٦/٤/٢٠٢٢ ١٣:٣١, Julia Lawall wrote:
>
>
> On Sat, 16 Apr 2022, Alaa Mohamed wrote:
>
>
> Convert kmap() to kmap_local_page()
>
> With kmap_local_page(), the mapping is per thread, CPU local and not
> globally visible.
>
> It's not clearer.
>
> I mean this " fix kunmap_local path value to take address of the mapped
> page" be more clearer
>
> This is a general statement about the function. You
> need to explain why it is appropriate to use it here. Unless it is the
> case that all calls to kmap should be converted to call kmap_local_page.
>
> It's required to convert all calls kmap to kmap_local_page. So, I don't
> what should the commit message be?
>
> Is this will be good :
>
> "kmap_local_page() was recently developed as a replacement for kmap().
> The
> kmap_local_page() creates a mapping which is restricted to local use by a
> single thread of execution. "
>
I think I am missing some thread context here. I'm not sure who said what
above. So I'm going to start over.
Alaa,
It is important to remember that a good commit message says 2 things.
1) What is the problem you are trying to solve
2) Overview of the solution
First off I understand your frustration. In my opinion fixes and clean ups
like this are very hard to write good commit messages for because so often the
code diff seems so self explanatory. However, each code change comes at the
identification of a problem. And remember that 'problem' does not always mean
a bug fix.
The deprecation of kmap() may not seem like a problem. I mean why can't we
just leave kmap() as it is? It works right?
But the problem is that the kmap (highmem) interface has become stale and its
original purpose was targeted toward large memory systems with 32 bit kernels.
There are very few systems being run like that any longer.
So how do we clean up the kmap interface to be more useful to the kernel
community now that 32 bit kernels with highmem are so rare?
The community has identified that a first step of that is to move away from and
eventually remove the kmap() call. This is due to the call being incorrectly
used to create long term mappings. Most calls to kmap() are not used
incorrectly but those call sites needed something in between kmap() and
kmap_atmoic(). That call is kmap_local_page().
Now that kmap_local_page() exists the kmap() calls can be audited and most (I
hope most)[1] can be replaced with kmap_local_page().
The change you have below is correct. But it lacks a good commit message. We
need to cover the 2 points above.
1) Julia is asking why you needed to do this change. What is the
problem or reason for this change? (Ira told you to is not a good
reason. ;-)
PS In fact me telling you to may actually be a very bad reason...
j/k ;-)
2) Why is this solution ok as part of the deprecation and removal of
kmap()?
A final note; the 2 above points don't need a lot of text. Here I used
2 simple sentences.
https://lore.kernel.org/lkml/20220124015409.807587-2-ira.weiny@intel.com/
I hope this helps,
Ira
[1] But not all... some uses of kmap() have been identified as being pretty
complex.
>
> julia
>
>
> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
> ---
> changes in V2:
> fix kunmap_local path value to take address of the mapped page.
> ---
> changes in V3:
> edit commit message to be clearer
> ---
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 2a5782063f4c..c14fc871dd41 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct igb_rx_buffer *rx_buffer,
>
> frame_size >>= 1;
>
> - data = kmap(rx_buffer->page);
> + data = kmap_local_page(rx_buffer->page);
>
> 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.35.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-wired-lan] [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
@ 2022-04-18 22:19 ` Ira Weiny
0 siblings, 0 replies; 19+ messages in thread
From: Ira Weiny @ 2022-04-18 22:19 UTC (permalink / raw)
To: intel-wired-lan
On Sat, Apr 16, 2022 at 03:14:57PM +0200, Alaa Mohamed wrote:
> On ???/??/???? ??:??, Julia Lawall wrote:
>
>
> On Sat, 16 Apr 2022, Alaa Mohamed wrote:
>
>
> Convert kmap() to kmap_local_page()
>
> With kmap_local_page(), the mapping is per thread, CPU local and not
> globally visible.
>
> It's not clearer.
>
> I mean this " fix kunmap_local path value to take address of the mapped
> page" be more clearer
>
> This is a general statement about the function. You
> need to explain why it is appropriate to use it here. Unless it is the
> case that all calls to kmap should be converted to call kmap_local_page.
>
> It's required to convert all calls kmap to kmap_local_page. So, I don't
> what should the commit message be?
>
> Is this will be good :
>
> "kmap_local_page() was recently developed as a replacement for kmap().
> The
> kmap_local_page() creates a mapping which is restricted to local use by a
> single thread of execution. "
>
I think I am missing some thread context here. I'm not sure who said what
above. So I'm going to start over.
Alaa,
It is important to remember that a good commit message says 2 things.
1) What is the problem you are trying to solve
2) Overview of the solution
First off I understand your frustration. In my opinion fixes and clean ups
like this are very hard to write good commit messages for because so often the
code diff seems so self explanatory. However, each code change comes at the
identification of a problem. And remember that 'problem' does not always mean
a bug fix.
The deprecation of kmap() may not seem like a problem. I mean why can't we
just leave kmap() as it is? It works right?
But the problem is that the kmap (highmem) interface has become stale and its
original purpose was targeted toward large memory systems with 32 bit kernels.
There are very few systems being run like that any longer.
So how do we clean up the kmap interface to be more useful to the kernel
community now that 32 bit kernels with highmem are so rare?
The community has identified that a first step of that is to move away from and
eventually remove the kmap() call. This is due to the call being incorrectly
used to create long term mappings. Most calls to kmap() are not used
incorrectly but those call sites needed something in between kmap() and
kmap_atmoic(). That call is kmap_local_page().
Now that kmap_local_page() exists the kmap() calls can be audited and most (I
hope most)[1] can be replaced with kmap_local_page().
The change you have below is correct. But it lacks a good commit message. We
need to cover the 2 points above.
1) Julia is asking why you needed to do this change. What is the
problem or reason for this change? (Ira told you to is not a good
reason. ;-)
PS In fact me telling you to may actually be a very bad reason...
j/k ;-)
2) Why is this solution ok as part of the deprecation and removal of
kmap()?
A final note; the 2 above points don't need a lot of text. Here I used
2 simple sentences.
https://lore.kernel.org/lkml/20220124015409.807587-2-ira.weiny at intel.com/
I hope this helps,
Ira
[1] But not all... some uses of kmap() have been identified as being pretty
complex.
>
> julia
>
>
> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
> ---
> changes in V2:
> fix kunmap_local path value to take address of the mapped page.
> ---
> changes in V3:
> edit commit message to be clearer
> ---
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 2a5782063f4c..c14fc871dd41 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct igb_rx_buffer *rx_buffer,
>
> frame_size >>= 1;
>
> - data = kmap(rx_buffer->page);
> + data = kmap_local_page(rx_buffer->page);
>
> 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.35.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
2022-04-18 22:19 ` [Intel-wired-lan] " Ira Weiny
@ 2022-04-19 13:37 ` Alaa Mohamed
-1 siblings, 0 replies; 19+ messages in thread
From: Alaa Mohamed @ 2022-04-19 13:37 UTC (permalink / raw)
To: Ira Weiny
Cc: Julia Lawall, outreachy, jesse.brandeburg, anthony.l.nguyen,
davem, kuba, pabeni, intel-wired-lan, netdev, linux-kernel
On ١٩/٤/٢٠٢٢ ٠٠:١٩, Ira Weiny wrote:
> On Sat, Apr 16, 2022 at 03:14:57PM +0200, Alaa Mohamed wrote:
>> On ١٦/٤/٢٠٢٢ ١٣:٣١, Julia Lawall wrote:
>>
>>
>> On Sat, 16 Apr 2022, Alaa Mohamed wrote:
>>
>>
>> Convert kmap() to kmap_local_page()
>>
>> With kmap_local_page(), the mapping is per thread, CPU local and not
>> globally visible.
>>
>> It's not clearer.
>>
>> I mean this " fix kunmap_local path value to take address of the mapped
>> page" be more clearer
>>
>> This is a general statement about the function. You
>> need to explain why it is appropriate to use it here. Unless it is the
>> case that all calls to kmap should be converted to call kmap_local_page.
>>
>> It's required to convert all calls kmap to kmap_local_page. So, I don't
>> what should the commit message be?
>>
>> Is this will be good :
>>
>> "kmap_local_page() was recently developed as a replacement for kmap().
>> The
>> kmap_local_page() creates a mapping which is restricted to local use by a
>> single thread of execution. "
>>
> I think I am missing some thread context here. I'm not sure who said what
> above. So I'm going to start over.
>
> Alaa,
>
> It is important to remember that a good commit message says 2 things.
>
> 1) What is the problem you are trying to solve
> 2) Overview of the solution
>
> First off I understand your frustration. In my opinion fixes and clean ups
> like this are very hard to write good commit messages for because so often the
> code diff seems so self explanatory. However, each code change comes at the
> identification of a problem. And remember that 'problem' does not always mean
> a bug fix.
>
> The deprecation of kmap() may not seem like a problem. I mean why can't we
> just leave kmap() as it is? It works right?
>
> But the problem is that the kmap (highmem) interface has become stale and its
> original purpose was targeted toward large memory systems with 32 bit kernels.
> There are very few systems being run like that any longer.
>
> So how do we clean up the kmap interface to be more useful to the kernel
> community now that 32 bit kernels with highmem are so rare?
>
> The community has identified that a first step of that is to move away from and
> eventually remove the kmap() call. This is due to the call being incorrectly
> used to create long term mappings. Most calls to kmap() are not used
> incorrectly but those call sites needed something in between kmap() and
> kmap_atmoic(). That call is kmap_local_page().
>
> Now that kmap_local_page() exists the kmap() calls can be audited and most (I
> hope most)[1] can be replaced with kmap_local_page().
>
> The change you have below is correct. But it lacks a good commit message. We
> need to cover the 2 points above.
>
> 1) Julia is asking why you needed to do this change. What is the
> problem or reason for this change? (Ira told you to is not a good
> reason. ;-)
>
> PS In fact me telling you to may actually be a very bad reason...
> j/k ;-)
>
> 2) Why is this solution ok as part of the deprecation and removal of
> kmap()?
>
> A final note; the 2 above points don't need a lot of text. Here I used
> 2 simple sentences.
>
> https://lore.kernel.org/lkml/20220124015409.807587-2-ira.weiny@intel.com/
>
> I hope this helps,
> Ira
>
> [1] But not all... some uses of kmap() have been identified as being pretty
> complex.
Thanks a lot for detailed explaining , yes you help me a lot.
Thanks again,
Alaa
>> julia
>>
>>
>> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
>> ---
>> changes in V2:
>> fix kunmap_local path value to take address of the mapped page.
>> ---
>> changes in V3:
>> edit commit message to be clearer
>> ---
>> drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> index 2a5782063f4c..c14fc871dd41 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> @@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct igb_rx_buffer *rx_buffer,
>>
>> frame_size >>= 1;
>>
>> - data = kmap(rx_buffer->page);
>> + data = kmap_local_page(rx_buffer->page);
>>
>> 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.35.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-wired-lan] [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
@ 2022-04-19 13:37 ` Alaa Mohamed
0 siblings, 0 replies; 19+ messages in thread
From: Alaa Mohamed @ 2022-04-19 13:37 UTC (permalink / raw)
To: intel-wired-lan
On ???/??/???? ??:??, Ira Weiny wrote:
> On Sat, Apr 16, 2022 at 03:14:57PM +0200, Alaa Mohamed wrote:
>> On ???/??/???? ??:??, Julia Lawall wrote:
>>
>>
>> On Sat, 16 Apr 2022, Alaa Mohamed wrote:
>>
>>
>> Convert kmap() to kmap_local_page()
>>
>> With kmap_local_page(), the mapping is per thread, CPU local and not
>> globally visible.
>>
>> It's not clearer.
>>
>> I mean this " fix kunmap_local path value to take address of the mapped
>> page" be more clearer
>>
>> This is a general statement about the function. You
>> need to explain why it is appropriate to use it here. Unless it is the
>> case that all calls to kmap should be converted to call kmap_local_page.
>>
>> It's required to convert all calls kmap to kmap_local_page. So, I don't
>> what should the commit message be?
>>
>> Is this will be good :
>>
>> "kmap_local_page() was recently developed as a replacement for kmap().
>> The
>> kmap_local_page() creates a mapping which is restricted to local use by a
>> single thread of execution. "
>>
> I think I am missing some thread context here. I'm not sure who said what
> above. So I'm going to start over.
>
> Alaa,
>
> It is important to remember that a good commit message says 2 things.
>
> 1) What is the problem you are trying to solve
> 2) Overview of the solution
>
> First off I understand your frustration. In my opinion fixes and clean ups
> like this are very hard to write good commit messages for because so often the
> code diff seems so self explanatory. However, each code change comes at the
> identification of a problem. And remember that 'problem' does not always mean
> a bug fix.
>
> The deprecation of kmap() may not seem like a problem. I mean why can't we
> just leave kmap() as it is? It works right?
>
> But the problem is that the kmap (highmem) interface has become stale and its
> original purpose was targeted toward large memory systems with 32 bit kernels.
> There are very few systems being run like that any longer.
>
> So how do we clean up the kmap interface to be more useful to the kernel
> community now that 32 bit kernels with highmem are so rare?
>
> The community has identified that a first step of that is to move away from and
> eventually remove the kmap() call. This is due to the call being incorrectly
> used to create long term mappings. Most calls to kmap() are not used
> incorrectly but those call sites needed something in between kmap() and
> kmap_atmoic(). That call is kmap_local_page().
>
> Now that kmap_local_page() exists the kmap() calls can be audited and most (I
> hope most)[1] can be replaced with kmap_local_page().
>
> The change you have below is correct. But it lacks a good commit message. We
> need to cover the 2 points above.
>
> 1) Julia is asking why you needed to do this change. What is the
> problem or reason for this change? (Ira told you to is not a good
> reason. ;-)
>
> PS In fact me telling you to may actually be a very bad reason...
> j/k ;-)
>
> 2) Why is this solution ok as part of the deprecation and removal of
> kmap()?
>
> A final note; the 2 above points don't need a lot of text. Here I used
> 2 simple sentences.
>
> https://lore.kernel.org/lkml/20220124015409.807587-2-ira.weiny at intel.com/
>
> I hope this helps,
> Ira
>
> [1] But not all... some uses of kmap() have been identified as being pretty
> complex.
Thanks a lot for detailed explaining , yes you help me a lot.
Thanks again,
Alaa
>> julia
>>
>>
>> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
>> ---
>> changes in V2:
>> fix kunmap_local path value to take address of the mapped page.
>> ---
>> changes in V3:
>> edit commit message to be clearer
>> ---
>> drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> index 2a5782063f4c..c14fc871dd41 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> @@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct igb_rx_buffer *rx_buffer,
>>
>> frame_size >>= 1;
>>
>> - data = kmap(rx_buffer->page);
>> + data = kmap_local_page(rx_buffer->page);
>>
>> 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.35.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
2022-04-16 11:31 ` [Intel-wired-lan] " Julia Lawall
@ 2022-04-16 13:18 ` Alaa Mohamed
-1 siblings, 0 replies; 19+ messages in thread
From: Alaa Mohamed @ 2022-04-16 13:18 UTC (permalink / raw)
To: Julia Lawall
Cc: outreachy, jesse.brandeburg, anthony.l.nguyen, davem, kuba,
pabeni, intel-wired-lan, netdev, linux-kernel, ira.weiny
On ١٦/٤/٢٠٢٢ ١٣:٣١, Julia Lawall wrote:
>
> On Sat, 16 Apr 2022, Alaa Mohamed wrote:
>
>> Convert kmap() to kmap_local_page()
>>
>> With kmap_local_page(), the mapping is per thread, CPU local and not
>> globally visible.
> It's not clearer.
I mean this " fix kunmap_local path value to take address of the mapped
page" be more clearer
> This is a general statement about the function. You
> need to explain why it is appropriate to use it here. Unless it is the
> case that all calls to kmap should be converted to call kmap_local_page.
It's required to convert all calls kmap to kmap_local_page. So, I don't
what should the commit message be?
Is this will be good :
"kmap_local_page() was recently developed as a replacement for kmap(). The
kmap_local_page() creates a mapping which is restricted to local use by a
single thread of execution. "
>
> julia
>
>> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
>> ---
>> changes in V2:
>> fix kunmap_local path value to take address of the mapped page.
>> ---
>> changes in V3:
>> edit commit message to be clearer
>> ---
>> drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> index 2a5782063f4c..c14fc871dd41 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> @@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct igb_rx_buffer *rx_buffer,
>>
>> frame_size >>= 1;
>>
>> - data = kmap(rx_buffer->page);
>> + data = kmap_local_page(rx_buffer->page);
>>
>> 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.35.2
>>
>>
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-wired-lan] [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
@ 2022-04-16 13:18 ` Alaa Mohamed
0 siblings, 0 replies; 19+ messages in thread
From: Alaa Mohamed @ 2022-04-16 13:18 UTC (permalink / raw)
To: intel-wired-lan
On ???/??/???? ??:??, Julia Lawall wrote:
>
> On Sat, 16 Apr 2022, Alaa Mohamed wrote:
>
>> Convert kmap() to kmap_local_page()
>>
>> With kmap_local_page(), the mapping is per thread, CPU local and not
>> globally visible.
> It's not clearer.
I mean this " fix kunmap_local path value to take address of the mapped
page" be more clearer
> This is a general statement about the function. You
> need to explain why it is appropriate to use it here. Unless it is the
> case that all calls to kmap should be converted to call kmap_local_page.
It's required to convert all calls kmap to kmap_local_page. So, I don't
what should the commit message be?
Is this will be good :
"kmap_local_page() was recently developed as a replacement for kmap().? The
kmap_local_page() creates a mapping which is restricted to local use by a
single thread of execution. "
>
> julia
>
>> Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
>> ---
>> changes in V2:
>> fix kunmap_local path value to take address of the mapped page.
>> ---
>> changes in V3:
>> edit commit message to be clearer
>> ---
>> drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> index 2a5782063f4c..c14fc871dd41 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> @@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct igb_rx_buffer *rx_buffer,
>>
>> frame_size >>= 1;
>>
>> - data = kmap(rx_buffer->page);
>> + data = kmap_local_page(rx_buffer->page);
>>
>> 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.35.2
>>
>>
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
2022-04-16 13:18 ` [Intel-wired-lan] " Alaa Mohamed
@ 2022-04-16 14:09 ` Julia Lawall
-1 siblings, 0 replies; 19+ messages in thread
From: Julia Lawall @ 2022-04-16 14:09 UTC (permalink / raw)
To: Alaa Mohamed
Cc: Julia Lawall, outreachy, jesse.brandeburg, anthony.l.nguyen,
davem, kuba, pabeni, intel-wired-lan, netdev, linux-kernel,
ira.weiny
[-- Attachment #1: Type: text/plain, Size: 2386 bytes --]
On Sat, 16 Apr 2022, Alaa Mohamed wrote:
>
> On ١٦/٤/٢٠٢٢ ١٣:٣١, Julia Lawall wrote:
> >
> > On Sat, 16 Apr 2022, Alaa Mohamed wrote:
> >
> > > Convert kmap() to kmap_local_page()
> > >
> > > With kmap_local_page(), the mapping is per thread, CPU local and not
> > > globally visible.
> > It's not clearer.
> I mean this " fix kunmap_local path value to take address of the mapped page"
> be more clearer
> > This is a general statement about the function. You
> > need to explain why it is appropriate to use it here. Unless it is the
> > case that all calls to kmap should be converted to call kmap_local_page.
> It's required to convert all calls kmap to kmap_local_page. So, I don't what
> should the commit message be?
If all calls should be changed then you can also say that.
I thought that a previous commit on the outreachy list made some arguments
about how the affacted value was just allocated and thus could not yet be
shared.
julia
>
> Is this will be good :
>
> "kmap_local_page() was recently developed as a replacement for kmap(). The
> kmap_local_page() creates a mapping which is restricted to local use by a
> single thread of execution. "
> >
> > julia
> >
> > > Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
> > > ---
> > > changes in V2:
> > > fix kunmap_local path value to take address of the mapped page.
> > > ---
> > > changes in V3:
> > > edit commit message to be clearer
> > > ---
> > > drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > index 2a5782063f4c..c14fc871dd41 100644
> > > --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > @@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct
> > > igb_rx_buffer *rx_buffer,
> > >
> > > frame_size >>= 1;
> > >
> > > - data = kmap(rx_buffer->page);
> > > + data = kmap_local_page(rx_buffer->page);
> > >
> > > 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.35.2
> > >
> > >
> > >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-wired-lan] [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
@ 2022-04-16 14:09 ` Julia Lawall
0 siblings, 0 replies; 19+ messages in thread
From: Julia Lawall @ 2022-04-16 14:09 UTC (permalink / raw)
To: intel-wired-lan
On Sat, 16 Apr 2022, Alaa Mohamed wrote:
>
> On ??/?/???? ??:??, Julia Lawall wrote:
> >
> > On Sat, 16 Apr 2022, Alaa Mohamed wrote:
> >
> > > Convert kmap() to kmap_local_page()
> > >
> > > With kmap_local_page(), the mapping is per thread, CPU local and not
> > > globally visible.
> > It's not clearer.
> I mean this " fix kunmap_local path value to take address of the mapped page"
> be more clearer
> > This is a general statement about the function. You
> > need to explain why it is appropriate to use it here. Unless it is the
> > case that all calls to kmap should be converted to call kmap_local_page.
> It's required to convert all calls kmap to kmap_local_page. So, I don't what
> should the commit message be?
If all calls should be changed then you can also say that.
I thought that a previous commit on the outreachy list made some arguments
about how the affacted value was just allocated and thus could not yet be
shared.
julia
>
> Is this will be good :
>
> "kmap_local_page() was recently developed as a replacement for kmap().? The
> kmap_local_page() creates a mapping which is restricted to local use by a
> single thread of execution. "
> >
> > julia
> >
> > > Signed-off-by: Alaa Mohamed <eng.alaamohamedsoliman.am@gmail.com>
> > > ---
> > > changes in V2:
> > > fix kunmap_local path value to take address of the mapped page.
> > > ---
> > > changes in V3:
> > > edit commit message to be clearer
> > > ---
> > > drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > index 2a5782063f4c..c14fc871dd41 100644
> > > --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > @@ -1798,14 +1798,14 @@ static int igb_check_lbtest_frame(struct
> > > igb_rx_buffer *rx_buffer,
> > >
> > > frame_size >>= 1;
> > >
> > > - data = kmap(rx_buffer->page);
> > > + data = kmap_local_page(rx_buffer->page);
> > >
> > > 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.35.2
> > >
> > >
> > >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
2022-04-16 14:09 ` [Intel-wired-lan] " Julia Lawall
@ 2022-04-16 15:52 ` Fabio M. De Francesco
-1 siblings, 0 replies; 19+ messages in thread
From: Fabio M. De Francesco @ 2022-04-16 15:52 UTC (permalink / raw)
To: Alaa Mohamed, Julia Lawall
Cc: Julia Lawall, outreachy, jesse.brandeburg, anthony.l.nguyen,
davem, kuba, pabeni, intel-wired-lan, netdev, linux-kernel,
ira.weiny
On sabato 16 aprile 2022 16:09:58 CEST Julia Lawall wrote:
>
> On Sat, 16 Apr 2022, Alaa Mohamed wrote:
>
> >
> > On ١٦/٤/٢٠٢٢ ١٣:٣١, Julia Lawall wrote:
> > >
> > > On Sat, 16 Apr 2022, Alaa Mohamed wrote:
> > >
> > > > Convert kmap() to kmap_local_page()
> > > >
> > > > With kmap_local_page(), the mapping is per thread, CPU local and
not
> > > > globally visible.
> > > It's not clearer.
> > I mean this " fix kunmap_local path value to take address of the mapped
page"
> > be more clearer
> > > This is a general statement about the function. You
> > > need to explain why it is appropriate to use it here. Unless it is
the
> > > case that all calls to kmap should be converted to call
kmap_local_page.
> > It's required to convert all calls kmap to kmap_local_page. So, I don't
what
> > should the commit message be?
>
> If all calls should be changed then you can also say that.
If all calls should be changed with no regards to the surrounding contexts
and special situations, we can just make an automated s/kmap()/
kmap_local_page()/ or something else similar :)
Thanks,
Fabio M. De Francesco
>
> I thought that a previous commit on the outreachy list made some
arguments
> about how the affacted value was just allocated and thus could not yet be
> shared.
>
> julia
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-wired-lan] [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
@ 2022-04-16 15:52 ` Fabio M. De Francesco
0 siblings, 0 replies; 19+ messages in thread
From: Fabio M. De Francesco @ 2022-04-16 15:52 UTC (permalink / raw)
To: intel-wired-lan
On sabato 16 aprile 2022 16:09:58 CEST Julia Lawall wrote:
>
> On Sat, 16 Apr 2022, Alaa Mohamed wrote:
>
> >
> > On ????/??/??? ???? ????:????, Julia Lawall wrote:
> > >
> > > On Sat, 16 Apr 2022, Alaa Mohamed wrote:
> > >
> > > > Convert kmap() to kmap_local_page()
> > > >
> > > > With kmap_local_page(), the mapping is per thread, CPU local and
not
> > > > globally visible.
> > > It's not clearer.
> > I mean this " fix kunmap_local path value to take address of the mapped
page"
> > be more clearer
> > > This is a general statement about the function. You
> > > need to explain why it is appropriate to use it here. Unless it is
the
> > > case that all calls to kmap should be converted to call
kmap_local_page.
> > It's required to convert all calls kmap to kmap_local_page. So, I don't
what
> > should the commit message be?
>
> If all calls should be changed then you can also say that.
If all calls should be changed with no regards to the surrounding contexts
and special situations, we can just make an automated s/kmap()/
kmap_local_page()/ or something else similar :)
Thanks,
Fabio M. De Francesco
>
> I thought that a previous commit on the outreachy list made some
arguments
> about how the affacted value was just allocated and thus could not yet be
> shared.
>
> julia
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-wired-lan] [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
2022-04-16 15:52 ` [Intel-wired-lan] " Fabio M. De Francesco
(?)
@ 2022-04-16 16:28 ` Fabio M. De Francesco
-1 siblings, 0 replies; 19+ messages in thread
From: Fabio M. De Francesco @ 2022-04-16 16:28 UTC (permalink / raw)
To: intel-wired-lan
On sabato 16 aprile 2022 17:52:20 CEST Fabio M. De Francesco wrote:
> On sabato 16 aprile 2022 16:09:58 CEST Julia Lawall wrote:
> >
> > On Sat, 16 Apr 2022, Alaa Mohamed wrote:
> >
> > >
> > > On ????/??/??? ???? ????:????, Julia Lawall wrote:
> > > >
> > > > On Sat, 16 Apr 2022, Alaa Mohamed wrote:
> > > >
> > > > > Convert kmap() to kmap_local_page()
> > > > >
> > > > > With kmap_local_page(), the mapping is per thread, CPU local and
> not
> > > > > globally visible.
> > > > It's not clearer.
> > > I mean this " fix kunmap_local path value to take address of the
mapped
> page"
> > > be more clearer
> > > > This is a general statement about the function. You
> > > > need to explain why it is appropriate to use it here. Unless it is
> the
> > > > case that all calls to kmap should be converted to call
> kmap_local_page.
> > > It's required to convert all calls kmap to kmap_local_page. So, I
don't
> what
> > > should the commit message be?
> >
> > If all calls should be changed then you can also say that.
>
> If all calls should be changed with no regards to the surrounding
contexts
> and special situations, we can just make an automated s/kmap()/
> kmap_local_page()/ or something else similar :)
Obviously, I was just kidding because we cannot massively and blindly
change all kmap() calls to kmap_local_page().
IMO, here the changes in code are good but Julia's objections are
legitimate too.
Thanks,
Fabio
> >
> > I thought that a previous commit on the outreachy list made some
> arguments
> > about how the affacted value was just allocated and thus could not yet
be
> > shared.
> >
> > julia
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20220416/51c29e10/attachment-0001.html>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Intel-wired-lan] [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
2022-04-16 15:52 ` [Intel-wired-lan] " Fabio M. De Francesco
(?)
(?)
@ 2022-04-16 21:43 ` Fabio M. De Francesco
-1 siblings, 0 replies; 19+ messages in thread
From: Fabio M. De Francesco @ 2022-04-16 21:43 UTC (permalink / raw)
To: intel-wired-lan
On sabato 16 aprile 2022 17:52:20 CEST Fabio M. De Francesco wrote:
> On sabato 16 aprile 2022 16:09:58 CEST Julia Lawall wrote:
> >
> > If all calls should be changed then you can also say that.
>
> If all calls should be changed with no regards to the surrounding
contexts
> and special situations, we can just make an automated s/kmap()/
> kmap_local_page()/ or something else similar :)
Hi Julia,
Of course I was just kidding when talking of massively automated
substitutions. They are not feasible and we cannot blindly replace all
kmap() calls with kmap_local_page().
Although these code changes look good, your objections are appropriate and
legitimate.
Not all kmap() calls can be changed to kmap_local_page() and, if someone
wants to make such replacements, they should also "prove" somehow that they
are doing the right changes in that specific context.
For example, the following is one of those cases where such a replacement
is not allowed and a different solution has yet to be found:
https://lore.kernel.org/lkml/2a7030f5-d55f-94c7-90ba-5a57235159f6 at amd.com/
Furthermore, if people cannot "prove" that this change is feasible, their
patches will probably be ignored / rejected just because many maintainers
still don't know if those changes are correct and safe.
Whoever wants to do these changes should understand the specific context in
which they are working.
For example, there have also been cases where alloc_page() + kmap() was
simply replaced by kmalloc(). Sure!
If you are interested to see how and why, please take a look at the commit
633b0616cfe0 ("x86/sgx: Remove unnecessary kmap() from
sgx_ioc_enclave_init()") from Ira Weiny.
Regards,
Fabio
> >
> > I thought that a previous commit on the outreachy list made some
> arguments
> > about how the affacted value was just allocated and thus could not yet
be
> > shared.
> >
> > julia
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20220416/daee33e4/attachment-0001.html>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
2022-04-16 15:52 ` [Intel-wired-lan] " Fabio M. De Francesco
` (2 preceding siblings ...)
(?)
@ 2022-04-16 22:21 ` Fabio M. De Francesco
2022-04-17 7:07 ` Julia Lawall
-1 siblings, 1 reply; 19+ messages in thread
From: Fabio M. De Francesco @ 2022-04-16 22:21 UTC (permalink / raw)
To: outreachy
On sabato 16 aprile 2022 17:52:20 CEST Fabio M. De Francesco wrote:
> If all calls should be changed with no regards to the surrounding
contexts
> and special situations, we can just make an automated s/kmap()/
> kmap_local_page()/ or something else similar
(I'm afraid that somehow I messed up with the HTML filters in KMail
therefore I have to send this email again to the Outreachy list because
this message has already been rejected twice - I hope this time it works).
Hi Julia,
Of course I was just kidding when talking of massively automated
substitutions. They are not feasible and we cannot blindly replace all
kmap() calls with kmap_local_page().
Although these code changes look good, your objections are appropriate and
legitimate.
Not all kmap() calls can be changed to kmap_local_page() and, if someone
wants to make such replacements, they should also "prove" somehow that they
are doing the right changes in that specific context.
For example, the following is one of those cases where such a replacement
is not allowed and a different solution has yet to be found:
https://lore.kernel.org/lkml/2a7030f5-d55f-94c7-90ba-5a57235159f6@amd.com/
Furthermore, if people cannot "prove" that this change is feasible, their
patches will probably be ignored / rejected just because many maintainers
still don't know if those changes are correct and safe.
Whoever wants to do these changes should understand the specific context in
which they are working.
For example, there have also been cases where alloc_page() + kmap() was
simply replaced by kmalloc(). Sure!
If you are interested to see how and why, please take a look at the commit
633b0616cfe0 ("x86/sgx: Remove unnecessary kmap() from
sgx_ioc_enclave_init()") from Ira Weiny.
Regards,
Fabio
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page()
2022-04-16 22:21 ` Fabio M. De Francesco
@ 2022-04-17 7:07 ` Julia Lawall
0 siblings, 0 replies; 19+ messages in thread
From: Julia Lawall @ 2022-04-17 7:07 UTC (permalink / raw)
To: Fabio M. De Francesco; +Cc: outreachy
On Sun, 17 Apr 2022, Fabio M. De Francesco wrote:
> On sabato 16 aprile 2022 17:52:20 CEST Fabio M. De Francesco wrote:
> > If all calls should be changed with no regards to the surrounding
> contexts
> > and special situations, we can just make an automated s/kmap()/
> > kmap_local_page()/ or something else similar
>
> (I'm afraid that somehow I messed up with the HTML filters in KMail
> therefore I have to send this email again to the Outreachy list because
> this message has already been rejected twice - I hope this time it works).
>
> Hi Julia,
>
> Of course I was just kidding when talking of massively automated
>
> substitutions. They are not feasible and we cannot blindly replace all
> kmap() calls with kmap_local_page().
>
> Although these code changes look good, your objections are appropriate and
> legitimate.
>
> Not all kmap() calls can be changed to kmap_local_page() and, if someone
> wants to make such replacements, they should also "prove" somehow that they
> are doing the right changes in that specific context.
>
> For example, the following is one of those cases where such a replacement
> is not allowed and a different solution has yet to be found:
>
> https://lore.kernel.org/lkml/2a7030f5-d55f-94c7-90ba-5a57235159f6@amd.com/
>
> Furthermore, if people cannot "prove" that this change is feasible, their
> patches will probably be ignored / rejected just because many maintainers
> still don't know if those changes are correct and safe.
I think that all patches in which the log message does not explain why the
change is correct should be rejected. Not because the maintainer can't do
the proof himself, but because the reasoning should be explicit in the git
history.
julia
>
> Whoever wants to do these changes should understand the specific context in
> which they are working.
>
> For example, there have also been cases where alloc_page() + kmap() was
> simply replaced by kmalloc(). Sure!
>
> If you are interested to see how and why, please take a look at the commit
> 633b0616cfe0 ("x86/sgx: Remove unnecessary kmap() from
> sgx_ioc_enclave_init()") from Ira Weiny.
>
> Regards,
>
> Fabio
>
>
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-04-19 13:37 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-16 11:14 [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to kmap_local_page() Alaa Mohamed
2022-04-16 11:14 ` [Intel-wired-lan] " Alaa Mohamed
2022-04-16 11:31 ` Julia Lawall
2022-04-16 11:31 ` [Intel-wired-lan] " Julia Lawall
2022-04-16 13:14 ` Alaa Mohamed
2022-04-18 22:19 ` Ira Weiny
2022-04-18 22:19 ` [Intel-wired-lan] " Ira Weiny
2022-04-19 13:37 ` Alaa Mohamed
2022-04-19 13:37 ` [Intel-wired-lan] " Alaa Mohamed
2022-04-16 13:18 ` Alaa Mohamed
2022-04-16 13:18 ` [Intel-wired-lan] " Alaa Mohamed
2022-04-16 14:09 ` Julia Lawall
2022-04-16 14:09 ` [Intel-wired-lan] " Julia Lawall
2022-04-16 15:52 ` Fabio M. De Francesco
2022-04-16 15:52 ` [Intel-wired-lan] " Fabio M. De Francesco
2022-04-16 16:28 ` Fabio M. De Francesco
2022-04-16 21:43 ` Fabio M. De Francesco
2022-04-16 22:21 ` Fabio M. De Francesco
2022-04-17 7:07 ` Julia Lawall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.