1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next v4 0/7] BPF path manager, part 3
  4. View patch

[PATCH mptcp-next v4 1/7] mptcp: add mptcp_pm_genl_fill_addr helper

Geliang Tang posted 7 patches 1 year ago
There is a newer version of this series
[PATCH mptcp-next v4 1/7] mptcp: add mptcp_pm_genl_fill_addr helper
Posted by Geliang Tang 1 year ago 
From: Geliang Tang <tanggeliang@kylinos.cn>

To save some redundant code in dump_addr() interfaces of both the
netlink PM and userspace PM, the code that calls netlink message
helpers (genlmsg_put/cancel/end) and mptcp_nl_fill_addr() is wrapped
into a new helper mptcp_pm_genl_fill_addr().

This helper will also be used in BPF path managers.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c           | 21 +++++++++++++++++++++
 net/mptcp/pm_netlink.c   | 12 +-----------
 net/mptcp/pm_userspace.c | 12 +-----------
 net/mptcp/protocol.h     |  3 +++
 4 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index b1f36dc1a091..16cacce6c10f 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -489,6 +489,27 @@ int mptcp_pm_nl_get_addr_doit(struct sk_buff *skb, struct genl_info *info)
 	return ret;
 }
 
+int mptcp_pm_genl_fill_addr(struct sk_buff *msg,
+			    struct netlink_callback *cb,
+			    struct mptcp_pm_addr_entry *entry)
+{
+	void *hdr;
+
+	hdr = genlmsg_put(msg, NETLINK_CB(cb->skb).portid,
+			  cb->nlh->nlmsg_seq, &mptcp_genl_family,
+			  NLM_F_MULTI, MPTCP_PM_CMD_GET_ADDR);
+	if (!hdr)
+		return -EINVAL;
+
+	if (mptcp_nl_fill_addr(msg, entry) < 0) {
+		genlmsg_cancel(msg, hdr);
+		return -EINVAL;
+	}
+
+	genlmsg_end(msg, hdr);
+	return 0;
+}
+
 static int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb)
 {
 	const struct genl_info *info = genl_info_dump(cb);
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index fef01692eaed..afd517ff260c 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1798,7 +1798,6 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
 	int id = cb->args[0];
-	void *hdr;
 	int i;
 
 	pernet = pm_nl_get_pernet(net);
@@ -1813,19 +1812,10 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 			if (entry->addr.id <= id)
 				continue;
 
-			hdr = genlmsg_put(msg, NETLINK_CB(cb->skb).portid,
-					  cb->nlh->nlmsg_seq, &mptcp_genl_family,
-					  NLM_F_MULTI, MPTCP_PM_CMD_GET_ADDR);
-			if (!hdr)
+			if (mptcp_pm_genl_fill_addr(msg, cb, entry))
 				break;
 
-			if (mptcp_nl_fill_addr(msg, entry) < 0) {
-				genlmsg_cancel(msg, hdr);
-				break;
-			}
-
 			id = entry->addr.id;
-			genlmsg_end(msg, hdr);
 		}
 	}
 	rcu_read_unlock();
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 277cf092a870..b50462b527bd 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -641,7 +641,6 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 	struct mptcp_sock *msk;
 	int ret = -EINVAL;
 	struct sock *sk;
-	void *hdr;
 
 	bitmap = (struct id_bitmap *)cb->ctx;
 
@@ -657,19 +656,10 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 		if (test_bit(entry->addr.id, bitmap->map))
 			continue;
 
-		hdr = genlmsg_put(msg, NETLINK_CB(cb->skb).portid,
-				  cb->nlh->nlmsg_seq, &mptcp_genl_family,
-				  NLM_F_MULTI, MPTCP_PM_CMD_GET_ADDR);
-		if (!hdr)
+		if (mptcp_pm_genl_fill_addr(msg, cb, entry))
 			break;
 
-		if (mptcp_nl_fill_addr(msg, entry) < 0) {
-			genlmsg_cancel(msg, hdr);
-			break;
-		}
-
 		__set_bit(entry->addr.id, bitmap->map);
-		genlmsg_end(msg, hdr);
 	}
 	spin_unlock_bh(&msk->pm.lock);
 	release_sock(sk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 6fb536de9981..20941405a1ea 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1060,6 +1060,9 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
 					      struct request_sock *req);
 int mptcp_nl_fill_addr(struct sk_buff *skb,
 		       struct mptcp_pm_addr_entry *entry);
+int mptcp_pm_genl_fill_addr(struct sk_buff *msg,
+			    struct netlink_callback *cb,
+			    struct mptcp_pm_addr_entry *entry);
 
 static inline bool mptcp_pm_should_add_signal(struct mptcp_sock *msk)
 {
-- 
2.43.0
Re: [PATCH mptcp-next v4 1/7] mptcp: add mptcp_pm_genl_fill_addr helper
Posted by Matthieu Baerts 1 year ago 
Hi Geliang,

On 16/01/2025 08:26, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> To save some redundant code in dump_addr() interfaces of both the
> netlink PM and userspace PM, the code that calls netlink message
> helpers (genlmsg_put/cancel/end) and mptcp_nl_fill_addr() is wrapped
> into a new helper mptcp_pm_genl_fill_addr().

I'm OK with the suggested modification here below (just one small
detail, please see below), but ...

> This helper will also be used in BPF path managers.

... that doesn't sound right: I don't see why a BPF PM has to dump
addresses via Netlink. In your cover-letter, you mentioned the
dump_addr() will not be implemented in the BPF PM, so I guess you left
this line by accident, right?

If not: with the in-kernel PM, it is needed to dump the endpoints to
know what has been set. With the userspace PM, the dump is useful for a
userspace PM daemon to recover from a restart. For these two PM, Netlink
is used between the userspace and the kernelspace, so that makes sense
to continue to do so. With the BPF PM, I don't really see the point: all
the communication (configuration, status, decisions, etc.) should be
done via BPF, no?

(...)

> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index fef01692eaed..afd517ff260c 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1798,7 +1798,6 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
>  	struct mptcp_pm_addr_entry *entry;
>  	struct pm_nl_pernet *pernet;
>  	int id = cb->args[0];
> -	void *hdr;
>  	int i;
>  
>  	pernet = pm_nl_get_pernet(net);
> @@ -1813,19 +1812,10 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
>  			if (entry->addr.id <= id)
>  				continue;
>  
> -			hdr = genlmsg_put(msg, NETLINK_CB(cb->skb).portid,
> -					  cb->nlh->nlmsg_seq, &mptcp_genl_family,
> -					  NLM_F_MULTI, MPTCP_PM_CMD_GET_ADDR);
> -			if (!hdr)
> +			if (mptcp_pm_genl_fill_addr(msg, cb, entry))

Even if technically, it is correct, it feels "strange" to see:

  if (foo())
      /* error path */

It feels a bit like having:

  if (success())
      /* error */

If the goal is to stop in case of error, better being explicit about that:

  if (foo() < 0)
      /* error path */

>  				break;
>  
> -			if (mptcp_nl_fill_addr(msg, entry) < 0) {
> -				genlmsg_cancel(msg, hdr);
> -				break;
> -			}
> -
>  			id = entry->addr.id;
> -			genlmsg_end(msg, hdr);
>  		}
>  	}
>  	rcu_read_unlock();
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 277cf092a870..b50462b527bd 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -641,7 +641,6 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
>  	struct mptcp_sock *msk;
>  	int ret = -EINVAL;
>  	struct sock *sk;
> -	void *hdr;
>  
>  	bitmap = (struct id_bitmap *)cb->ctx;
>  
> @@ -657,19 +656,10 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
>  		if (test_bit(entry->addr.id, bitmap->map))
>  			continue;
>  
> -		hdr = genlmsg_put(msg, NETLINK_CB(cb->skb).portid,
> -				  cb->nlh->nlmsg_seq, &mptcp_genl_family,
> -				  NLM_F_MULTI, MPTCP_PM_CMD_GET_ADDR);
> -		if (!hdr)
> +		if (mptcp_pm_genl_fill_addr(msg, cb, entry))

Same here: < 0

>  			break;
>  
> -		if (mptcp_nl_fill_addr(msg, entry) < 0) {
> -			genlmsg_cancel(msg, hdr);
> -			break;
> -		}
> -
>  		__set_bit(entry->addr.id, bitmap->map);
> -		genlmsg_end(msg, hdr);
>  	}
>  	spin_unlock_bh(&msk->pm.lock);
>  	release_sock(sk);

(...)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.