[ofa-general] [PATCHv2] IB/ipoib: S/G and HW checksum support

[Jason Gunthorpe]

On Wed, Sep 05, 2007 at 09:19:13AM +0300, Michael S. Tsirkin wrote:

> > for those packets
> > than it is today - dev_queue_xmit today calls skb_checksum_help on
> > behalf of ipoib for every packet.
> 
> I don't think it does, normally: the packets it gets now usually
> have CHECKSUM_COMPLETE.

Are you sure? This part has changed alot recently, but it used to be
that you never get CHECKSUM_COMPLETE on the TX side, only PARTIAL or
NONE. skb_checksum_help and ip_forward both convert CHECKSUM_CMOMPLETE
to CHECKSUM_NONE. No in tree ethernet driver looks at
CHECKSUM_COMPLETE on the TX path.

The code I am thinking of is the test in dev_queue_xmit:

        /* If packet is not checksummed and device does not support
         * checksumming for this protocol, complete checksumming here.
         */
        if (skb->ip_summed == CHECKSUM_PARTIAL) {
                skb_set_transport_header(skb, skb->csum_start -
                                              skb_headroom(skb));

                if (!(dev->features & NETIF_F_GEN_CSUM) &&
                    (!(dev->features & NETIF_F_IP_CSUM) ||
                     skb->protocol != htons(ETH_P_IP)))
                        if (skb_checksum_help(skb))
                                goto out_kfree_skb;
        }

Since this is the only use of NETIF_F_GEN_CSUM, I assuem that this is
the only place where L4 csum is computed for packets originating
within the host.

> > Also, my other thought was about the RX path, it should work more like
> > 
> >    if (header->flags & cpu_to_be16(IPOIB_HEADER_F_HWCSUM))
> >        ip_summed = CHECKSUM_PARTIAL     // Sender says the csum is bad
> >    else
> >      if (enabled_hw_csum_support)
> >         ip_summed = CHECKSUM_UNNECESSARY // Sender says the csum should be good
> 
> Hmm. Where does this last line come from? It looks wrong ...
> 
> >      else
> >         ip_summed = CHECKSUM_NONE;   // Force checking

The idea I had is if you turn on hw_csum_support then the RX side
never csum checks. It either uses UNNECESSARY or PARTIAL, depending
on the case. If you turn that off, then the RX side csum checks every
packet it can. That addresses this:

> It's not that simple: F_HWCSUM is also a hint for RX side,
> so it might be a win if the *remote* does not have RX checksum
> offloading.

The F_HWCSUM flag is then really better named
IPOIB_HEADER_F_L4_CSUM_UNCOMPUTED

> But yes, maybe I should ignore multicast speed for now, and say
> that it will get fixed by hardware offloading in the future.

Judging by the other comments in this thread, it still seems to me
this would be best as RC only, notionally with the idea that RC is
only used between hosts and not between gateways and hosts
(administratively configured). That way the end-to-end nature of the
checksum is retained. Gateways that want to support RC can negotiate
this feature off.

You may also want to look at using the new TSO/GSO/LRO stuff in a RC
context. If you could send an entire GSO in one go and receive it as a
LRO that might be a big improvement too.

Regards,
Jason