From: Ferenc Fejes <fejes@inf.elte.hu>
To: Liu Jian <liujian56@huawei.com>,
john.fastabend@gmail.com, jakub@cloudflare.com, ast@kernel.org,
daniel@iogearbox.net, andrii@kernel.org, martin.lau@linux.dev,
song@kernel.org, yonghong.song@linux.dev, kpsingh@kernel.org,
sdf@google.com, haoluo@google.com, jolsa@kernel.org,
davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, dsahern@kernel.org
Cc: netdev@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next v2 1/7] bpf, sockmap: add BPF_F_PERMANENTLY flag for skmsg redirect
Date: Thu, 17 Aug 2023 14:05:20 +0200 [thread overview]
Message-ID: <c1ba1a3235464b8306a22c050225332fa3929a10.camel@inf.elte.hu> (raw)
In-Reply-To: <20230811093237.3024459-2-liujian56@huawei.com>
Hi Liu!
On Fri, 2023-08-11 at 17:32 +0800, Liu Jian wrote:
> If the sockmap msg redirection function is used only to forward
> packets
> and no other operation, the execution result of the
> BPF_SK_MSG_VERDICT
> program is the same each time. In this case, the BPF program only
> needs to
> be run once. Add BPF_F_PERMANENTLY flag to bpf_msg_redirect_map() and
> bpf_msg_redirect_hash() to implement this ability.
Did you considered other names for this flag e.g. BPF_F_SPLICED or
BPF_F_PIPED?
BTW good addition, makes sense for the skb case too.
>
> Then we can enable this function in the bpf program as follows:
> bpf_msg_redirect_hash(xx, xx, xx, BPF_F_INGRESS | BPF_F_PERMANENTLY);
>
> Test results using netperf TCP_STREAM mode:
> for i in 1 64 128 512 1k 2k 32k 64k 100k 500k 1m;then
> netperf -T 1,2 -t TCP_STREAM -H 127.0.0.1 -l 20 -- -m $i -s 100m,100m
> -S 100m,100m
> done
>
> before:
> 3.84 246.52 496.89 1885.03 3415.29 6375.03 40749.09 48764.40 51611.34
> 55678.26 55992.78
> after:
> 4.43 279.20 555.82 2080.79 3870.70 7105.44 41836.41 49709.75 51861.56
> 55211.00 54566.85
>
> Signed-off-by: Liu Jian <liujian56@huawei.com>
> ---
> include/linux/skmsg.h | 1 +
> include/uapi/linux/bpf.h | 7 +++++--
> net/core/skmsg.c | 1 +
> net/core/sock_map.c | 4 ++--
> net/ipv4/tcp_bpf.c | 21 +++++++++++++++------
> tools/include/uapi/linux/bpf.h | 7 +++++--
> 6 files changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
> index 054d7911bfc9..b2da9c432f52 100644
> --- a/include/linux/skmsg.h
> +++ b/include/linux/skmsg.h
> @@ -82,6 +82,7 @@ struct sk_psock {
> u32 cork_bytes;
> u32 eval;
> bool redir_ingress; /* undefined
> if sk_redir is null */
> + bool eval_permanently;
> struct sk_msg *cork;
> struct sk_psock_progs progs;
> #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 70da85200695..cf622ea4f018 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3004,7 +3004,8 @@ union bpf_attr {
> * egress interfaces can be used for redirection. The
> * **BPF_F_INGRESS** value in *flags* is used to make
> the
> * distinction (ingress path is selected if the flag is
> present,
> - * egress path otherwise). This is the only flag
> supported for now.
> + * egress path otherwise). The **BPF_F_PERMANENTLY**
> value in
> + * *flags* is used to indicates whether the eBPF result
> is permanent.
> * Return
> * **SK_PASS** on success, or **SK_DROP** on error.
> *
> @@ -3276,7 +3277,8 @@ union bpf_attr {
> * egress interfaces can be used for redirection. The
> * **BPF_F_INGRESS** value in *flags* is used to make
> the
> * distinction (ingress path is selected if the flag is
> present,
> - * egress path otherwise). This is the only flag
> supported for now.
> + * egress path otherwise). The **BPF_F_PERMANENTLY**
> value in
> + * *flags* is used to indicates whether the eBPF result
> is permanent.
> * Return
> * **SK_PASS** on success, or **SK_DROP** on error.
> *
> @@ -5872,6 +5874,7 @@ enum {
> /* BPF_FUNC_clone_redirect and BPF_FUNC_redirect flags. */
> enum {
> BPF_F_INGRESS = (1ULL << 0),
> + BPF_F_PERMANENTLY = (1ULL << 1),
> };
>
> /* BPF_FUNC_skb_set_tunnel_key and BPF_FUNC_skb_get_tunnel_key
> flags. */
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index a29508e1ff35..b2bf9b5c4252 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -875,6 +875,7 @@ int sk_psock_msg_verdict(struct sock *sk, struct
> sk_psock *psock,
> ret = bpf_prog_run_pin_on_cpu(prog, msg);
> ret = sk_psock_map_verd(ret, msg->sk_redir);
> psock->apply_bytes = msg->apply_bytes;
> + psock->eval_permanently = msg->flags & BPF_F_PERMANENTLY;
> if (ret == __SK_REDIRECT) {
> if (psock->sk_redir) {
> sock_put(psock->sk_redir);
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 08ab108206bf..6a0c90be7f4f 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -662,7 +662,7 @@ BPF_CALL_4(bpf_msg_redirect_map, struct sk_msg *,
> msg,
> {
> struct sock *sk;
>
> - if (unlikely(flags & ~(BPF_F_INGRESS)))
> + if (unlikely(flags & ~(BPF_F_INGRESS | BPF_F_PERMANENTLY)))
> return SK_DROP;
>
> sk = __sock_map_lookup_elem(map, key);
> @@ -1261,7 +1261,7 @@ BPF_CALL_4(bpf_msg_redirect_hash, struct sk_msg
> *, msg,
> {
> struct sock *sk;
>
> - if (unlikely(flags & ~(BPF_F_INGRESS)))
> + if (unlikely(flags & ~(BPF_F_INGRESS | BPF_F_PERMANENTLY)))
> return SK_DROP;
>
> sk = __sock_hash_lookup_elem(map, key);
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index 81f0dff69e0b..36cf2b0fa6f8 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -419,8 +419,10 @@ static int tcp_bpf_send_verdict(struct sock *sk,
> struct sk_psock *psock,
> if (!psock->apply_bytes) {
> /* Clean up before releasing the sock lock.
> */
> eval = psock->eval;
> - psock->eval = __SK_NONE;
> - psock->sk_redir = NULL;
> + if (!psock->eval_permanently) {
> + psock->eval = __SK_NONE;
> + psock->sk_redir = NULL;
> + }
> }
> if (psock->cork) {
> cork = true;
> @@ -433,9 +435,15 @@ static int tcp_bpf_send_verdict(struct sock *sk,
> struct sk_psock *psock,
> ret = tcp_bpf_sendmsg_redir(sk_redir, redir_ingress,
> msg, tosend, flags);
> sent = origsize - msg->sg.size;
> + /* disable the ability when something wrong */
> + if (unlikely(ret < 0))
> + psock->eval_permanently = 0;
>
> - if (eval == __SK_REDIRECT)
> + if (!psock->eval_permanently && eval ==
> __SK_REDIRECT) {
> sock_put(sk_redir);
> + psock->sk_redir = NULL;
> + psock->eval = __SK_NONE;
> + }
>
> lock_sock(sk);
> if (unlikely(ret < 0)) {
> @@ -460,8 +468,8 @@ static int tcp_bpf_send_verdict(struct sock *sk,
> struct sk_psock *psock,
> }
>
> if (likely(!ret)) {
> - if (!psock->apply_bytes) {
> - psock->eval = __SK_NONE;
> + if (!psock->apply_bytes && !psock->eval_permanently)
> {
> + psock->eval = __SK_NONE;
> if (psock->sk_redir) {
> sock_put(psock->sk_redir);
> psock->sk_redir = NULL;
> @@ -540,7 +548,8 @@ static int tcp_bpf_sendmsg(struct sock *sk,
> struct msghdr *msg, size_t size)
> if (psock->cork_bytes && !enospc)
> goto out_err;
> /* All cork bytes are accounted, rerun the
> prog. */
> - psock->eval = __SK_NONE;
> + if (!psock->eval_permanently)
> + psock->eval = __SK_NONE;
> psock->cork_bytes = 0;
> }
>
> diff --git a/tools/include/uapi/linux/bpf.h
> b/tools/include/uapi/linux/bpf.h
> index 70da85200695..cf622ea4f018 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -3004,7 +3004,8 @@ union bpf_attr {
> * egress interfaces can be used for redirection. The
> * **BPF_F_INGRESS** value in *flags* is used to make
> the
> * distinction (ingress path is selected if the flag is
> present,
> - * egress path otherwise). This is the only flag
> supported for now.
> + * egress path otherwise). The **BPF_F_PERMANENTLY**
> value in
> + * *flags* is used to indicates whether the eBPF result
> is permanent.
> * Return
> * **SK_PASS** on success, or **SK_DROP** on error.
> *
> @@ -3276,7 +3277,8 @@ union bpf_attr {
> * egress interfaces can be used for redirection. The
> * **BPF_F_INGRESS** value in *flags* is used to make
> the
> * distinction (ingress path is selected if the flag is
> present,
> - * egress path otherwise). This is the only flag
> supported for now.
> + * egress path otherwise). The **BPF_F_PERMANENTLY**
> value in
> + * *flags* is used to indicates whether the eBPF result
> is permanent.
> * Return
> * **SK_PASS** on success, or **SK_DROP** on error.
> *
> @@ -5872,6 +5874,7 @@ enum {
> /* BPF_FUNC_clone_redirect and BPF_FUNC_redirect flags. */
> enum {
> BPF_F_INGRESS = (1ULL << 0),
> + BPF_F_PERMANENTLY = (1ULL << 1),
> };
>
> /* BPF_FUNC_skb_set_tunnel_key and BPF_FUNC_skb_get_tunnel_key
> flags. */
Ferenc
next prev parent reply other threads:[~2023-08-17 12:06 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-11 9:32 [PATCH bpf-next v2 0/7] add BPF_F_PERMANENTLY flag for sockmap skmsg redirect Liu Jian
2023-08-11 9:32 ` [PATCH bpf-next v2 1/7] bpf, sockmap: add BPF_F_PERMANENTLY flag for " Liu Jian
2023-08-17 6:13 ` John Fastabend
2023-08-19 9:25 ` liujian (CE)
2023-08-20 18:03 ` Jakub Sitnicki
2023-08-17 12:05 ` Ferenc Fejes [this message]
2023-08-19 9:32 ` liujian (CE)
2023-08-20 18:19 ` Jakub Sitnicki
2023-08-21 7:40 ` Jakub Sitnicki
2023-08-11 9:32 ` [PATCH bpf-next v2 2/7] selftests/bpf: Add txmsg ingress permanently test for sockmap Liu Jian
2023-08-11 9:32 ` [PATCH bpf-next v2 3/7] selftests/bpf: Add txmsg redir " Liu Jian
2023-08-11 9:32 ` [PATCH bpf-next v2 4/7] selftests/bpf: add skmsg verdict tests Liu Jian
2023-08-11 9:32 ` [PATCH bpf-next v2 5/7] selftests/bpf: add two skmsg verdict tests for BPF_F_PERMANENTLY flag Liu Jian
2023-08-11 9:32 ` [PATCH bpf-next v2 6/7] selftests/bpf: add tests for verdict skmsg to itself Liu Jian
2023-08-11 9:32 ` [PATCH bpf-next v2 7/7] selftests/bpf: add tests for verdict skmsg to closed socket Liu Jian
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=c1ba1a3235464b8306a22c050225332fa3929a10.camel@inf.elte.hu \
--to=fejes@inf.elte.hu \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=dsahern@kernel.org \
--cc=edumazet@google.com \
--cc=haoluo@google.com \
--cc=jakub@cloudflare.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=liujian56@huawei.com \
--cc=martin.lau@linux.dev \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=sdf@google.com \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/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).