netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Tom Herbert <tom@herbertland.com>
To: Davide Caratti <dcaratti@redhat.com>
Cc: David Laight <David.Laight@aculab.com>,
	"David S. Miller" <davem@davemloft.net>,
	Linux Kernel Network Developers <netdev@vger.kernel.org>,
	"linux-sctp@vger.kernel.org" <linux-sctp@vger.kernel.org>,
	Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Subject: Re: [RFC PATCH net-next 2/5] net: split skb_checksum_help
Date: Mon, 27 Feb 2017 07:11:03 -0800	[thread overview]
Message-ID: <CALx6S36bZ_tZrXxBj9fNActemMDKYJzA5K1vK9jVED9z9+FwBg@mail.gmail.com> (raw)
In-Reply-To: <1488202783.2713.67.camel@redhat.com>

On Mon, Feb 27, 2017 at 5:39 AM, Davide Caratti <dcaratti@redhat.com> wrote:
> On Mon, 2017-01-23 at 12:59 -0800, Tom Herbert wrote:
>> > > > It might make sense to create some CRC helper functions, but last time
>> > > > I checked there are so few users of CRC in skbufs I'm not even sure
>> > > > that would make sense.
>
> hello Tom and David,
>
> after some (thinking + testing) time, I'm going to re-post this RFC as v2 with
> some feedbacks. Thank you in advance for looking at it!
>
> On Thu, 2017-02-02 at 10:08 -0800, Tom Herbert wrote:
>> On Thu, 2017-02-02 at 16:07 +0100, Davide Caratti wrote:
>> > This is exactly the cause of issues I see with SCTP. These packets can be
>> > wrongly checksummed using skb_checksum_help, or simply not checksummed at
>> > all; and in both cases, the packet goes out from the NIC with wrong L4
>> > checksum.
>> >
>> Okay, makes sense. Please consider doing the following:
>>
>> - Add a bit to skbuf called something like "csum_not_inet". When
>> ip_summed == CHECKSUM_PARTIAL and this bit is set that means we are
>> dealing with something other than an Internet checksum.
>
> Ok, done. Another solution would be to extend possible values of
> skb->ip_summed, and define a new value suitable for identifying
> not-yet-checksummed SCTP packets (something like CRC32C_PARTIAL). Since
> skb->ip_summed is 2-bit wide, the overall effect on skb metadata is the
> same as adding skb->csum_not_inet [1].
>
>> - At the top of skb_checksum_help (or maybe before the point where the
>> inet specific checksum start begins do something like:
>>
>>    if (unlikely(skb->csum_not_inet))
>>        return skb_checksum_help_not_inet(...);
>>
>>    The rest of skb_checksum_help should remained unchanged.
>
> According to documentation [2], validate_xmit_skb() is a good place where
> the if() statement above can be done, to preserve the possibility of having
> the CRC32c computation offloaded by the NIC hardware:
>
> if (unlikely(skb->csum_not_inet && !(features & NETIF_F_SCTP_CRC))
>                return skb_checksum_help_not_inet(...);
>
> On Thu, 2017-02-02 at 16:55 +0000, David Laight wrote:
>>
>> I'd put the onus on any such interface to perform the checksum (and
>> set CHECKSUM_COMPLETE (or is it UNNECESSARY?) before passing the
>> message onto an interface that doesn't advertise CRC32 support.
>>
>> You certainly don't want to have to go through all the ethernet drivers!
>
> Ideally, a driver not able to offload checksum computation should call
> skb_checksum_help() or skb_sctp_csum_help() to resolve CHECKSUM_PARTIAL
> and turn it to CHECKSUM_NONE.
> But this wouldn't solve all possible setups: there can be scenarios
> where the NIC is configured with NETIF_F_SCTP_CRC set and NETIF_F_CSUM_HW
> cleared (it's evil, but possible). In this situation, non-GSO SCTP packets
> having CHECKSUM_PARTIAL will be systematically corrupted when they are
> processed by validate_xmit_skb().
>
> On Thu, 2017-02-02 at 10:08 -0800, Tom Herbert wrote:
>
>>
>> - Add a description of the new bit and how skb_checksum_help can work
>> to the comments for CHECKSUM_PARTIAL in skbuff.h
>
> Done.
>
>>
>> - Add FCOE to the list of protocol that can set CHECKSUM_UNNECESSARY
>> for a CRC/csum
>
> Done.
>
>>
>> - Add a note to CHECKSUM_COMPLETE section that it can only refer to an
>> Internet checksum
>
> Done.
>
> /* references + notes */
>
> [1] ... this recalls to latest comment from David Laight:
> On Thu, 2017-02-02 at 16:55 +0000, David Laight wrote:
>>
>> I have to admit to not knowing exactly what the CHECKSUM_xxx flags
>> actually mean. I have a good idea about what the intention is though.
>
> According to domumentation, CHECKSUM_COMPLETE and CHECKSUM_UNNECESSARY are
> not used for SCTP (nor in the TX path at all); nevertheless, IPVS snat/dnat
> actually set CHECKSUM_UNNECESSARY on SCTP packets after the checksum is
> updated (see 97203abe6bc4 "net: ipvs: sctp: do not recalc...).
>
CHECKSUM_PARTIAL is the preferred mechanism on the transmit path this
defers defers the checksum computation as long as possible.
Unfortunately, if SCTP is encapsulated in UDP we will probably need to
run the SCTP CRC on the host which will be done with your changes to
skb_checksum_help.

> I'm not sure if setting CHECKSUM_UNNECESSARY fits my case, because this would
> implicitly skip RX validation when using devices like veth or loopback.
>
CHECKSUM_UNNECESSARY can be used in the transmit path (really the
forwarding path), however this I think this must imply that the
checksum in the packet must be correct. Please see my post about
drivers that are mistakingly using CHECKSUM_UNNECESSARY with LRO since
the checksum in the packet sent into the stack is not correct.

Tom

> [2] Documentation/networking/checksum_offloads.txt
>
> regards,
> --
> davide

  reply	other threads:[~2017-02-27 15:18 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-23 16:52 [RFC PATCH net-next 0/5] net: improve support for SCTP checksums Davide Caratti
2017-01-23 16:52 ` [RFC PATCH net-next 1/5] skbuff: add stub to help computing crc32c on SCTP packets Davide Caratti
2017-01-23 16:52 ` [RFC PATCH net-next 2/5] net: split skb_checksum_help Davide Caratti
2017-01-23 20:59   ` Tom Herbert
2017-01-24 16:35     ` David Laight
2017-02-02 15:07       ` Davide Caratti
2017-02-02 16:55         ` David Laight
2017-02-02 18:08         ` Tom Herbert
2017-02-27 13:39           ` Davide Caratti
2017-02-27 15:11             ` Tom Herbert [this message]
2017-02-28 10:31               ` Davide Caratti
2017-02-28 10:32             ` [PATCH RFC net-next v2 1/4] skbuff: add stub to help computing crc32c on SCTP packets Davide Caratti
2017-02-28 10:32               ` [PATCH RFC net-next v2 2/4] net: introduce skb_sctp_csum_help Davide Caratti
2017-02-28 10:32               ` [PATCH RFC net-next v2 3/4] net: more accurate checksumming in validate_xmit_skb Davide Caratti
2017-02-28 19:50                 ` Tom Herbert
2017-02-28 10:32               ` [PATCH RFC net-next v2 4/4] Documentation: update notes on checksum offloading Davide Caratti
2017-02-28 22:46               ` [PATCH RFC net-next v2 1/4] skbuff: add stub to help computing crc32c on SCTP packets Alexander Duyck
2017-03-01  3:17                 ` Tom Herbert
2017-03-01 10:53                 ` David Laight
2017-03-06 21:51                 ` Davide Caratti
2017-03-07 18:06                   ` Alexander Duyck
2017-03-18 13:17                     ` Davide Caratti
2017-03-18 22:35                       ` Tom Herbert
2017-04-07 14:16                         ` [PATCH RFC net-next v3 0/7] improve CRC32c in the forwarding path Davide Caratti
2017-04-07 14:16                           ` [PATCH RFC net-next v3 1/7] skbuff: add stub to help computing crc32c on SCTP packets Davide Caratti
2017-04-07 14:16                           ` [PATCH RFC net-next v3 2/7] net: introduce skb_crc32c_csum_help Davide Caratti
2017-04-07 14:16                           ` [PATCH RFC net-next v3 3/7] sk_buff: remove support for csum_bad in sk_buff Davide Caratti
2017-04-07 14:16                           ` [PATCH RFC net-next v3 4/7] net: use skb->csum_algo to identify packets needing crc32c Davide Caratti
2017-04-07 15:43                             ` Tom Herbert
2017-04-07 17:29                               ` Davide Caratti
2017-04-07 18:11                                 ` Tom Herbert
2017-04-13 10:36                                   ` Davide Caratti
2017-04-20 13:38                                   ` [PATCH RFC net-next v4 0/7] net: improve support for SCTP checksums Davide Caratti
2017-04-20 13:38                                     ` [PATCH RFC net-next v4 1/7] skbuff: add stub to help computing crc32c on SCTP packets Davide Caratti
2017-04-20 13:38                                     ` [PATCH RFC net-next v4 2/7] net: introduce skb_crc32c_csum_help Davide Caratti
2017-04-27 12:29                                       ` Marcelo Ricardo Leitner
2017-04-20 13:38                                     ` [PATCH RFC net-next v4 3/7] sk_buff: remove support for csum_bad in sk_buff Davide Caratti
2017-04-27  1:34                                       ` [sk_buff] 95510aef27: BUG:Bad_page_state_in_process kernel test robot
2017-04-29 20:21                                       ` [PATCH RFC net-next v4 3/7] sk_buff: remove support for csum_bad in sk_buff Tom Herbert
2017-04-20 13:38                                     ` [PATCH RFC net-next v4 4/7] net: use skb->csum_not_inet to identify packets needing crc32c Davide Caratti
2017-04-29 20:18                                       ` Tom Herbert
2017-04-20 13:38                                     ` [PATCH RFC net-next v4 5/7] net: more accurate checksumming in validate_xmit_skb() Davide Caratti
2017-04-20 13:38                                     ` [PATCH RFC net-next v4 6/7] openvswitch: more accurate checksumming in queue_userspace_packet() Davide Caratti
2017-04-20 13:38                                     ` [PATCH RFC net-next v4 7/7] sk_buff.h: improve description of CHECKSUM_{COMPLETE,UNNECESSARY} Davide Caratti
2017-04-29 20:20                                       ` Tom Herbert
2017-04-27 12:41                                     ` [PATCH RFC net-next v4 0/7] net: improve support for SCTP checksums Marcelo Ricardo Leitner
2017-04-07 14:16                           ` [PATCH RFC net-next v3 5/7] net: more accurate checksumming in validate_xmit_skb() Davide Caratti
2017-04-07 14:16                           ` [PATCH RFC net-next v3 6/7] openvswitch: more accurate checksumming in queue_userspace_packet() Davide Caratti
2017-04-07 14:16                           ` [PATCH RFC net-next v3 7/7] sk_buff.h: improve description of CHECKSUM_{COMPLETE,UNNECESSARY} Davide Caratti
2017-01-23 16:52 ` [RFC PATCH net-next 3/5] net: introduce skb_sctp_csum_help Davide Caratti
2017-01-23 16:52 ` [RFC PATCH net-next 4/5] net: more accurate checksumming in validate_xmit_skb Davide Caratti
2017-01-23 16:52 ` [RFC PATCH net-next 5/5] Documentation: add description of skb_sctp_csum_help Davide Caratti

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CALx6S36bZ_tZrXxBj9fNActemMDKYJzA5K1vK9jVED9z9+FwBg@mail.gmail.com \
    --to=tom@herbertland.com \
    --cc=David.Laight@aculab.com \
    --cc=davem@davemloft.net \
    --cc=dcaratti@redhat.com \
    --cc=linux-sctp@vger.kernel.org \
    --cc=marcelo.leitner@gmail.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).