Re: [PATCH RFC 0/3] net: Add driver helper function to determine if checksum is offloadable

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3106 bytes --]

On Mon, 2015-10-05 at 16:39 -0700, Tom Herbert wrote:
> When drivers have support for offloading transport IP checksums, they
> will indicate this in the features for the device. As described in
> skbuff.h, a driver will usually advertise NETIF_F_HW_CSUM,
> NETIF_F_IP_CSUM, and/or NETIF_F_IPV6_CSUM. The first of these
> (NETIF_F_HW_CSUM) is the preferred method since this implies that the
> device has implemented a generic checksum offload feature that should
> work under arbitrary scenarios (e.g. for different protocols, with or
> without encapsulation).
> 
> For narrow features support (NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM
> for offload of TCP/UDP), the features flags may not be sufficient to
> deduce whether a packet may be offloaded. Some devices will not be able
> to offload encapsulated checksums, some cannot offload transport
> checksums in packet with IPv6 extension headers, etc. In these cases
> a driver will need to perform additional packet inspection to determine
> if a packet's checksum can be offloaded to a device.
> 
> This patch defines a helper function that drivers can call to check if
> it is able to offload the checksum for a particular packet. In an
> argument to the function, the driver specifies what type of packets it
> is able to offload to a device. The function is intended to check for
> the most common restrictions of devices (like by IP version, transport
> protocol, encapsulation, extension headers). Since the function includes
> checks for IP version and transport protocol, the driver is able
> to advertise NETIF_F_HW_CSUM instead of protocol specific support.
> This should put us on a path to deprecate NETIF_F_IP_CSUM and
> NETIF_F_IPV6_CSUM.
> 
> The helper function may be called from ndo_features_check or the xmit
> routine of the driver. The csum_help argument does not need to be set
> when the function is called from ndo_features_check.

Looks good in general; thanks.

I do strongly believe we want to encourage your helper to be called
from .ndo_features_check(). Because if you can't do the checksum, then
you can't do TSO. And if you can't do TSO, you *really* want your
hard_start_xmit() function to be handed one skb at a time and not have
to call skb_gso_segment() for itself when it might not have enough room
in its descriptor ring for *all* the resulting segments.

Also, is your skb_csum_offload_chk() going to do the right thing for
SCTP packets? Looks like it doesn't do crc32...

And how accurate is the check we have, on the various IP output
routines, for rt->dst.dev->features & NETIF_F_xx_CSUM? Could we
consider just ditching that check and always using CHECKSUM_PARTIAL, in
the knowledge that we check it before it hits a non-capable driver
anyway? Or do we benefit from doing the software checksum early, due to
improved cache locality when we've probably just copied the data from
userspace in many cases?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]