diff mbox series

[v1,1/1] igc: Complete to commit Add support for TSO

Message ID 20200205123115.44103-1-sasha.neftin@intel.com
State Superseded
Delegated to: Jeff Kirsher
Headers show
Series [v1,1/1] igc: Complete to commit Add support for TSO | expand

Commit Message

Sasha Neftin Feb. 5, 2020, 12:31 p.m. UTC
commit f38b782dccab ("igc: Add support for TSO")
Add option to setting transmit command (TUCMD) of the context
descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.

Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
 drivers/net/ethernet/intel/igc/igc_main.c    | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Jesse Brandeburg Feb. 5, 2020, 7:03 p.m. UTC | #1
On Wed, 5 Feb 2020 14:31:15 +0200 Sasha wrote:
> commit f38b782dccab ("igc: Add support for TSO")

is that a Fixes tag?

> Add option to setting transmit command (TUCMD) of the context
> descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.

You said what you did but not why.
Sasha Neftin Feb. 6, 2020, 7:48 a.m. UTC | #2
On 2/5/2020 21:03, Jesse Brandeburg wrote:
> On Wed, 5 Feb 2020 14:31:15 +0200 Sasha wrote:
>> commit f38b782dccab ("igc: Add support for TSO")
> 
> is that a Fixes tag?
> 
no, this is previous commit for support TSO
>> Add option to setting transmit command (TUCMD) of the context
>> descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.
> 
> You said what you did but not why.
> 
I would align the igc driver with the same approach as igb and ixgbe 
drivers. I referred to their code.
Brown, Aaron F Feb. 12, 2020, 10:39 p.m. UTC | #3
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Sasha Neftin
> Sent: Wednesday, February 5, 2020 4:31 AM
> To: intel-wired-lan@lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add
> support for TSO
> 
> commit f38b782dccab ("igc: Add support for TSO")
> Add option to setting transmit command (TUCMD) of the context
> descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.
> 
> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
>  drivers/net/ethernet/intel/igc/igc_main.c    | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
Aside from possibly including the responses to Jesse's questions on this...
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Alexander Duyck Feb. 12, 2020, 10:47 p.m. UTC | #4
On Wed, Feb 12, 2020 at 2:39 PM Brown, Aaron F <aaron.f.brown@intel.com> wrote:
>
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> > Sasha Neftin
> > Sent: Wednesday, February 5, 2020 4:31 AM
> > To: intel-wired-lan@lists.osuosl.org
> > Subject: [Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add
> > support for TSO
> >
> > commit f38b782dccab ("igc: Add support for TSO")
> > Add option to setting transmit command (TUCMD) of the context
> > descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.
> >
> > Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
> >  drivers/net/ethernet/intel/igc/igc_main.c    | 3 ++-
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> Aside from possibly including the responses to Jesse's questions on this...
> Tested-by: Aaron Brown <aaron.f.brown@intel.com>

I'm not sure the patch makes any sense. Does the driver support UDP
GSO? I don't see the feature flag (NETIF_F_GSO_UDP_L4) anywhere that
enables it.

If it doesn't enable it then it doesn't make much sense to update the
code to add this one bit until it does since you can't actually test
it as the stack will not ask you to segment UDP frames.
Sasha Neftin Feb. 13, 2020, 8:02 a.m. UTC | #5
On 2/13/2020 00:47, Alexander Duyck wrote:
> On Wed, Feb 12, 2020 at 2:39 PM Brown, Aaron F <aaron.f.brown@intel.com> wrote:
>>
>>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
>>> Sasha Neftin
>>> Sent: Wednesday, February 5, 2020 4:31 AM
>>> To: intel-wired-lan@lists.osuosl.org
>>> Subject: [Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add
>>> support for TSO
>>>
>>> commit f38b782dccab ("igc: Add support for TSO")
>>> Add option to setting transmit command (TUCMD) of the context
>>> descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.
>>>
>>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>>> ---
>>>   drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
>>>   drivers/net/ethernet/intel/igc/igc_main.c    | 3 ++-
>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>> Aside from possibly including the responses to Jesse's questions on this...
>> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> 
> I'm not sure the patch makes any sense. Does the driver support UDP
> GSO? I don't see the feature flag (NETIF_F_GSO_UDP_L4) anywhere that
> enables it.
> 
Yes, it is supported. The MAC is similar to igb from segmentation 
perspectives. I missed add it. Please, let me finish some testing and I 
will add NETIF_F_GSO_UDP_L4 flag to _features_check and _probe methods. 
I will process another patch with complements.
> If it doesn't enable it then it doesn't make much sense to update the
> code to add this one bit until it does since you can't actually test
> it as the stack will not ask you to segment UDP frames.
>
Alexander Duyck Feb. 13, 2020, 4:59 p.m. UTC | #6
On Thu, Feb 13, 2020 at 12:02 AM Neftin, Sasha <sasha.neftin@intel.com> wrote:
>
> On 2/13/2020 00:47, Alexander Duyck wrote:
> > On Wed, Feb 12, 2020 at 2:39 PM Brown, Aaron F <aaron.f.brown@intel.com> wrote:
> >>
> >>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> >>> Sasha Neftin
> >>> Sent: Wednesday, February 5, 2020 4:31 AM
> >>> To: intel-wired-lan@lists.osuosl.org
> >>> Subject: [Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add
> >>> support for TSO
> >>>
> >>> commit f38b782dccab ("igc: Add support for TSO")
> >>> Add option to setting transmit command (TUCMD) of the context
> >>> descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.
> >>>
> >>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
> >>> ---
> >>>   drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
> >>>   drivers/net/ethernet/intel/igc/igc_main.c    | 3 ++-
> >>>   2 files changed, 3 insertions(+), 1 deletion(-)
> >>>
> >> Aside from possibly including the responses to Jesse's questions on this...
> >> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
> >
> > I'm not sure the patch makes any sense. Does the driver support UDP
> > GSO? I don't see the feature flag (NETIF_F_GSO_UDP_L4) anywhere that
> > enables it.
> >
> Yes, it is supported. The MAC is similar to igb from segmentation
> perspectives. I missed add it. Please, let me finish some testing and I
> will add NETIF_F_GSO_UDP_L4 flag to _features_check and _probe methods.

Thats fine. I just wouldn't submit this patch as it is. It should be
adding the NETIF_F_GSO_UDP_L4 flag so you can actually test the code
and verify the hardware is handling it correctly.
Sasha Neftin Feb. 16, 2020, 8:40 a.m. UTC | #7
On 2/13/2020 18:59, Alexander Duyck wrote:
> On Thu, Feb 13, 2020 at 12:02 AM Neftin, Sasha <sasha.neftin@intel.com> wrote:
>>
>> On 2/13/2020 00:47, Alexander Duyck wrote:
>>> On Wed, Feb 12, 2020 at 2:39 PM Brown, Aaron F <aaron.f.brown@intel.com> wrote:
>>>>
>>>>> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
>>>>> Sasha Neftin
>>>>> Sent: Wednesday, February 5, 2020 4:31 AM
>>>>> To: intel-wired-lan@lists.osuosl.org
>>>>> Subject: [Intel-wired-lan] [PATCH v1 1/1] igc: Complete to commit Add
>>>>> support for TSO
>>>>>
>>>>> commit f38b782dccab ("igc: Add support for TSO")
>>>>> Add option to setting transmit command (TUCMD) of the context
>>>>> descriptor based on skb_shinfo gso_type and SKB_GSO_UDP_L4 flag.
>>>>>
>>>>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>>>>> ---
>>>>>    drivers/net/ethernet/intel/igc/igc_defines.h | 1 +
>>>>>    drivers/net/ethernet/intel/igc/igc_main.c    | 3 ++-
>>>>>    2 files changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>> Aside from possibly including the responses to Jesse's questions on this...
>>>> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
>>>
>>> I'm not sure the patch makes any sense. Does the driver support UDP
>>> GSO? I don't see the feature flag (NETIF_F_GSO_UDP_L4) anywhere that
>>> enables it.
>>>
>> Yes, it is supported. The MAC is similar to igb from segmentation
>> perspectives. I missed add it. Please, let me finish some testing and I
>> will add NETIF_F_GSO_UDP_L4 flag to _features_check and _probe methods.
> 
> Thats fine. I just wouldn't submit this patch as it is. It should be
> adding the NETIF_F_GSO_UDP_L4 flag so you can actually test the code
> and verify the hardware is handling it correctly.
> 
Thanks Alex. I will address fix in v2.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 6468253ec4f5..b70651c42098 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -529,6 +529,7 @@ 
 #define IGC_VLAPQF_QUEUE_MASK	0x03
 
 #define IGC_ADVTXD_MACLEN_SHIFT		9  /* Adv ctxt desc mac len shift */
+#define IGC_ADVTXD_TUCMD_L4T_UDP	0x00000000  /* L4 Packet TYPE of UDP */
 #define IGC_ADVTXD_TUCMD_IPV4		0x00000400  /* IP Packet Type:1=IPv4 */
 #define IGC_ADVTXD_TUCMD_L4T_TCP	0x00000800  /* L4 Packet Type of TCP */
 #define IGC_ADVTXD_TUCMD_L4T_SCTP	0x00001000 /* L4 packet TYPE of SCTP */
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index dec6f5a4f03f..660b14abce1d 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1231,7 +1231,8 @@  static int igc_tso(struct igc_ring *tx_ring,
 	l4.hdr = skb_checksum_start(skb);
 
 	/* ADV DTYP TUCMD MKRLOC/ISCSIHEDLEN */
-	type_tucmd = IGC_ADVTXD_TUCMD_L4T_TCP;
+	type_tucmd = (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) ?
+		      IGC_ADVTXD_TUCMD_L4T_UDP : IGC_ADVTXD_TUCMD_L4T_TCP;
 
 	/* initialize outer IP header fields */
 	if (ip.v4->version == 4) {