netdev - Re: [PATCH] improve mvneta (Marvell 370) driver TCPv6 performance [1]

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] [day] [month] [year] [list]
Date:	Fri, 26 Jun 2015 23:03:32 +0200
From:	Andrew Lunn <andrew@...n.ch>
To:	Phil Hofer <philhofer@...eoussystems.com>
Cc:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	netdev@...r.kernel.org, Oren Laskin <orenlaskin@...eoussystems.com>
Subject: Re: [PATCH] improve mvneta (Marvell 370) driver TCPv6 performance [1]

On Fri, Jun 26, 2015 at 01:31:09PM -0700, Phil Hofer wrote:
> I've made some changes based on advice from Willy Tarreau.
> The changes don't influence the IPv6 performance numbers on our board
> relative to the previous patch. I still get about 90% of IPv4
> performance.
> 
> Enabling NETIF_F_TSO6 still causes problems; that will probably have
> to be addressed in a separate patch. I still haven't figured out why
> that breaks the driver.

Hi Phil

The Change Log message is supposed to explain what the patch does, and
importantly, why. It will land in the git repository as documentation
for the patch. Please could you re-write you log.

> Signed off: Phil Hofer <philhofer@...eoussystems.com>
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c
> b/drivers/net/ethernet/marvell/mvneta.c
> index 5bdf782..3e0a90a 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -338,7 +338,8 @@ struct mvneta_port {
>  #define MVNETA_RXD_ERR_LEN             BIT(18)
>  #define MVNETA_RXD_ERR_RESOURCE                (BIT(17) | BIT(18))
>  #define MVNETA_RXD_ERR_CODE_MASK       (BIT(17) | BIT(18))
> -#define MVNETA_RXD_L3_IP4              BIT(25)
> +#define MVNETA_RXD_L3_IP               BIT(24)
> +#define MVNETA_RXD_IP_HEAD_OK          BIT(25)
>  #define MVNETA_RXD_FIRST_LAST_DESC     (BIT(26) | BIT(27))
>  #define MVNETA_RXD_L4_CSUM_OK          BIT(30)
> 
> @@ -1235,13 +1236,17 @@ static u32 mvneta_txq_desc_csum(int l3_offs,
> int l3_proto,
>         command =  l3_offs    << MVNETA_TX_L3_OFF_SHIFT;
>         command |= ip_hdr_len << MVNETA_TX_IP_HLEN_SHIFT;
> 
> +       /* IPv6 packet header checksumming
> +        * is not supported, but TCPv6 header
> +        * checksumming *is* supported.
> +        */
>         if (l3_proto == htons(ETH_P_IP))
>                 command |= MVNETA_TXD_IP_CSUM;
> -       else
> +       else if (l3_proto == htons(ETH_P_IPV6))
>                 command |= MVNETA_TX_L3_IP6;
> 
>         if (l4_proto == IPPROTO_TCP)
> -               command |=  MVNETA_TX_L4_CSUM_FULL;
> +               command |= MVNETA_TX_L4_CSUM_FULL;

What white space change should really be in a different patch.

>         else if (l4_proto == IPPROTO_UDP)
>                 command |= MVNETA_TX_L4_UDP | MVNETA_TX_L4_CSUM_FULL;
>         else
> @@ -1288,13 +1293,16 @@ static void mvneta_rx_error(struct mvneta_port *pp,
>  static void mvneta_rx_csum(struct mvneta_port *pp, u32 status,
>                            struct sk_buff *skb)
>  {
> -       if ((status & MVNETA_RXD_L3_IP4) &&
> -           (status & MVNETA_RXD_L4_CSUM_OK)) {
> +       /* IPHeadOk will be set for IPv6
> +        * and IPv4 packets with proper
> +        * checksums.
> +        */
> +       if ((status & MVNETA_RXD_IP_HEAD_OK) &&
> +               (status & MVNETA_RXD_L4_CSUM_OK)) {
>                 skb->csum = 0;
>                 skb->ip_summed = CHECKSUM_UNNECESSARY;
>                 return;
>         }
> -

Unneeded white space change.

>         skb->ip_summed = CHECKSUM_NONE;
>  }
> 
> @@ -3129,7 +3137,7 @@ static int mvneta_probe(struct platform_device *pdev)
> 
>         netif_napi_add(dev, &pp->napi, mvneta_poll, NAPI_POLL_WEIGHT);
> 
> -       dev->features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
> +       dev->features = NETIF_F_SG | NETIF_F_IP_CSUM |
> NETIF_F_IPV6_CSUM | NETIF_F_TSO;

It looks like you patch has been mangled by your mail client. Please
use git send-email to avoid such issues.

    Andrew

>         dev->hw_features |= dev->features;
>         dev->vlan_features |= dev->features;
>         dev->priv_flags |= IFF_UNICAST_FLT;
> 
> 
> On Fri, Jun 26, 2015 at 12:57 PM, Phil Hofer
> <philhofer@...eoussystems.com> wrote:
> > My apologies.
> >
> > I have a follow-up on the way with some of Willy's suggested changes as well.
> >
> > On Fri, Jun 26, 2015 at 11:52 AM, Thomas Petazzoni
> > <thomas.petazzoni@...e-electrons.com> wrote:
> >> Hello Phil,
> >>
> >> On Fri, 26 Jun 2015 11:12:03 -0700, Phil Hofer wrote:
> >>> TCP performance over IPv6 was only about 60% of IPv4 performance. This
> >>> patch makes up most (but not all) of the difference.
> >>>
> >>> Signed off by: Thomas Petazzoni (thomas.petazzoni@...e-electrons.com)
> >>
> >> What makes you think you can use my Signed-off-by for a patch that I
> >> haven't written? It should be your Signed-off-by here, certainly not
> >> mine.
> >>
> >> Also, the e-mail address should be between <...>, not between (...).
> >>
> >> Thanks!
> >>
> >> Thomas
> >> --
> >> Thomas Petazzoni, CTO, Free Electrons
> >> Embedded Linux, Kernel and Android engineering
> >> http://free-electrons.com
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html