[Intel-wired-lan] [PATCH net-next] ixgbe: Remove unnecessary use of kmap()

[Intel-wired-lan] [PATCH net-next] ixgbe: Remove unnecessary use of kmap()

[Anirudh Venkataramanan]

Pages for Rx are allocated in ixgbe_alloc_mapped_page() using
dev_alloc_pages(), which does the allocation using flags GFP_ATOMIC |
__GFP_NOWARN. Pages allocated thus can't come from highmem, so remove
calls to kmap() and kunmap().

While here, also remove the local variable "match", and just return
true/false.

I wasn't able to get a hold of a system with an ixgbe NIC to test this
change. This is a pretty trivial change and I don't expect anything to
break, but a "Tested-by" from someone who has the hardware would be nice.

Cc: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Fabio M. De Francesco <fmdefrancesco@gmail.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/ixgbe/ixgbe_ethtool.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 04f453e..90c0e0b 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1960,20 +1960,17 @@ static bool ixgbe_check_lbtest_frame(struct ixgbe_rx_buffer *rx_buffer,
 				     unsigned int frame_size)
 {
 	unsigned char *data;
-	bool match = true;
 
 	frame_size >>= 1;
 
-	data = kmap(rx_buffer->page) + rx_buffer->page_offset;
+	data = page_address(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);
+		return false;
 
-	return match;
+	return true;
 }
 
 static u16 ixgbe_clean_test_rings(struct ixgbe_ring *rx_ring,
-- 
2.37.2

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

Re: [Intel-wired-lan] [PATCH net-next] ixgbe: Remove unnecessary use of kmap()

[Alexander Duyck]

On Wed, Sep 28, 2022 at 12:26 PM Anirudh Venkataramanan
<anirudh.venkataramanan@intel.com> wrote:
>
> Pages for Rx are allocated in ixgbe_alloc_mapped_page() using
> dev_alloc_pages(), which does the allocation using flags GFP_ATOMIC |
> __GFP_NOWARN. Pages allocated thus can't come from highmem, so remove
> calls to kmap() and kunmap().
>
> While here, also remove the local variable "match", and just return
> true/false.
>
> I wasn't able to get a hold of a system with an ixgbe NIC to test this
> change. This is a pretty trivial change and I don't expect anything to
> break, but a "Tested-by" from someone who has the hardware would be nice.
>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.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/ixgbe/ixgbe_ethtool.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> index 04f453e..90c0e0b 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> @@ -1960,20 +1960,17 @@ static bool ixgbe_check_lbtest_frame(struct ixgbe_rx_buffer *rx_buffer,
>                                      unsigned int frame_size)
>  {
>         unsigned char *data;
> -       bool match = true;
>
>         frame_size >>= 1;
>
> -       data = kmap(rx_buffer->page) + rx_buffer->page_offset;
> +       data = page_address(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);
> +               return false;
>
> -       return match;
> +       return true;
>  }
>
>  static u16 ixgbe_clean_test_rings(struct ixgbe_ring *rx_ring,

Rather than bothering with the return true/false why not just return
the compare value? So it would be:
        return data[3] == 0xFF && data[frame_size + 10] == 0xBE &&
data[frame_size + 12] == 0xAF;
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next] ixgbe: Remove unnecessary use of kmap()

[Anirudh Venkataramanan]

On 9/28/2022 1:40 PM, Alexander Duyck wrote:
> On Wed, Sep 28, 2022 at 12:26 PM Anirudh Venkataramanan
> <anirudh.venkataramanan@intel.com> wrote:
>>
>> Pages for Rx are allocated in ixgbe_alloc_mapped_page() using
>> dev_alloc_pages(), which does the allocation using flags GFP_ATOMIC |
>> __GFP_NOWARN. Pages allocated thus can't come from highmem, so remove
>> calls to kmap() and kunmap().
>>
>> While here, also remove the local variable "match", and just return
>> true/false.
>>
>> I wasn't able to get a hold of a system with an ixgbe NIC to test this
>> change. This is a pretty trivial change and I don't expect anything to
>> break, but a "Tested-by" from someone who has the hardware would be nice.
>>
>> Cc: Alexander Duyck <alexander.duyck@gmail.com>
>> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
>> Cc: Ira Weiny <ira.weiny@intel.com>
>> Cc: Fabio M. De Francesco <fmdefrancesco@gmail.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/ixgbe/ixgbe_ethtool.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> index 04f453e..90c0e0b 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> @@ -1960,20 +1960,17 @@ static bool ixgbe_check_lbtest_frame(struct ixgbe_rx_buffer *rx_buffer,
>>                                       unsigned int frame_size)
>>   {
>>          unsigned char *data;
>> -       bool match = true;
>>
>>          frame_size >>= 1;
>>
>> -       data = kmap(rx_buffer->page) + rx_buffer->page_offset;
>> +       data = page_address(rx_buffer->page) + rx_buffer->page_offset;

Okay, Fabio has already made this exact change and it's currently in 
Tony's kernel tree (next-queue:dev-queue). I missed it because I was 
barking up the wrong (kernel) tree. My apologies for the thrash.

>>
>>          if (data[3] != 0xFF ||
>>              data[frame_size + 10] != 0xBE ||
>>              data[frame_size + 12] != 0xAF)
>> -               match = false;
>> -
>> -       kunmap(rx_buffer->page);
>> +               return false;
>>
>> -       return match;
>> +       return true;
>>   }
>>
>>   static u16 ixgbe_clean_test_rings(struct ixgbe_ring *rx_ring,
> 
> Rather than bothering with the return true/false why not just return
> the compare value? So it would be:
>          return data[3] == 0xFF && data[frame_size + 10] == 0xBE &&
> data[frame_size + 12] == 0xAF;

Sure, I can do a standalone patch for this.

Ani


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