netdev - RE: [net-next v3 20/29] fm10k: Add support for netdev offloads

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <D29CC2C771335549BDCBB23A2B92BE379C7F84CE@ORSMSX110.amr.corp.intel.com>
Date:	Wed, 1 Oct 2014 13:52:37 +0000
From:	"Duyck, Alexander H" <alexander.h.duyck@...el.com>
To:	Sathya Perla <Sathya.Perla@...lex.Com>,
	Or Gerlitz <gerlitz.or@...il.com>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC:	David Miller <davem@...emloft.net>,
	Linux Netdev List <netdev@...r.kernel.org>,
	"nhorman@...hat.com" <nhorman@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>,
	"jogreene@...hat.com" <jogreene@...hat.com>,
	Tom Herbert <therbert@...gle.com>
Subject: RE: [net-next v3 20/29] fm10k: Add support for netdev offloads

> -----Original Message-----
> From: Sathya Perla [mailto:Sathya.Perla@...lex.Com]
> Sent: Wednesday, October 01, 2014 5:22 AM
> To: Duyck, Alexander H; Or Gerlitz; Kirsher, Jeffrey T
> Cc: David Miller; Linux Netdev List; nhorman@...hat.com;
> sassmann@...hat.com; jogreene@...hat.com; Tom Herbert
> Subject: RE: [net-next v3 20/29] fm10k: Add support for netdev offloads
> 
> 
> > -----Original Message-----
> > From: netdev-owner@...r.kernel.org [mailto:netdev-
> >
> > On 10/01/2014 01:31 AM, Or Gerlitz wrote:
> > > On Tue, Sep 23, 2014 at 2:16 PM, Jeff Kirsher
> > > <jeffrey.t.kirsher@...el.com> wrote:
> > >> From: Alexander Duyck <alexander.h.duyck@...el.com> This patch adds
> > >> support for basic offloads including TSO, Tx checksum, Rx checksum,
> > >> Rx hash, and the same features applied to VXLAN/NVGRE
> > tunnels.
> > >
> > >> --- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > >> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
> > > [...]
> > >> +#define VXLAN_HLEN (sizeof(struct udphdr) + 8) static struct
> > >> +ethhdr *fm10k_port_is_vxlan(struct sk_buff *skb) {
> > >> +       struct fm10k_intfc *interface = netdev_priv(skb->dev);
> > >> +       struct fm10k_vxlan_port *vxlan_port;
> > >> +
> > >> +       /* we can only offload a vxlan if we recognize it as such */
> > >> +       vxlan_port = list_first_entry_or_null(&interface->vxlan_port,
> > >> +                                             struct
> > >> + fm10k_vxlan_port, list);
> > >> +
> > >> +       if (!vxlan_port)
> > >> +               return NULL;
> > >> +       if (vxlan_port->port != udp_hdr(skb)->dest)
> > >> +               return NULL;
> > >> +
> > >> +       /* return offset of udp_hdr plus 8 bytes for VXLAN header */
> > >> +       return (struct ethhdr *)(skb_transport_header(skb) +
> > >> +VXLAN_HLEN); }
> > >> +
> ...
> >
> > >> +static int fm10k_tso(struct fm10k_ring *tx_ring,
> > >> +                    struct fm10k_tx_buffer *first) {
> > >> +       struct sk_buff *skb = first->skb;
> > >> +       struct fm10k_tx_desc *tx_desc;
> > >> +       unsigned char *th;
> > >> +       u8 hdrlen;
> > >> +
> > >> +       if (skb->ip_summed != CHECKSUM_PARTIAL)
> > >> +               return 0;
> > >> +
> > >> +       if (!skb_is_gso(skb))
> > >> +               return 0;
> > >> +
> > >> +       /* compute header lengths */
> > >> +       if (skb->encapsulation) {
> > >> +               if (!fm10k_tx_encap_offload(skb))
> > >> +                       goto err_vxlan;
> > >> +               th = skb_inner_transport_header(skb);
> > >> +       } else {
> > >> +               th = skb_transport_header(skb);
> > >> +       }
> > >> +
> > >> +       /* compute offset from SOF to transport header and add
> > >> + header len
> > */
> > >> +       hdrlen = (th - skb->data) + (((struct tcphdr *)th)->doff <<
> > >> + 2);
> > >> +
> > >> +       first->tx_flags |= FM10K_TX_FLAGS_CSUM;
> > >> +
> > >> +       /* update gso size and bytecount with header size */
> > >> +       first->gso_segs = skb_shinfo(skb)->gso_segs;
> > >> +       first->bytecount += (first->gso_segs - 1) * hdrlen;
> > >> +
> > >> +       /* populate Tx descriptor header size and mss */
> > >> +       tx_desc = FM10K_TX_DESC(tx_ring, tx_ring->next_to_use);
> > >> +       tx_desc->hdrlen = hdrlen;
> > >> +       tx_desc->mss = cpu_to_le16(skb_shinfo(skb)->gso_size);
> > >> +
> > >> +       return 1;
> > >> +err_vxlan:
> > >> +       tx_ring->netdev->features &= ~NETIF_F_GSO_UDP_TUNNEL;
> > >> +       if (!net_ratelimit())
> > >> +               netdev_err(tx_ring->netdev,
> > >> +                          "TSO requested for unsupported tunnel,
> > >> +disabling
> > offload\n");
> > >> +       return -1;
> > >> +}
> > >
> > > why? if TSO was requested for some packet the driver can't do, you
> > > disable
> > GSO
> > > for udp tunnels for any future packets too? maybe just disable it
> > > permanently  till you feel safer to run under the current stack?
> >
> > That is essentially what I am doing.  If the user wants they can turn
> > the feature back on via ethtool, but there is no point in having it on
> > if the device itself cannot support the packets that are coming through.
> >
> 
> To turn off the offload, shouldn't you be using netdev_update_features()
> (->ndo_fix_features()) instead of directly flipping bits in netdev->features?

Why?  That would just be me calling my own ndo operation wouldn't it, since I am changing the features of the device covered by this driver.

Thanks,

Alex