All of lore.kernel.org
 help / color / mirror / Atom feed
From: Shmulik Ladkani <shmulik@metanetworks.com>
To: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>,
	Eric Dumazet <eric.dumazet@gmail.com>,
	Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
	eyal@metanetworks.com, netdev <netdev@vger.kernel.org>,
	Shmulik Ladkani <shmulik.ladkani@gmail.com>
Subject: Re: [PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list
Date: Fri, 6 Sep 2019 09:20:42 +0300	[thread overview]
Message-ID: <20190906092042.08f980e3@pixies> (raw)
In-Reply-To: <CAKgT0Uf-OvKKycJz7aTZ93J=RdUuwd=SFS9C9pTngieDxe0uYQ@mail.gmail.com>

On Thu, 5 Sep 2019 14:49:44 -0700
Alexander Duyck <alexander.duyck@gmail.com> wrote:

> I would change the order of the tests you use here so that we can
> eliminate the possibility of needing to perform many tests for the
> more common cases. You could probably swap "list_skb" and "mss !=
> GSO_BY_FRAGS" since list_skb is more likely to be false for many of
> the common cases such as a standard TSO send from a socket. You might
> even consider moving the GSO_BY_FRAGS check toward the end of your
> checks since SCTP is the only protocol that I believe uses it and the
> likelihood of encountering it is much lower compared to other
> protocols.
> 
> You could probably test for !list_skb->head_frag before seeing if
> there is a headlen since many NICs would be generating frames using
> head_frag, so in the GRO case you mentioned above it could probably
> save you some effort on a number of NICs.
> 
> You might also consider moving this code up before we push the mac
> header back on and instead of setting sg to false you could just clear
> the NETIF_F_SG flag from features. It would save you from having to
> then remove doffset in your last check.

Thanks Alexander for the input. Will encorporate as much as possible
into a v2 patch.

BTW, I've strugged with myself regarding order of tests, and came
up with this suggestion, as my motivation was to have the tests order
tell a coherent logical story when read top-to-bottom left-to-right, FWIW.
For example, although 'mss != skb_headlen(head_skb)' could be tested
earlier, the condition by itself isn't meaningful unless we have an
existing frag_list and with a !head_frag.

Best
Shmulik

  reply	other threads:[~2019-09-06  6:20 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-05 18:36 [PATCH net] net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list Shmulik Ladkani
2019-09-05 21:49 ` Alexander Duyck
2019-09-06  6:20   ` Shmulik Ladkani [this message]
2019-09-05 21:51 ` Willem de Bruijn
2019-09-06  6:47   ` Shmulik Ladkani
2019-09-06 14:49     ` Willem de Bruijn
2019-09-06 15:37       ` Shmulik Ladkani
2019-09-06 15:42         ` Alexander Duyck
2019-09-06 16:44           ` Willem de Bruijn
2019-09-06 20:02             ` Shmulik Ladkani

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=20190906092042.08f980e3@pixies \
    --to=shmulik@metanetworks.com \
    --cc=alexander.duyck@gmail.com \
    --cc=daniel@iogearbox.net \
    --cc=eric.dumazet@gmail.com \
    --cc=eyal@metanetworks.com \
    --cc=netdev@vger.kernel.org \
    --cc=shmulik.ladkani@gmail.com \
    --cc=willemdebruijn.kernel@gmail.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.