All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paolo Abeni <pabeni@redhat.com>
To: Eric Dumazet <edumazet@google.com>
Cc: Yunsheng Lin <linyunsheng@huawei.com>,
	David Miller <davem@davemloft.net>,
	 Jakub Kicinski <kuba@kernel.org>,
	MPTCP Upstream <mptcp@lists.linux.dev>,
	netdev <netdev@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	 linuxarm@openeuler.org,
	Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
	David Ahern <dsahern@kernel.org>
Subject: Re: [PATCH net-next] tcp: add tcp_tx_skb_cache_key checking in sk_stream_alloc_skb()
Date: Wed, 01 Sep 2021 18:01:03 +0200	[thread overview]
Message-ID: <59ad13bb312805bb1d183c5817d5f7b6fd6a90dd.camel@redhat.com> (raw)
In-Reply-To: <c40a178110ee705b2be32272b9b3e512a40a4cae.camel@redhat.com>

On Wed, 2021-09-01 at 17:25 +0200, Paolo Abeni wrote:
> On Wed, 2021-09-01 at 08:16 -0700, Eric Dumazet wrote:
> > On Wed, Sep 1, 2021 at 8:06 AM Eric Dumazet <edumazet@google.com> wrote:
> > > On Wed, Sep 1, 2021 at 3:52 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > > > On Wed, 2021-09-01 at 18:39 +0800, Yunsheng Lin wrote:
> > > > > Since tcp_tx_skb_cache is disabled by default in:
> > > > > commit 0b7d7f6b2208 ("tcp: add tcp_tx_skb_cache sysctl")
> > > > > 
> > > > > Add tcp_tx_skb_cache_key checking in sk_stream_alloc_skb() to
> > > > > avoid possible branch-misses.
> > > > > 
> > > > > Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> > > > 
> > > > Note that MPTCP is currently exploiting sk->sk_tx_skb_cache. If we get
> > > > this patch goes in as-is, it will break mptcp.
> > > > 
> > > > One possible solution would be to let mptcp usage enable sk-
> > > > > sk_tx_skb_cache, but that has relevant side effects on plain TCP.
> > > > 
> > > > Another options would be re-work once again the mptcp xmit path to
> > > > avoid using sk->sk_tx_skb_cache.
> > > > 
> > > 
> > > Hmmm, I actually wrote a revert of this feature but forgot to submit
> > > it last year.
> > > 
> > > commit c36cfbd791f62c0f7c6b32132af59dfdbe6be21b (HEAD -> listener_scale4)
> > > Author: Eric Dumazet <edumazet@google.com>
> > > Date:   Wed May 20 06:38:38 2020 -0700
> > > 
> > >     tcp: remove sk_{tr}x_skb_cache
> > > 
> > >     This reverts the following patches :
> > > 
> > >     2e05fcae83c41eb2df10558338dc600dc783af47 ("tcp: fix compile error
> > > if !CONFIG_SYSCTL")
> > >     4f661542a40217713f2cee0bb6678fbb30d9d367 ("tcp: fix zerocopy and
> > > notsent_lowat issues")
> > >     472c2e07eef045145bc1493cc94a01c87140780a ("tcp: add one skb cache for tx")
> > >     8b27dae5a2e89a61c46c6dbc76c040c0e6d0ed4c ("tcp: add one skb cache for rx")
> > > 
> > >     Having a cache of one skb (in each direction) per TCP socket is fragile,
> > >     since it can cause a significant increase of memory needs,
> > >     and not good enough for high speed flows anyway where more than one skb
> > >     is needed.
> > > 
> > >     We want instead to add a generic infrastructure, with more flexible per-cpu
> > >     caches, for alien NUMA nodes.
> > > 
> > >     Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > 
> > > I will update this commit to also remove the part in MPTCP.
> > > 
> > > Let's remove this feature and replace it with something less costly.
> > 
> > Paolo, can you work on MPTP side, so that my revert can be then applied ?
> 
> You are way too fast, I was still replying to your previous email,
> asking if I could help :)
> 
> I'll a look ASAP. Please, allow for some latency: I'm way slower!

I think the easiest way and the one with less code duplication will
require accessing the tcp_mark_push() and skb_entail() helpers from the
MPTCP code, making them not static and exposing them e.g. in net/tcp.h.
Would that be acceptable or should I look for other options?

Thanks!

Paolo


  parent reply	other threads:[~2021-09-01 16:01 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-01 10:39 [PATCH net-next] tcp: add tcp_tx_skb_cache_key checking in sk_stream_alloc_skb() Yunsheng Lin
2021-09-01 10:52 ` Paolo Abeni
2021-09-01 10:52   ` Paolo Abeni
2021-09-01 15:06   ` Eric Dumazet
2021-09-01 15:06     ` Eric Dumazet
2021-09-01 15:16     ` Eric Dumazet
2021-09-01 15:16       ` Eric Dumazet
2021-09-01 15:25       ` Paolo Abeni
2021-09-01 15:25         ` Paolo Abeni
2021-09-01 15:29         ` Eric Dumazet
2021-09-01 15:29           ` Eric Dumazet
2021-09-01 16:01         ` Paolo Abeni [this message]
2021-09-01 16:01           ` Paolo Abeni
2021-09-01 16:02           ` Eric Dumazet
2021-09-01 16:02             ` Eric Dumazet
2021-09-02  0:47 ` Yunsheng Lin
2021-09-02  1:13   ` Eric Dumazet
2021-09-02  2:05     ` Yunsheng Lin

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=59ad13bb312805bb1d183c5817d5f7b6fd6a90dd.camel@redhat.com \
    --to=pabeni@redhat.com \
    --cc=davem@davemloft.net \
    --cc=dsahern@kernel.org \
    --cc=edumazet@google.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxarm@openeuler.org \
    --cc=linyunsheng@huawei.com \
    --cc=mptcp@lists.linux.dev \
    --cc=netdev@vger.kernel.org \
    --cc=yoshfuji@linux-ipv6.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 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.