[bpf-next,02/19] skmsg: get rid of struct sk_psock_parser

Commit Message

Cong Wang Feb. 3, 2021, 4:16 a.m. UTC

From: Cong Wang <[e-mail]>

struct sk_psock_parser is embedded in sk_psock, it is
unnecessary as skb verdict also uses ->saved_data_ready.
We can simply fold these fields into sk_psock.

Cc: John Fastabend <[e-mail]>
Cc: Daniel Borkmann <[e-mail]>
Cc: Jakub Sitnicki <[e-mail]>
Cc: Lorenz Bauer <[e-mail]>
Signed-off-by: Cong Wang <[e-mail]>
---
 include/linux/skmsg.h | 16 +++++-------
 net/core/skmsg.c      | 58 ++++++++++++++++---------------------------
 net/core/sock_map.c   |  8 +++---
 3 files changed, 31 insertions(+), 51 deletions(-)

Comments

Jakub Sitnicki Feb. 5, 2021, 11:25 a.m. UTC | #1

On Wed, Feb 03, 2021 at 05:16 AM CET, Cong Wang wrote:
> From: Cong Wang <[e-mail]>
>
> struct sk_psock_parser is embedded in sk_psock, it is
> unnecessary as skb verdict also uses ->saved_data_ready.
> We can simply fold these fields into sk_psock.
>
> Cc: John Fastabend <[e-mail]>
> Cc: Daniel Borkmann <[e-mail]>
> Cc: Jakub Sitnicki <[e-mail]>
> Cc: Lorenz Bauer <[e-mail]>
> Signed-off-by: Cong Wang <[e-mail]>
> ---

This one looks like a candidate for splitting out of the series, as it
stands on its own, to make the itself series smaller.

Also, it seems that we always have:

  parser.enabled/bpf_running == (saved_data_ready != NULL)

Maybe parser.enabled can be turned into a predicate function.

[...]

John Fastabend Feb. 8, 2021, 8:39 a.m. UTC | #2

Jakub Sitnicki wrote:
> On Wed, Feb 03, 2021 at 05:16 AM CET, Cong Wang wrote:
> > From: Cong Wang <[e-mail]>
> >
> > struct sk_psock_parser is embedded in sk_psock, it is
> > unnecessary as skb verdict also uses ->saved_data_ready.
> > We can simply fold these fields into sk_psock.
> >
> > Cc: John Fastabend <[e-mail]>
> > Cc: Daniel Borkmann <[e-mail]>
> > Cc: Jakub Sitnicki <[e-mail]>
> > Cc: Lorenz Bauer <[e-mail]>
> > Signed-off-by: Cong Wang <[e-mail]>
> > ---
> 
> This one looks like a candidate for splitting out of the series, as it
> stands on its own, to make the itself series smaller.
> 
> Also, it seems that we always have:
> 
>   parser.enabled/bpf_running == (saved_data_ready != NULL)
> 
> Maybe parser.enabled can be turned into a predicate function.
> 
> [...]

Agree. To speed this along consider breaking it into three
series.

 - couple cleanup things: this patch, config option, etc.

 - udp changes

 - af_unix changes.

Although if you really think udp changes and af_unix need to go
together that is fine imo. I think the basic rule is to try and avoid
getting patch counts above 10 or so if at all possible.

At least this patch, the renaming patch, and the config patch
can get pulled out into their own series so we can get those
merged and out of the way.

Thanks,
John

Cong Wang Feb. 9, 2021, 12:19 a.m. UTC | #3

On Mon, Feb 8, 2021 at 12:39 AM John Fastabend <[e-mail]> wrote:
>
> Jakub Sitnicki wrote:
> > On Wed, Feb 03, 2021 at 05:16 AM CET, Cong Wang wrote:
> > > From: Cong Wang <[e-mail]>
> > >
> > > struct sk_psock_parser is embedded in sk_psock, it is
> > > unnecessary as skb verdict also uses ->saved_data_ready.
> > > We can simply fold these fields into sk_psock.
> > >
> > > Cc: John Fastabend <[e-mail]>
> > > Cc: Daniel Borkmann <[e-mail]>
> > > Cc: Jakub Sitnicki <[e-mail]>
> > > Cc: Lorenz Bauer <[e-mail]>
> > > Signed-off-by: Cong Wang <[e-mail]>
> > > ---
> >
> > This one looks like a candidate for splitting out of the series, as it
> > stands on its own, to make the itself series smaller.
> >
> > Also, it seems that we always have:
> >
> >   parser.enabled/bpf_running == (saved_data_ready != NULL)
> >
> > Maybe parser.enabled can be turned into a predicate function.

Yeah, this looks cleaner.

> >
> > [...]
>
> Agree. To speed this along consider breaking it into three
> series.
>
>  - couple cleanup things: this patch, config option, etc.
>
>  - udp changes
>
>  - af_unix changes.
>
> Although if you really think udp changes and af_unix need to go
> together that is fine imo. I think the basic rule is to try and avoid
> getting patch counts above 10 or so if at all possible.
>
> At least this patch, the renaming patch, and the config patch
> can get pulled out into their own series so we can get those
> merged and out of the way.

Sounds good. Since each patch is already separated, there is no
extra burden on my side to split the whole patchset as suggested
above, except adjusting the cover letters.

Thanks.

Patch

diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 8edbbf5f2f93..56d641df3b0c 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -70,12 +70,6 @@  struct sk_psock_link {
 	void				*link_raw;
 };
 
-struct sk_psock_parser {
-	struct strparser		strp;
-	bool				enabled;
-	void (*saved_data_ready)(struct sock *sk);
-};
-
 struct sk_psock_work_state {
 	struct sk_buff			*skb;
 	u32				len;
@@ -90,7 +84,8 @@  struct sk_psock {
 	u32				eval;
 	struct sk_msg			*cork;
 	struct sk_psock_progs		progs;
-	struct sk_psock_parser		parser;
+	struct strparser		strp;
+	bool				bpf_running;
 	struct sk_buff_head		ingress_skb;
 	struct list_head		ingress_msg;
 	unsigned long			state;
@@ -100,6 +95,7 @@  struct sk_psock {
 	void (*saved_unhash)(struct sock *sk);
 	void (*saved_close)(struct sock *sk, long timeout);
 	void (*saved_write_space)(struct sock *sk);
+	void (*saved_data_ready)(struct sock *sk);
 	struct proto			*sk_proto;
 	struct sk_psock_work_state	work_state;
 	struct work_struct		work;
@@ -400,8 +396,8 @@  static inline void sk_psock_put(struct sock *sk, struct sk_psock *psock)
 
 static inline void sk_psock_data_ready(struct sock *sk, struct sk_psock *psock)
 {
-	if (psock->parser.enabled)
-		psock->parser.saved_data_ready(sk);
+	if (psock->bpf_running)
+		psock->saved_data_ready(sk);
 	else
 		sk->sk_data_ready(sk);
 }
@@ -440,6 +436,6 @@  static inline bool sk_psock_strp_enabled(struct sk_psock *psock)
 {
 	if (!psock)
 		return false;
-	return psock->parser.enabled;
+	return psock->bpf_running;
 }
 #endif /* _LINUX_SKMSG_H */
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 1261512d6807..f72fcb03d25c 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -653,7 +653,7 @@  static void sk_psock_destroy_deferred(struct work_struct *gc)
 
 	/* Parser has been stopped */
 	if (psock->progs.skb_parser)
-		strp_done(&psock->parser.strp);
+		strp_done(&psock->strp);
 
 	cancel_work_sync(&psock->work);
 
@@ -750,14 +750,6 @@  static int sk_psock_bpf_run(struct sk_psock *psock, struct bpf_prog *prog,
 	return bpf_prog_run_pin_on_cpu(prog, skb);
 }
 
-static struct sk_psock *sk_psock_from_strp(struct strparser *strp)
-{
-	struct sk_psock_parser *parser;
-
-	parser = container_of(strp, struct sk_psock_parser, strp);
-	return container_of(parser, struct sk_psock, parser);
-}
-
 static void sk_psock_skb_redirect(struct sk_buff *skb)
 {
 	struct sk_psock *psock_other;
@@ -899,7 +891,7 @@  static int sk_psock_strp_read_done(struct strparser *strp, int err)
 
 static int sk_psock_strp_parse(struct strparser *strp, struct sk_buff *skb)
 {
-	struct sk_psock *psock = sk_psock_from_strp(strp);
+	struct sk_psock *psock = container_of(strp, struct sk_psock, strp);
 	struct bpf_prog *prog;
 	int ret = skb->len;
 
@@ -923,10 +915,10 @@  static void sk_psock_strp_data_ready(struct sock *sk)
 	psock = sk_psock(sk);
 	if (likely(psock)) {
 		if (tls_sw_has_ctx_rx(sk)) {
-			psock->parser.saved_data_ready(sk);
+			psock->saved_data_ready(sk);
 		} else {
 			write_lock_bh(&sk->sk_callback_lock);
-			strp_data_ready(&psock->parser.strp);
+			strp_data_ready(&psock->strp);
 			write_unlock_bh(&sk->sk_callback_lock);
 		}
 	}
@@ -1009,57 +1001,49 @@  int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock)
 		.parse_msg	= sk_psock_strp_parse,
 	};
 
-	psock->parser.enabled = false;
-	return strp_init(&psock->parser.strp, sk, &cb);
+	psock->bpf_running = false;
+	return strp_init(&psock->strp, sk, &cb);
 }
 
 void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock)
 {
-	struct sk_psock_parser *parser = &psock->parser;
-
-	if (parser->enabled)
+	if (psock->bpf_running)
 		return;
 
-	parser->saved_data_ready = sk->sk_data_ready;
+	psock->saved_data_ready = sk->sk_data_ready;
 	sk->sk_data_ready = sk_psock_verdict_data_ready;
 	sk->sk_write_space = sk_psock_write_space;
-	parser->enabled = true;
+	psock->bpf_running = true;
 }
 
 void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock)
 {
-	struct sk_psock_parser *parser = &psock->parser;
-
-	if (parser->enabled)
+	if (psock->bpf_running)
 		return;
 
-	parser->saved_data_ready = sk->sk_data_ready;
+	psock->saved_data_ready = sk->sk_data_ready;
 	sk->sk_data_ready = sk_psock_strp_data_ready;
 	sk->sk_write_space = sk_psock_write_space;
-	parser->enabled = true;
+	psock->bpf_running = true;
 }
 
 void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock)
 {
-	struct sk_psock_parser *parser = &psock->parser;
-
-	if (!parser->enabled)
+	if (!psock->bpf_running)
 		return;
 
-	sk->sk_data_ready = parser->saved_data_ready;
-	parser->saved_data_ready = NULL;
-	strp_stop(&parser->strp);
-	parser->enabled = false;
+	sk->sk_data_ready = psock->saved_data_ready;
+	psock->saved_data_ready = NULL;
+	strp_stop(&psock->strp);
+	psock->bpf_running = false;
 }
 
 void sk_psock_stop_verdict(struct sock *sk, struct sk_psock *psock)
 {
-	struct sk_psock_parser *parser = &psock->parser;
-
-	if (!parser->enabled)
+	if (!psock->bpf_running)
 		return;
 
-	sk->sk_data_ready = parser->saved_data_ready;
-	parser->saved_data_ready = NULL;
-	parser->enabled = false;
+	sk->sk_data_ready = psock->saved_data_ready;
+	psock->saved_data_ready = NULL;
+	psock->bpf_running = false;
 }
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index d758fb83c884..37ff8e13e4cc 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -148,9 +148,9 @@  static void sock_map_del_link(struct sock *sk,
 			struct bpf_map *map = link->map;
 			struct bpf_stab *stab = container_of(map, struct bpf_stab,
 							     map);
-			if (psock->parser.enabled && stab->progs.skb_parser)
+			if (psock->bpf_running && stab->progs.skb_parser)
 				strp_stop = true;
-			if (psock->parser.enabled && stab->progs.skb_verdict)
+			if (psock->bpf_running && stab->progs.skb_verdict)
 				verdict_stop = true;
 			list_del(&link->list);
 			sk_psock_free_link(link);
@@ -283,14 +283,14 @@  static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs,
 		goto out_drop;
 
 	write_lock_bh(&sk->sk_callback_lock);
-	if (skb_parser && skb_verdict && !psock->parser.enabled) {
+	if (skb_parser && skb_verdict && !psock->bpf_running) {
 		ret = sk_psock_init_strp(sk, psock);
 		if (ret)
 			goto out_unlock_drop;
 		psock_set_prog(&psock->progs.skb_verdict, skb_verdict);
 		psock_set_prog(&psock->progs.skb_parser, skb_parser);
 		sk_psock_start_strp(sk, psock);
-	} else if (!skb_parser && skb_verdict && !psock->parser.enabled) {
+	} else if (!skb_parser && skb_verdict && !psock->bpf_running) {
 		psock_set_prog(&psock->progs.skb_verdict, skb_verdict);
 		sk_psock_start_verdict(sk,psock);
 	}