Re: [Intel-wired-lan] [RESEND PATCH net-next 2/2] e1000e: Remove unnecessary use of kmap_atomic()

[Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>]

On 9/29/2022 3:40 PM, Ira Weiny wrote:
> On Mon, Sep 19, 2022 at 11:09:49AM -0700, Venkataramanan, Anirudh wrote:
>> alloc_rx_buf() allocates ps_page->page and buffer_info->page using either
>> GFP_ATOMIC or GFP_KERNEL. Memory allocated with GFP_KERNEL/GFP_ATOMIC can't
>> come from highmem and so there's no need to kmap() them. Just use
>> page_address().
>>
>> I don't have access to a 32-bit system so did some limited testing on qemu
>> (qemu-system-i386 -m 4096 -smp 4 -device e1000e) with a 32-bit Debian 11.04
>> image.
>>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
>> Suggested-by: Ira Weiny <ira.weiny@intel.com>
>> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>> Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
>> ---
>>   drivers/net/ethernet/intel/e1000e/netdev.c | 17 ++++-------------
>>   1 file changed, 4 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>> index 321f2a9..05a59e5 100644
>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>> @@ -1393,21 +1393,14 @@ static bool e1000_clean_rx_irq_ps(struct e1000_ring *rx_ring, int *work_done,
>>   			 */
> 
> The comment just above here says this:
> 
> 1389                         /* page alloc/put takes too long and effects small
> 1390                          * packet throughput, so unsplit small packets and
> 1391                          * save the alloc/put only valid in softirq (napi)
> 1392                          * context to call kmap_*
> 1393                          */

I did see this when I was making the code change but it evaporated from 
my working memory. Thanks for catching it.

> 
> I'm unable to grok what that means exactly but I feel like the kmap part is no
> longer appropriate?
> 
> Maybe just delete ... 'to call kmap_*'?

Okay, so did some git archeology and found that the original comment 
just said this:

/* page alloc/put takes too long and effects small packet
  * throughput, so unsplit small packets and save the alloc/put*/

...  which is what I think we need to revert to.

Tony, do you want me to send a v2? Or is making an inline edit to the 
patch you've already applied to dev-tree less work for you?

I am okay either way. Just tell me which one makes life easier for you.

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