Date:   Tue, 19 Apr 2022 15:37:47 +0200
From:   Alaa Mohamed <eng.alaamohamedsoliman.am@...il.com>
To:     Ira Weiny <ira.weiny@...el.com>
Cc:     Julia Lawall <julia.lawall@...ia.fr>, outreachy@...ts.linux.dev,
        jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com,
        davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] intel: igb: igb_ethtool.c: Convert kmap() to
 kmap_local_page()


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

Powered by blists - more mailing lists