| 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 |
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.
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.
> 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>
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.
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. >
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.
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 --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) {
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(-)