bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrey Ignatov <rdna@fb.com>
To: <sdf@google.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Network Development <netdev@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>
Subject: Re: [PATCH bpf-next 2/3] bpf: allow bpf_{s,g}etsockopt from cgroup bind{4,6} hooks
Date: Mon, 30 Nov 2020 15:02:42 -0800	[thread overview]
Message-ID: <20201130230242.GA73546@rdna-mbp> (raw)
In-Reply-To: <20201130163813.GA553169@google.com>

sdf@google.com <sdf@google.com> [Mon, 2020-11-30 08:38 -0800]:
> On 11/29, Andrey Ignatov wrote:
> > Alexei Starovoitov <alexei.starovoitov@gmail.com> [Tue, 2020-11-17 20:05
> > -0800]:
> > > On Tue, Nov 17, 2020 at 4:17 PM Stanislav Fomichev <sdf@google.com>
> > wrote:
> [..]
> > >
> > > I think it is ok, but I need to go through the locking paths more.
> > > Andrey,
> > > please take a look as well.
> > Sorry for delay, I was offline for the last two weeks.
> No worries, I was OOO myself last week, thanks for the feedback!
> >  From the correctness perspective it looks fine to me.
> >  From the performance perspective I can think of one relevant scenario.
> > Quite common use-case in applications is to use bind(2) not before
> > listen(2) but before connect(2) for client sockets so that connection
> > can be set up from specific source IP and, optionally, port.
> > Binding to both IP and port case is not interesting since it's already
> > slow due to get_port().
> > But some applications do care about connection setup performance and at
> > the same time need to set source IP only (no port). In this case they
> > use IP_BIND_ADDRESS_NO_PORT socket option, what makes bind(2) fast
> > (we've discussed it with Stanislav earlier in [0]).
> > I can imagine some pathological case when an application sets up tons of
> > connections with bind(2) before connect(2) for sockets with
> > IP_BIND_ADDRESS_NO_PORT enabled (that by itself requires setsockopt(2)
> > though, i.e. socket lock/unlock) and that another lock/unlock to run
> > bind hook may add some overhead. Though I do not know how critical that
> > overhead may be and whether it's worth to benchmark or not (maybe too
> > much paranoia).
> > [0] https://lore.kernel.org/bpf/20200505182010.GB55644@rdna-mbp/
> Even in case of IP_BIND_ADDRESS_NO_PORT, inet[6]_bind() does
> lock_sock down the line, so it's not like we are switching
> a lockless path to the one with the lock, right?

Right, I understand that it's going from one lock/unlock to two (not
from zero to one), that's what I meant by "another". My point was about
this one more lock.

> And in this case, similar to listen, the socket is still uncontended and
> owned by the userspace. So that extra lock/unlock should be cheap
> enough to be ignored (spin_lock_bh on the warm cache line).
> Am I missing something?

As I mentioned it may come up only in "pathological case" what is
probably fine to ignore, i.e. I'd rather agree with "cheap enough to be
ignored" and benchmark would likely confirm it, I just couldn't say that
for sure w/o numbers so brought this point.

Given that we both agree that it should be fine to ignore this +1 lock,
IMO it should be good to go unless someone else has objections.

Andrey Ignatov

  reply	other threads:[~2020-11-30 23:04 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-18  0:17 [PATCH bpf-next 0/3] bpf: expose bpf_{s,g}etsockopt helpers to bind{4,6} hooks Stanislav Fomichev
2020-11-18  0:17 ` [PATCH bpf-next 1/3] selftests/bpf: rewrite test_sock_addr bind bpf into C Stanislav Fomichev
2020-12-02  0:26   ` Andrii Nakryiko
2020-12-02 17:04     ` sdf
2020-11-18  0:17 ` [PATCH bpf-next 2/3] bpf: allow bpf_{s,g}etsockopt from cgroup bind{4,6} hooks Stanislav Fomichev
2020-11-18  4:05   ` Alexei Starovoitov
2020-11-30  1:05     ` Andrey Ignatov
2020-11-30 16:38       ` sdf
2020-11-30 23:02         ` Andrey Ignatov [this message]
2020-12-01 18:43           ` sdf
2020-12-01 19:22             ` Andrey Ignatov
2020-12-01 19:21   ` Andrey Ignatov
2020-11-18  0:17 ` [PATCH bpf-next 3/3] selftests/bpf: extend bind{4,6} programs with a call to bpf_setsockopt Stanislav Fomichev
2020-12-02  0:22   ` Andrii Nakryiko
2020-12-02 17:25 [PATCH bpf-next 0/3] bpf: expose bpf_{s,g}etsockopt helpers to bind{4,6} hooks Stanislav Fomichev
2020-12-02 17:25 ` [PATCH bpf-next 2/3] bpf: allow bpf_{s,g}etsockopt from cgroup " Stanislav Fomichev

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:

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

  git send-email \
    --in-reply-to=20201130230242.GA73546@rdna-mbp \
    --to=rdna@fb.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=netdev@vger.kernel.org \
    --cc=sdf@google.com \


* 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).