Re: [Intel-wired-lan] [next-queue 4/4] ixgbe: enable tso with ipsec offload

[Shannon Nelson]

On 3/15/2018 3:03 PM, Alexander Duyck wrote:

On Thu, Mar 15, 2018 at 2:23 PM, Shannon Nelson
 wrote:

Fix things up to support TSO offload in conjunction
with IPsec hw offload.  This raises throughput with
IPsec offload on to nearly line rate.

Signed-off-by: Shannon Nelson 
---
  drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c |  7 +--
  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  | 25 +++--
  2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
index 5ddea43..bfbcfc2 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
@@ -896,6 +896,7 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
  void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter)
  {
 struct ixgbe_ipsec *ipsec;
+   netdev_features_t features;
 size_t size;

 if (adapter->hw.mac.type == ixgbe_mac_82598EB)
@@ -929,8 +930,10 @@ void ixgbe_init_ipsec_offload(struct ixgbe_adapter 
*adapter)
 ixgbe_ipsec_clear_hw_tables(adapter);

 adapter->netdev->xfrmdev_ops = _xfrmdev_ops;
-   adapter->netdev->features |= NETIF_F_HW_ESP;
-   adapter->netdev->hw_enc_features |= NETIF_F_HW_ESP;
+
+   features = NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM | NETIF_F_GSO_ESP;
+   adapter->netdev->features |= features;
+   adapter->netdev->hw_enc_features |= features;


Instead of adding the local variable you might just create a new
define that includes these 3 feature flags and then use that here. You
could use the way I did IXGBE_GSO_PARTIAL_FEATURES as an example.


 return;

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index a54f3d8..6022666 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7721,9 +7721,11 @@ static void ixgbe_service_task(struct work_struct *work)

  static int ixgbe_tso(struct ixgbe_ring *tx_ring,
  struct ixgbe_tx_buffer *first,
-u8 *hdr_len)
+u8 *hdr_len,
+struct ixgbe_ipsec_tx_data *itd)
  {
 u32 vlan_macip_lens, type_tucmd, mss_l4len_idx;
+   u32 fceof_saidx = 0;
 struct sk_buff *skb = first->skb;


Reverse xmas tree this. It should probably be moved down to just past
the declaration of paylen and l4_offset.


 union {
 struct iphdr *v4;
@@ -7762,9 +7764,13 @@ static int ixgbe_tso(struct ixgbe_ring *tx_ring,
 unsigned char *trans_start = ip.hdr + (ip.v4->ihl * 4);

 /* IP header will have to cancel out any data that
-* is not a part of the outer IP header
+* is not a part of the outer IP header, except for
+* IPsec where we want the IP+ESP header.
  */
-   ip.v4->check = csum_fold(csum_partial(trans_start,
+   if (first->tx_flags & IXGBE_TX_FLAGS_IPSEC)
+   ip.v4->check = 0;
+   else
+   ip.v4->check = csum_fold(csum_partial(trans_start,
   csum_start - trans_start,
   0));
 type_tucmd |= IXGBE_ADVTXD_TUCMD_IPV4;


I would say this should be flipped like so:
ip.v4->check =  (skb_shinfo(skb)->gso_type & SKB_GSO_PARTIAL) ?
  csum_fold(csum_partial(trans_start,
csum_start - trans_start, 0) : 0;


@@ -7797,12 +7803,15 @@ static int ixgbe_tso(struct ixgbe_ring *tx_ring,
 mss_l4len_idx = (*hdr_len - l4_offset) << IXGBE_ADVTXD_L4LEN_SHIFT;
 mss_l4len_idx |= skb_shinfo(skb)->gso_size << IXGBE_ADVTXD_MSS_SHIFT;

+   fceof_saidx |= itd->sa_idx;
+   type_tucmd |= itd->flags | itd->trailer_len;
+
 /* vlan_macip_lens: HEADLEN, MACLEN, VLAN tag */
 vlan_macip_lens = l4.hdr - ip.hdr;
 vlan_macip_lens |= (ip.hdr - skb->data) << IXGBE_ADVTXD_MACLEN_SHIFT;
 vlan_macip_lens |= first->tx_flags & IXGBE_TX_FLAGS_VLAN_MASK;

-   ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, 0, type_tucmd,
+   ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, fceof_saidx, type_tucmd,
   mss_l4len_idx);

 return 1;
@@ -8493,7 +8502,8 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
 if (skb->sp && !ixgbe_ipsec_tx(tx_ring, first, _tx))
 goto out_drop;
  #endif
-   tso = ixgbe_tso(tx_ring, first, _len);
+
+   tso = ixgbe_tso(tx_ring, first, _len, _tx);
 if (tso < 0)
 goto out_drop;
 else if (!tso)


No need for the extra blank line. I would say just leave it as is and
add your extra argument.


Yep, you're right on all counts.  That SKB_GSO_PARTIAL 

Re: [Intel-wired-lan] [next-queue 4/4] ixgbe: enable tso with ipsec offload

[Alexander Duyck]

On Thu, Mar 15, 2018 at 2:23 PM, Shannon Nelson
 wrote:
> Fix things up to support TSO offload in conjunction
> with IPsec hw offload.  This raises throughput with
> IPsec offload on to nearly line rate.
>
> Signed-off-by: Shannon Nelson 
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c |  7 +--
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  | 25 +++--
>  2 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c 
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> index 5ddea43..bfbcfc2 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> @@ -896,6 +896,7 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring,
>  void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter)
>  {
> struct ixgbe_ipsec *ipsec;
> +   netdev_features_t features;
> size_t size;
>
> if (adapter->hw.mac.type == ixgbe_mac_82598EB)
> @@ -929,8 +930,10 @@ void ixgbe_init_ipsec_offload(struct ixgbe_adapter 
> *adapter)
> ixgbe_ipsec_clear_hw_tables(adapter);
>
> adapter->netdev->xfrmdev_ops = _xfrmdev_ops;
> -   adapter->netdev->features |= NETIF_F_HW_ESP;
> -   adapter->netdev->hw_enc_features |= NETIF_F_HW_ESP;
> +
> +   features = NETIF_F_HW_ESP | NETIF_F_HW_ESP_TX_CSUM | NETIF_F_GSO_ESP;
> +   adapter->netdev->features |= features;
> +   adapter->netdev->hw_enc_features |= features;

Instead of adding the local variable you might just create a new
define that includes these 3 feature flags and then use that here. You
could use the way I did IXGBE_GSO_PARTIAL_FEATURES as an example.

> return;
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index a54f3d8..6022666 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7721,9 +7721,11 @@ static void ixgbe_service_task(struct work_struct 
> *work)
>
>  static int ixgbe_tso(struct ixgbe_ring *tx_ring,
>  struct ixgbe_tx_buffer *first,
> -u8 *hdr_len)
> +u8 *hdr_len,
> +struct ixgbe_ipsec_tx_data *itd)
>  {
> u32 vlan_macip_lens, type_tucmd, mss_l4len_idx;
> +   u32 fceof_saidx = 0;
> struct sk_buff *skb = first->skb;

Reverse xmas tree this. It should probably be moved down to just past
the declaration of paylen and l4_offset.

> union {
> struct iphdr *v4;
> @@ -7762,9 +7764,13 @@ static int ixgbe_tso(struct ixgbe_ring *tx_ring,
> unsigned char *trans_start = ip.hdr + (ip.v4->ihl * 4);
>
> /* IP header will have to cancel out any data that
> -* is not a part of the outer IP header
> +* is not a part of the outer IP header, except for
> +* IPsec where we want the IP+ESP header.
>  */
> -   ip.v4->check = csum_fold(csum_partial(trans_start,
> +   if (first->tx_flags & IXGBE_TX_FLAGS_IPSEC)
> +   ip.v4->check = 0;
> +   else
> +   ip.v4->check = csum_fold(csum_partial(trans_start,
>   csum_start - 
> trans_start,
>   0));
> type_tucmd |= IXGBE_ADVTXD_TUCMD_IPV4;

I would say this should be flipped like so:
ip.v4->check =  (skb_shinfo(skb)->gso_type & SKB_GSO_PARTIAL) ?
 csum_fold(csum_partial(trans_start,
csum_start - trans_start, 0) : 0;

> @@ -7797,12 +7803,15 @@ static int ixgbe_tso(struct ixgbe_ring *tx_ring,
> mss_l4len_idx = (*hdr_len - l4_offset) << IXGBE_ADVTXD_L4LEN_SHIFT;
> mss_l4len_idx |= skb_shinfo(skb)->gso_size << IXGBE_ADVTXD_MSS_SHIFT;
>
> +   fceof_saidx |= itd->sa_idx;
> +   type_tucmd |= itd->flags | itd->trailer_len;
> +
> /* vlan_macip_lens: HEADLEN, MACLEN, VLAN tag */
> vlan_macip_lens = l4.hdr - ip.hdr;
> vlan_macip_lens |= (ip.hdr - skb->data) << IXGBE_ADVTXD_MACLEN_SHIFT;
> vlan_macip_lens |= first->tx_flags & IXGBE_TX_FLAGS_VLAN_MASK;
>
> -   ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, 0, type_tucmd,
> +   ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, fceof_saidx, type_tucmd,
>   mss_l4len_idx);
>
> return 1;
> @@ -8493,7 +8502,8 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
> if (skb->sp && !ixgbe_ipsec_tx(tx_ring, first, _tx))
> goto out_drop;
>  #endif
> -   tso = ixgbe_tso(tx_ring, first, _len);
> +
> +   tso = ixgbe_tso(tx_ring, first, _len, _tx);
> if (tso < 0)
> goto out_drop;
> else if (!tso)

No need for the extra blank line. I would say just leave