netdev - Re: [PATCH v2 1/3] igb: Add UDP segmentation offload support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <0ff6df40-d9cc-0b4c-6f1f-df4b39c3f3e3@akamai.com>
Date:   Fri, 11 Oct 2019 09:18:32 -0700
From:   Josh Hunt <johunt@...mai.com>
To:     Alexander Duyck <alexander.h.duyck@...ux.intel.com>,
        netdev@...r.kernel.org, intel-wired-lan@...ts.osuosl.org,
        jeffrey.t.kirsher@...el.com
Cc:     willemb@...gle.com, sridhar.samudrala@...el.com,
        aaron.f.brown@...el.com
Subject: Re: [PATCH v2 1/3] igb: Add UDP segmentation offload support



On 10/11/19 8:29 AM, Alexander Duyck wrote:
> On Thu, 2019-10-10 at 20:25 -0400, Josh Hunt wrote:
>> Based on a series from Alexander Duyck this change adds UDP segmentation
>> offload support to the igb driver.
>>
>> CC: Alexander Duyck <alexander.h.duyck@...el.com>
>> CC: Willem de Bruijn <willemb@...gle.com>
>> Signed-off-by: Josh Hunt <johunt@...mai.com>
>> ---
>>   drivers/net/ethernet/intel/igb/e1000_82575.h |  1 +
>>   drivers/net/ethernet/intel/igb/igb_main.c    | 23 +++++++++++++++++------
>>   2 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.h b/drivers/net/ethernet/intel/igb/e1000_82575.h
>> index 6ad775b1a4c5..63ec253ac788 100644
>> --- a/drivers/net/ethernet/intel/igb/e1000_82575.h
>> +++ b/drivers/net/ethernet/intel/igb/e1000_82575.h
>> @@ -127,6 +127,7 @@ struct e1000_adv_tx_context_desc {
>>   };
>>   
>>   #define E1000_ADVTXD_MACLEN_SHIFT    9  /* Adv ctxt desc mac len shift */
>> +#define E1000_ADVTXD_TUCMD_L4T_UDP 0x00000000  /* L4 Packet TYPE of UDP */
>>   #define E1000_ADVTXD_TUCMD_IPV4    0x00000400  /* IP Packet Type: 1=IPv4 */
>>   #define E1000_ADVTXD_TUCMD_L4T_TCP 0x00000800  /* L4 Packet TYPE of TCP */
>>   #define E1000_ADVTXD_TUCMD_L4T_SCTP 0x00001000 /* L4 packet TYPE of SCTP */
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index 105b0624081a..4131bc8b079e 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -2516,6 +2516,7 @@ igb_features_check(struct sk_buff *skb, struct net_device *dev,
>>   	if (unlikely(mac_hdr_len > IGB_MAX_MAC_HDR_LEN))
>>   		return features & ~(NETIF_F_HW_CSUM |
>>   				    NETIF_F_SCTP_CRC |
>> +				    NETIF_F_GSO_UDP_L4 |
>>   				    NETIF_F_HW_VLAN_CTAG_TX |
>>   				    NETIF_F_TSO |
>>   				    NETIF_F_TSO6);
>> @@ -2524,6 +2525,7 @@ igb_features_check(struct sk_buff *skb, struct net_device *dev,
>>   	if (unlikely(network_hdr_len >  IGB_MAX_NETWORK_HDR_LEN))
>>   		return features & ~(NETIF_F_HW_CSUM |
>>   				    NETIF_F_SCTP_CRC |
>> +				    NETIF_F_GSO_UDP_L4 |
>>   				    NETIF_F_TSO |
>>   				    NETIF_F_TSO6);
>>   
>> @@ -3120,7 +3122,7 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   			    NETIF_F_HW_CSUM;
>>   
>>   	if (hw->mac.type >= e1000_82576)
>> -		netdev->features |= NETIF_F_SCTP_CRC;
>> +		netdev->features |= NETIF_F_SCTP_CRC | NETIF_F_GSO_UDP_L4;
>>   
>>   	if (hw->mac.type >= e1000_i350)
>>   		netdev->features |= NETIF_F_HW_TC;
>> @@ -5694,6 +5696,7 @@ static int igb_tso(struct igb_ring *tx_ring,
>>   	} ip;
>>   	union {
>>   		struct tcphdr *tcp;
>> +		struct udphdr *udp;
>>   		unsigned char *hdr;
>>   	} l4;
>>   	u32 paylen, l4_offset;
>> @@ -5713,7 +5716,8 @@ static int igb_tso(struct igb_ring *tx_ring,
>>   	l4.hdr = skb_checksum_start(skb);
>>   
>>   	/* ADV DTYP TUCMD MKRLOC/ISCSIHEDLEN */
>> -	type_tucmd = E1000_ADVTXD_TUCMD_L4T_TCP;
>> +	type_tucmd = (!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)) ?
>> +		      E1000_ADVTXD_TUCMD_L4T_TCP : E1000_ADVTXD_TUCMD_L4T_UDP;
> 
> The logic here seems a bit convoluted. Instead of testing for
> '!SKB_GSO_UDP_L4' why not just make L4T_UDP the first option and drop the
> '!'? That will make the TCP offload the default case rather than the UDP
> offload.
> 
> The same applies to the other 2 patches.

OK sure. I think I was trying to keep the TCP case first for some 
reason, but agree that I should probably just reverse the order. Will 
send a v3.

Josh