[PATCH mptcp-next v2 0/7] some cleanups

[PATCH mptcp-next v2 0/7] some cleanups

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v2:
 - address Matt's comments in v1

Geliang Tang (7):
  mptcp: make pm_remove_addrs_and_subflows static
  mptcp: drop duplicate header inclusions
  mptcp: update set_flags interfaces
  mptcp: set error messages for set_flags
  mptcp: drop lookup_by_id in lookup_addr
  mptcp: add check_id for lookup_anno_list_by_saddr
  selftests: mptcp: flush userspace addrs list

 net/mptcp/diag.c                              |  1 -
 net/mptcp/mptcp_diag.c                        |  1 -
 net/mptcp/pm.c                                | 13 +--
 net/mptcp/pm_netlink.c                        | 92 +++++++++----------
 net/mptcp/pm_userspace.c                      | 41 +++++++--
 net/mptcp/protocol.c                          |  1 -
 net/mptcp/protocol.h                          | 22 +++--
 net/mptcp/subflow.c                           |  2 -
 .../testing/selftests/net/mptcp/mptcp_join.sh | 41 +++++++--
 9 files changed, 124 insertions(+), 90 deletions(-)

-- 
2.40.1

[PATCH mptcp-next v2 1/7] mptcp: make pm_remove_addrs_and_subflows static

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

mptcp_pm_remove_addrs_and_subflows() is only used in pm_netlink.c, it's
no longer used in pm_userspace.c any more since the commit 8b1c94da1e48
("mptcp: only send RM_ADDR in nl_cmd_remove"). So this patch changes it
to a static function.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_netlink.c | 4 ++--
 net/mptcp/protocol.h   | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index f04e354b0c64..16f8bd47f4b8 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1547,8 +1547,8 @@ void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list)
 	}
 }
 
-void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
-					struct list_head *rm_list)
+static void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
+					       struct list_head *rm_list)
 {
 	struct mptcp_rm_list alist = { .nr = 0 }, slist = { .nr = 0 };
 	struct mptcp_pm_addr_entry *entry;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index d611968ae6a4..746d0d1f94ec 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -988,8 +988,6 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
 int mptcp_pm_remove_subflow(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
 void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list);
-void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
-					struct list_head *rm_list);
 
 void mptcp_free_local_addr_list(struct mptcp_sock *msk);
 
-- 
2.40.1

[PATCH mptcp-next v2 2/7] mptcp: drop duplicate header inclusions

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The headers net/tcp.h, net/genetlink.h and uapi/linux/mptcp.h are included
in protocol.h already, no need to include them again directly. This patch
removes these duplicate header inclusions.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/diag.c       | 1 -
 net/mptcp/mptcp_diag.c | 1 -
 net/mptcp/pm.c         | 1 -
 net/mptcp/pm_netlink.c | 3 ---
 net/mptcp/protocol.c   | 1 -
 net/mptcp/subflow.c    | 2 --
 6 files changed, 9 deletions(-)

diff --git a/net/mptcp/diag.c b/net/mptcp/diag.c
index 6ff6f14674aa..aefe26e5ae72 100644
--- a/net/mptcp/diag.c
+++ b/net/mptcp/diag.c
@@ -10,7 +10,6 @@
 #include <linux/net.h>
 #include <linux/inet_diag.h>
 #include <net/netlink.h>
-#include <uapi/linux/mptcp.h>
 #include "protocol.h"
 
 static int subflow_get_info(struct sock *sk, struct sk_buff *skb)
diff --git a/net/mptcp/mptcp_diag.c b/net/mptcp/mptcp_diag.c
index bd8ff5950c8d..0566dd793810 100644
--- a/net/mptcp/mptcp_diag.c
+++ b/net/mptcp/mptcp_diag.c
@@ -10,7 +10,6 @@
 #include <linux/net.h>
 #include <linux/inet_diag.h>
 #include <net/netlink.h>
-#include <uapi/linux/mptcp.h>
 #include "protocol.h"
 
 static int sk_diag_dump(struct sock *sk, struct sk_buff *skb,
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index b4bdd92a5648..28e5d514bf20 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -6,7 +6,6 @@
 #define pr_fmt(fmt) "MPTCP: " fmt
 
 #include <linux/kernel.h>
-#include <net/tcp.h>
 #include <net/mptcp.h>
 #include "protocol.h"
 
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 16f8bd47f4b8..a900df9f173d 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -8,12 +8,9 @@
 
 #include <linux/inet.h>
 #include <linux/kernel.h>
-#include <net/tcp.h>
 #include <net/inet_common.h>
 #include <net/netns/generic.h>
 #include <net/mptcp.h>
-#include <net/genetlink.h>
-#include <uapi/linux/mptcp.h>
 
 #include "protocol.h"
 #include "mib.h"
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 50dcba41b6ef..b2c4eecf86c4 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -15,7 +15,6 @@
 #include <net/inet_common.h>
 #include <net/inet_hashtables.h>
 #include <net/protocol.h>
-#include <net/tcp.h>
 #include <net/tcp_states.h>
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
 #include <net/transp_v6.h>
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 6403c56f2902..1626dd20c68f 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -15,13 +15,11 @@
 #include <net/inet_common.h>
 #include <net/inet_hashtables.h>
 #include <net/protocol.h>
-#include <net/tcp.h>
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
 #include <net/ip6_route.h>
 #include <net/transp_v6.h>
 #endif
 #include <net/mptcp.h>
-#include <uapi/linux/mptcp.h>
 #include "protocol.h"
 #include "mib.h"
 
-- 
2.40.1

[PATCH mptcp-next v2 3/7] mptcp: update set_flags interfaces

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch updates set_flags interfaces, make it more similar to the
interfaces of dump_addr and get_addr:

 mptcp_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c           | 10 +++----
 net/mptcp/pm_netlink.c   | 58 +++++++++++++++++-----------------------
 net/mptcp/pm_userspace.c | 32 +++++++++++++++++-----
 net/mptcp/protocol.h     | 10 +++----
 4 files changed, 58 insertions(+), 52 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 28e5d514bf20..55406720c607 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -456,13 +456,11 @@ int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb)
 	return mptcp_pm_nl_dump_addr(msg, cb);
 }
 
-int mptcp_pm_set_flags(struct net *net, struct nlattr *token,
-		       struct mptcp_pm_addr_entry *loc,
-		       struct mptcp_pm_addr_entry *rem, u8 bkup)
+int mptcp_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 {
-	if (token)
-		return mptcp_userspace_pm_set_flags(net, token, loc, rem, bkup);
-	return mptcp_pm_nl_set_flags(net, loc, bkup);
+	if (info->attrs[MPTCP_PM_ATTR_TOKEN])
+		return mptcp_userspace_pm_set_flags(skb, info);
+	return mptcp_pm_nl_set_flags(skb, info);
 }
 
 void mptcp_pm_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index a900df9f173d..c799fe84dfd3 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1887,66 +1887,58 @@ static int mptcp_nl_set_flags(struct net *net,
 	return ret;
 }
 
-int mptcp_pm_nl_set_flags(struct net *net, struct mptcp_pm_addr_entry *addr, u8 bkup)
+int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 {
-	struct pm_nl_pernet *pernet = pm_nl_get_pernet(net);
+	struct mptcp_pm_addr_entry addr = { .addr = { .family = AF_UNSPEC }, };
+	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
 	u8 changed, mask = MPTCP_PM_ADDR_FLAG_BACKUP |
 			   MPTCP_PM_ADDR_FLAG_FULLMESH;
+	struct net *net = sock_net(skb->sk);
 	struct mptcp_pm_addr_entry *entry;
+	struct pm_nl_pernet *pernet;
 	u8 lookup_by_id = 0;
+	u8 bkup = 0;
+	int ret;
+
+	pernet = pm_nl_get_pernet(net);
+
+	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
+	if (ret < 0)
+		return ret;
 
-	if (addr->addr.family == AF_UNSPEC) {
+	if (addr.addr.family == AF_UNSPEC) {
 		lookup_by_id = 1;
-		if (!addr->addr.id)
+		if (!addr.addr.id)
 			return -EOPNOTSUPP;
 	}
 
+	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
+		bkup = 1;
+
 	spin_lock_bh(&pernet->lock);
-	entry = __lookup_addr(pernet, &addr->addr, lookup_by_id);
+	entry = __lookup_addr(pernet, &addr.addr, lookup_by_id);
 	if (!entry) {
 		spin_unlock_bh(&pernet->lock);
 		return -EINVAL;
 	}
-	if ((addr->flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
+	if ((addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
 	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
 		spin_unlock_bh(&pernet->lock);
 		return -EINVAL;
 	}
 
-	changed = (addr->flags ^ entry->flags) & mask;
-	entry->flags = (entry->flags & ~mask) | (addr->flags & mask);
-	*addr = *entry;
+	changed = (addr.flags ^ entry->flags) & mask;
+	entry->flags = (entry->flags & ~mask) | (addr.flags & mask);
+	addr = *entry;
 	spin_unlock_bh(&pernet->lock);
 
-	mptcp_nl_set_flags(net, &addr->addr, bkup, changed);
+	mptcp_nl_set_flags(net, &addr.addr, bkup, changed);
 	return 0;
 }
 
 int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	struct mptcp_pm_addr_entry remote = { .addr = { .family = AF_UNSPEC }, };
-	struct mptcp_pm_addr_entry addr = { .addr = { .family = AF_UNSPEC }, };
-	struct nlattr *attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
-	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
-	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
-	struct net *net = sock_net(skb->sk);
-	u8 bkup = 0;
-	int ret;
-
-	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
-	if (ret < 0)
-		return ret;
-
-	if (attr_rem) {
-		ret = mptcp_pm_parse_entry(attr_rem, info, false, &remote);
-		if (ret < 0)
-			return ret;
-	}
-
-	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
-		bkup = 1;
-
-	return mptcp_pm_set_flags(net, token, &addr, &remote, bkup);
+	return mptcp_pm_set_flags(skb, info);
 }
 
 static void mptcp_nl_mcast_send(struct net *net, struct sk_buff *nlskb, gfp_t gfp)
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index b9809d988693..7ef3b69852f0 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -546,14 +546,19 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 	return err;
 }
 
-int mptcp_userspace_pm_set_flags(struct net *net, struct nlattr *token,
-				 struct mptcp_pm_addr_entry *loc,
-				 struct mptcp_pm_addr_entry *rem, u8 bkup)
+int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 {
+	struct mptcp_pm_addr_entry loc = { .addr = { .family = AF_UNSPEC }, };
+	struct mptcp_pm_addr_entry rem = { .addr = { .family = AF_UNSPEC }, };
+	struct nlattr *attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
+	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
+	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
+	struct net *net = sock_net(skb->sk);
 	struct mptcp_sock *msk;
 	int ret = -EINVAL;
 	struct sock *sk;
 	u32 token_val;
+	u8 bkup = 0;
 
 	token_val = nla_get_u32(token);
 
@@ -566,12 +571,27 @@ int mptcp_userspace_pm_set_flags(struct net *net, struct nlattr *token,
 	if (!mptcp_pm_is_userspace(msk))
 		goto set_flags_err;
 
-	if (loc->addr.family == AF_UNSPEC ||
-	    rem->addr.family == AF_UNSPEC)
+	ret = mptcp_pm_parse_entry(attr, info, false, &loc);
+	if (ret < 0)
+		goto set_flags_err;
+
+	if (attr_rem) {
+		ret = mptcp_pm_parse_entry(attr_rem, info, false, &rem);
+		if (ret < 0)
+			goto set_flags_err;
+	}
+
+	if (loc.addr.family == AF_UNSPEC ||
+	    rem.addr.family == AF_UNSPEC) {
+		ret = -EINVAL;
 		goto set_flags_err;
+	}
+
+	if (loc.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
+		bkup = 1;
 
 	lock_sock(sk);
-	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc->addr, &rem->addr, bkup);
+	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc.addr, &rem.addr, bkup);
 	release_sock(sk);
 
 set_flags_err:
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 746d0d1f94ec..7905783c95e4 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -975,13 +975,9 @@ int mptcp_pm_nl_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int
 int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
 						   unsigned int id,
 						   u8 *flags, int *ifindex);
-int mptcp_pm_set_flags(struct net *net, struct nlattr *token,
-		       struct mptcp_pm_addr_entry *loc,
-		       struct mptcp_pm_addr_entry *rem, u8 bkup);
-int mptcp_pm_nl_set_flags(struct net *net, struct mptcp_pm_addr_entry *addr, u8 bkup);
-int mptcp_userspace_pm_set_flags(struct net *net, struct nlattr *token,
-				 struct mptcp_pm_addr_entry *loc,
-				 struct mptcp_pm_addr_entry *rem, u8 bkup);
+int mptcp_pm_set_flags(struct sk_buff *skb, struct genl_info *info);
+int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info);
+int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info);
 int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 			   const struct mptcp_addr_info *addr,
 			   bool echo);
-- 
2.40.1

[PATCH mptcp-next v2 4/7] mptcp: set error messages for set_flags

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

In addition to returning the error value, this patch also sets an error
messages with GENL_SET_ERR_MSG or NL_SET_ERR_MSG_ATTR both for pm_netlink.c
and pm_userspace.c. It will help the userspace to identify the issue.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_netlink.c   | 6 +++++-
 net/mptcp/pm_userspace.c | 9 +++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index c799fe84dfd3..354083b8386f 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1908,8 +1908,10 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 
 	if (addr.addr.family == AF_UNSPEC) {
 		lookup_by_id = 1;
-		if (!addr.addr.id)
+		if (!addr.addr.id) {
+			GENL_SET_ERR_MSG(info, "missing required inputs");
 			return -EOPNOTSUPP;
+		}
 	}
 
 	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
@@ -1919,11 +1921,13 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 	entry = __lookup_addr(pernet, &addr.addr, lookup_by_id);
 	if (!entry) {
 		spin_unlock_bh(&pernet->lock);
+		GENL_SET_ERR_MSG(info, "address not found");
 		return -EINVAL;
 	}
 	if ((addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
 	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
 		spin_unlock_bh(&pernet->lock);
+		GENL_SET_ERR_MSG(info, "invalid addr flags");
 		return -EINVAL;
 	}
 
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 7ef3b69852f0..9f5d422d5ef6 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -563,13 +563,17 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 	token_val = nla_get_u32(token);
 
 	msk = mptcp_token_get_sock(net, token_val);
-	if (!msk)
+	if (!msk) {
+		NL_SET_ERR_MSG_ATTR(info->extack, token, "invalid token");
 		return ret;
+	}
 
 	sk = (struct sock *)msk;
 
-	if (!mptcp_pm_is_userspace(msk))
+	if (!mptcp_pm_is_userspace(msk)) {
+		GENL_SET_ERR_MSG(info, "userspace PM not selected");
 		goto set_flags_err;
+	}
 
 	ret = mptcp_pm_parse_entry(attr, info, false, &loc);
 	if (ret < 0)
@@ -583,6 +587,7 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 
 	if (loc.addr.family == AF_UNSPEC ||
 	    rem.addr.family == AF_UNSPEC) {
+		GENL_SET_ERR_MSG(info, "invalid address families");
 		ret = -EINVAL;
 		goto set_flags_err;
 	}
-- 
2.40.1

[PATCH mptcp-next v2 5/7] mptcp: drop lookup_by_id in lookup_addr

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

When the lookup_by_id parameter of __lookup_addr() is true, it's the same
as __lookup_addr_by_id(), it can be replaced by __lookup_addr_by_id()
directly. So drop this parameter, let __lookup_addr() only looks up address
on the local address list by comparing addresses in it, not address ids.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_netlink.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 354083b8386f..5c17d39146ea 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -499,15 +499,12 @@ __lookup_addr_by_id(struct pm_nl_pernet *pernet, unsigned int id)
 }
 
 static struct mptcp_pm_addr_entry *
-__lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info,
-	      bool lookup_by_id)
+__lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
 {
 	struct mptcp_pm_addr_entry *entry;
 
 	list_for_each_entry(entry, &pernet->local_addr_list, list) {
-		if ((!lookup_by_id &&
-		     mptcp_addresses_equal(&entry->addr, info, entry->addr.port)) ||
-		    (lookup_by_id && entry->addr.id == info->id))
+		if (mptcp_addresses_equal(&entry->addr, info, entry->addr.port))
 			return entry;
 	}
 	return NULL;
@@ -537,7 +534,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 
 		mptcp_local_address((struct sock_common *)msk->first, &mpc_addr);
 		rcu_read_lock();
-		entry = __lookup_addr(pernet, &mpc_addr, false);
+		entry = __lookup_addr(pernet, &mpc_addr);
 		if (entry) {
 			__clear_bit(entry->addr.id, msk->pm.id_avail_bitmap);
 			msk->mpc_endpoint_id = entry->addr.id;
@@ -1918,7 +1915,8 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 		bkup = 1;
 
 	spin_lock_bh(&pernet->lock);
-	entry = __lookup_addr(pernet, &addr.addr, lookup_by_id);
+	entry = lookup_by_id ? __lookup_addr_by_id(pernet, addr.addr.id) :
+			       __lookup_addr(pernet, &addr.addr);
 	if (!entry) {
 		spin_unlock_bh(&pernet->lock);
 		GENL_SET_ERR_MSG(info, "address not found");
-- 
2.40.1

[PATCH mptcp-next v2 6/7] mptcp: add check_id for lookup_anno_list_by_saddr

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new helper mptcp_addresses_equal_check_id() to test the
address ids, as well as the address. This can be used to test if the two
given addresses are identically equal, they have both the same address and
the same address id.

Add a new parameter check_id for mptcp_lookup_anno_list_by_saddr(), pass
it to mptcp_addresses_equal_check_id(). Then in mptcp_pm_del_add_timer(),
the input parameter check_id can be passed as the new parameter into
mptcp_lookup_anno_list_by_saddr(). After this, this condition:

        (!check_id || entry->addr.id == addr->id)

can be dropped, only test if 'entry' is NULL is enough.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c         |  2 +-
 net/mptcp/pm_netlink.c | 13 +++++++------
 net/mptcp/protocol.h   | 10 +++++++++-
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 55406720c607..2f5ccafe7e34 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -257,7 +257,7 @@ void mptcp_pm_add_addr_echoed(struct mptcp_sock *msk,
 
 	spin_lock_bh(&pm->lock);
 
-	if (mptcp_lookup_anno_list_by_saddr(msk, addr) && READ_ONCE(pm->work_pending))
+	if (mptcp_lookup_anno_list_by_saddr(msk, addr, false) && READ_ONCE(pm->work_pending))
 		mptcp_pm_schedule_work(msk, MPTCP_PM_SUBFLOW_ESTABLISHED);
 
 	spin_unlock_bh(&pm->lock);
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 5c17d39146ea..7d755c0a32bb 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -237,14 +237,15 @@ bool mptcp_pm_nl_check_work_pending(struct mptcp_sock *msk)
 
 struct mptcp_pm_add_entry *
 mptcp_lookup_anno_list_by_saddr(const struct mptcp_sock *msk,
-				const struct mptcp_addr_info *addr)
+				const struct mptcp_addr_info *addr,
+				bool check_id)
 {
 	struct mptcp_pm_add_entry *entry;
 
 	lockdep_assert_held(&msk->pm.lock);
 
 	list_for_each_entry(entry, &msk->pm.anno_list, list) {
-		if (mptcp_addresses_equal(&entry->addr, addr, true))
+		if (mptcp_addresses_equal_check_id(&entry->addr, addr, true, check_id))
 			return entry;
 	}
 
@@ -324,12 +325,12 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 	struct sock *sk = (struct sock *)msk;
 
 	spin_lock_bh(&msk->pm.lock);
-	entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
-	if (entry && (!check_id || entry->addr.id == addr->id))
+	entry = mptcp_lookup_anno_list_by_saddr(msk, addr, check_id);
+	if (entry)
 		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
 	spin_unlock_bh(&msk->pm.lock);
 
-	if (entry && (!check_id || entry->addr.id == addr->id))
+	if (entry)
 		sk_stop_timer_sync(sk, &entry->add_timer);
 
 	return entry;
@@ -344,7 +345,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
 
 	lockdep_assert_held(&msk->pm.lock);
 
-	add_entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
+	add_entry = mptcp_lookup_anno_list_by_saddr(msk, addr, false);
 
 	if (add_entry) {
 		if (mptcp_pm_is_kernel(msk))
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7905783c95e4..130d4c55e10a 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -673,6 +673,13 @@ bool mptcp_addresses_equal(const struct mptcp_addr_info *a,
 			   const struct mptcp_addr_info *b, bool use_port);
 void mptcp_local_address(const struct sock_common *skc, struct mptcp_addr_info *addr);
 
+static inline bool mptcp_addresses_equal_check_id(const struct mptcp_addr_info *a,
+						  const struct mptcp_addr_info *b,
+						  bool use_port, bool check_id)
+{
+	return mptcp_addresses_equal(a, b, use_port) ? a->id == b->id : false;
+}
+
 /* called with sk socket lock held */
 int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 			    const struct mptcp_addr_info *remote);
@@ -966,7 +973,8 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 		       const struct mptcp_addr_info *addr, bool check_id);
 struct mptcp_pm_add_entry *
 mptcp_lookup_anno_list_by_saddr(const struct mptcp_sock *msk,
-				const struct mptcp_addr_info *addr);
+				const struct mptcp_addr_info *addr,
+				bool check_id);
 int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
 					 unsigned int id,
 					 u8 *flags, int *ifindex);
-- 
2.40.1

[PATCH mptcp-next v2 7/7] selftests: mptcp: flush userspace addrs list

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new helper userspace_pm_flush() to flush all addresses
for the userspace PM. Invoke it in userspace pm dump address and subflow
tests. And use dump commands to check if the userspace pm local address
list is empty after addresses flushing.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 41 +++++++++++++++----
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 292fccb1f7f4..be1d2005c4fe 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3356,6 +3356,19 @@ userspace_pm_get_addr()
 	ip netns exec $1 ./pm_nl_ctl get $2 token $tk
 }
 
+# $1: ns
+userspace_pm_flush()
+{
+	local ns="${1}"
+	local id
+	local addr
+
+	while read -r _ id _ _ addr; do
+		userspace_pm_rm_addr "${ns}" "${id}"
+		userspace_pm_rm_sf "${ns}" "${addr}" $SUB_ESTABLISHED
+	done <<< $(userspace_pm_dump "${ns}")
+}
+
 userspace_pm_chk_dump_addr()
 {
 	local ns="${1}"
@@ -3500,27 +3513,37 @@ userspace_tests()
 	if reset_with_events "userspace pm create destroy subflow" &&
 	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
 		set_userspace_pm $ns2
-		pm_nl_set_limits $ns1 0 1
+		pm_nl_set_limits $ns1 0 2
 		speed=5 \
 			run_tests $ns1 $ns2 10.0.1.1 &
 		local tests_pid=$!
 		wait_mpj $ns2
+		userspace_pm_add_sf $ns2 10.0.2.2 10
 		userspace_pm_add_sf $ns2 10.0.3.2 20
-		chk_join_nr 1 1 1
-		chk_mptcp_info subflows 1 subflows 1
-		chk_subflows_total 2 2
+		chk_join_nr 2 2 2
+		chk_mptcp_info subflows 2 subflows 2
+		chk_subflows_total 3 3
 		userspace_pm_chk_dump_addr "${ns2}" \
-			"id 20 flags subflow 10.0.3.2" \
+			$'id 10 flags subflow 10.0.2.2\nid 20 flags subflow 10.0.3.2' \
 			"subflow"
+		userspace_pm_chk_get_addr "${ns2}" "10" "id 10 flags subflow 10.0.2.2"
 		userspace_pm_chk_get_addr "${ns2}" "20" "id 20 flags subflow 10.0.3.2"
 		userspace_pm_rm_addr $ns2 20
 		userspace_pm_rm_sf $ns2 10.0.3.2 $SUB_ESTABLISHED
 		userspace_pm_chk_dump_addr "${ns2}" \
-			"" \
+			"id 10 flags subflow 10.0.2.2" \
 			"after rm_addr 20"
-		chk_rm_nr 1 1
-		chk_mptcp_info subflows 0 subflows 0
-		chk_subflows_total 1 1
+		if mptcp_lib_kallsyms_has "mptcp_userspace_pm_dump_addr$"; then
+			userspace_pm_flush $ns2
+			userspace_pm_chk_dump_addr "${ns2}" "" "after flush"
+			chk_rm_nr 2 2
+			chk_mptcp_info subflows 0 subflows 0
+			chk_subflows_total 1 1
+		else
+			chk_rm_nr 1 1
+			chk_mptcp_info subflows 1 subflows 1
+			chk_subflows_total 2 2
+		fi
 		kill_events_pids
 		mptcp_lib_kill_wait $tests_pid
 	fi
-- 
2.40.1

Re: selftests: mptcp: flush userspace addrs list: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI (GitHub Action) did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8001362171

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ddc6b3c484c4


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-next v2 0/7] some cleanups

[Matthieu Baerts]

Hi Geliang,

On 22/02/2024 09:03, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> v2:
>  - address Matt's comments in v1

If you don't mind, I think it would be useful to add a short changelog
per patch, under the commit message using '---' as separator (or using
'git notes'): it would help to remember what was discussed, what was
applied, rejected or (accidentally or not) ignored. One line might be
enough, but it would be useful.

> Geliang Tang (7):
>   mptcp: make pm_remove_addrs_and_subflows static
>   mptcp: drop duplicate header inclusions
>   mptcp: update set_flags interfaces
>   mptcp: set error messages for set_flags
>   mptcp: drop lookup_by_id in lookup_addr

The five first patches look good to me:

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

>   mptcp: add check_id for lookup_anno_list_by_saddr
>   selftests: mptcp: flush userspace addrs list

Maybe the 6th patch can be simplified, and the 7th one can be dropped?

Also, please check my comments on patch 7: it is *really, really*
important for you to explain the *reasoning for why patches were
created*. Without a clear reason, we cannot accept patches.

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

Re: [PATCH mptcp-next v2 6/7] mptcp: add check_id for lookup_anno_list_by_saddr

[Matthieu Baerts]

Hi Geliang,

On 22/02/2024 09:03, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new helper mptcp_addresses_equal_check_id() to test the
> address ids, as well as the address. This can be used to test if the two
> given addresses are identically equal, they have both the same address and
> the same address id.
> 
> Add a new parameter check_id for mptcp_lookup_anno_list_by_saddr(), pass
> it to mptcp_addresses_equal_check_id(). Then in mptcp_pm_del_add_timer(),
> the input parameter check_id can be passed as the new parameter into
> mptcp_lookup_anno_list_by_saddr(). After this, this condition:
> 
>         (!check_id || entry->addr.id == addr->id)
> 
> can be dropped, only test if 'entry' is NULL is enough.

Or use an extra variable? Please see below.

(...)

> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 5c17d39146ea..7d755c0a32bb 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c

(...)

> @@ -324,12 +325,12 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
>  	struct sock *sk = (struct sock *)msk;
>  
>  	spin_lock_bh(&msk->pm.lock);
> -	entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
> -	if (entry && (!check_id || entry->addr.id == addr->id))
> +	entry = mptcp_lookup_anno_list_by_saddr(msk, addr, check_id);
> +	if (entry)
>  		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
>  	spin_unlock_bh(&msk->pm.lock);
>  
> -	if (entry && (!check_id || entry->addr.id == addr->id))
> +	if (entry)

I probably missed something, but why not only do the modification here
as suggested in the v1 instead of changing the signature of
mptcp_lookup_anno_list_by_saddr() and introducing
mptcp_addresses_equal_check_id() which is indirectly only used once from
here?


  bool stop_retrans;

  (...)

  spin_lock_bh(&msk->pm.lock);
  entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
  stop_retrans = entry && (!check_id || entry->addr.id == addr->id);
  if (stop_retrans)
      entry->retrans_times = ADD_ADDR_RETRANS_MAX;
  spin_unlock_bh(&msk->pm.lock);

  if (stop_retrans)
      sk_stop_timer_sync(sk, &entry->add_timer);

The refactoring would make sense if you have multiple users of this long
conditions. Here, it looks like it is adding more complexity to get
functions more generic, with more checks, just to handle one particular
case that can be extracted here, no?

>  		sk_stop_timer_sync(sk, &entry->add_timer);
>  
>  	return entry;
(...)

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

Re: [PATCH mptcp-next v2 7/7] selftests: mptcp: flush userspace addrs list

[Matthieu Baerts]

Hi Geliang,

On 22/02/2024 09:03, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a new helper userspace_pm_flush() to flush all addresses
> for the userspace PM. Invoke it in userspace pm dump address and subflow
> tests. And use dump commands to check if the userspace pm local address
> list is empty after addresses flushing.

I'm sorry to insist, but please *always* *always* *always* add an answer
to this question in your commit message: why should we take this patch?

Answers can be very short -- e.g. avoid duplicated code, fix a bug when
doing X, this new feature is useful for this use-case, etc. -- but these
answers *have to* be present.

Here, the commit message only explains what is being done: that's
interesting to verify if what you wanted to do is what you did. But most
of the time, we can see what is being done by quickly looking at the
diff. What is *not easy* to get by looking at the diff is the *reason*
why this patch is needed.

Of course, ↑ is valid for any patches for the Linux kernel, as mentioned
in different places in the doc:
-
https://docs.kernel.org/process/submitting-patches.html?highlight=reason#the-canonical-patch-format

> The explanation body will be committed to the permanent source changelog, so should make sense to a competent reader who has long since forgotten the immediate details of the discussion that might have led to this patch. Including symptoms of the failure which the patch addresses (kernel log messages, oops messages, etc.) are especially useful for people who might be searching the commit logs looking for the applicable patch. The text should be written in such detail so that when read weeks, months or even years later, it can give the reader the needed details to grasp the reasoning for why the patch was created.

-
https://docs.kernel.org/process/maintainer-netdev.html?highlight=reason#preparing-changes

> If your change is a bug fix, make sure your commit log indicates the end-user visible symptom, the underlying reason as to why it happens, and then if necessary, explain why the fix proposed is the best way to get things done.



Now back to this patch, I can very quickly see that you added a new
helper, called it, and checked there were no more addresses. But without
the reason in the commit message, I don't understand why we should do that:
- the output of userspace_pm_dump() is already verified
- the actions done by userspace_pm_rm_addr() and userspace_pm_rm_sf()
helpers are also already verified.
- it looks like we are not covering more .c code by doing that, no?
→ What are you trying to verify that was not verified before?

Maybe we don't need the patch?

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

Re: [PATCH mptcp-next v2 0/7] some cleanups

[Geliang Tang]

Hi Matt,

On Mon, Feb 26, 2024 at 11:06:11AM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 22/02/2024 09:03, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > v2:
> >  - address Matt's comments in v1
> 
> If you don't mind, I think it would be useful to add a short changelog
> per patch, under the commit message using '---' as separator (or using
> 'git notes'): it would help to remember what was discussed, what was
> applied, rejected or (accidentally or not) ignored. One line might be
> enough, but it would be useful.

Sure, I'll use 'git notes' to add notes in the future.

> 
> > Geliang Tang (7):
> >   mptcp: make pm_remove_addrs_and_subflows static
> >   mptcp: drop duplicate header inclusions
> >   mptcp: update set_flags interfaces
> >   mptcp: set error messages for set_flags
> >   mptcp: drop lookup_by_id in lookup_addr
> 
> The five first patches look good to me:
> 
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> >   mptcp: add check_id for lookup_anno_list_by_saddr
> >   selftests: mptcp: flush userspace addrs list
> 
> Maybe the 6th patch can be simplified, and the 7th one can be dropped?

Please apply the 5 first patches for me. I changed the states of them as
'Queued' on patchwork, and 'Changes Requested' for the last two.

> 
> Also, please check my comments on patch 7: it is *really, really*
> important for you to explain the *reasoning for why patches were
> created*. Without a clear reason, we cannot accept patches.

Thanks for the note on patch 7. I'll always explain the reason for the
patch in commit logs in the future. I promise.


Thanks,
-Geliang

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

Re: [PATCH mptcp-next v2 0/7] some cleanups

[Matthieu Baerts]

Hi Geliang,

On 27/02/2024 02:40, Geliang Tang wrote:
> Hi Matt,
> 
> On Mon, Feb 26, 2024 at 11:06:11AM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 22/02/2024 09:03, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> v2:
>>>  - address Matt's comments in v1
>>
>> If you don't mind, I think it would be useful to add a short changelog
>> per patch, under the commit message using '---' as separator (or using
>> 'git notes'): it would help to remember what was discussed, what was
>> applied, rejected or (accidentally or not) ignored. One line might be
>> enough, but it would be useful.
> 
> Sure, I'll use 'git notes' to add notes in the future.

Thanks!

>>> Geliang Tang (7):
>>>   mptcp: make pm_remove_addrs_and_subflows static
>>>   mptcp: drop duplicate header inclusions
>>>   mptcp: update set_flags interfaces
>>>   mptcp: set error messages for set_flags
>>>   mptcp: drop lookup_by_id in lookup_addr
>>
>> The five first patches look good to me:
>>
>> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>>
>>>   mptcp: add check_id for lookup_anno_list_by_saddr
>>>   selftests: mptcp: flush userspace addrs list
>>
>> Maybe the 6th patch can be simplified, and the 7th one can be dropped?
> 
> Please apply the 5 first patches for me. I changed the states of them as
> 'Queued' on patchwork, and 'Changes Requested' for the last two.

I will do it here, but I think it would be better to avoid applying part
of a series: the CI validated the whole series.

Here, it is OK, just some individual cleanups, it looks fine!

>> Also, please check my comments on patch 7: it is *really, really*
>> important for you to explain the *reasoning for why patches were
>> created*. Without a clear reason, we cannot accept patches.
> 
> Thanks for the note on patch 7. I'll always explain the reason for the
> patch in commit logs in the future. I promise.

Great, thanks, that will help for the reviews and for later :)

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

Re: [PATCH mptcp-next v2 0/7] some cleanups

[Matthieu Baerts]

Hi Geliang,

On 22/02/2024 09:03, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> v2:
>  - address Matt's comments in v1
> 
> Geliang Tang (7):
>   mptcp: make pm_remove_addrs_and_subflows static
>   mptcp: drop duplicate header inclusions
>   mptcp: update set_flags interfaces
>   mptcp: set error messages for set_flags
>   mptcp: drop lookup_by_id in lookup_addr

Thank you for the patches! I just applied these 5 patches in our tree
(feat. for net-next):

New patches for t/upstream:
- eb5a7de62ab0: mptcp: make pm_remove_addrs_and_subflows static
- f2d06a42d2cc: mptcp: drop duplicate header inclusions
- 6df3b1d4a5a9: mptcp: update set_flags interfaces
- 5cf3d6085921: mptcp: set error messages for set_flags
- 789aaac60f7f: mptcp: drop lookup_by_id in lookup_addr
- Results: 425c5fffb4c5..ace42d7d3a0f (export)

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

Re: [PATCH mptcp-next v2 6/7] mptcp: add check_id for lookup_anno_list_by_saddr

[Matthieu Baerts]

Hi Geliang,

On 26/02/2024 11:07, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 22/02/2024 09:03, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> This patch adds a new helper mptcp_addresses_equal_check_id() to test the
>> address ids, as well as the address. This can be used to test if the two
>> given addresses are identically equal, they have both the same address and
>> the same address id.
>>
>> Add a new parameter check_id for mptcp_lookup_anno_list_by_saddr(), pass
>> it to mptcp_addresses_equal_check_id(). Then in mptcp_pm_del_add_timer(),
>> the input parameter check_id can be passed as the new parameter into
>> mptcp_lookup_anno_list_by_saddr(). After this, this condition:
>>
>>         (!check_id || entry->addr.id == addr->id)
>>
>> can be dropped, only test if 'entry' is NULL is enough.
> 
> Or use an extra variable? Please see below.
> 
> (...)
> 
>> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
>> index 5c17d39146ea..7d755c0a32bb 100644
>> --- a/net/mptcp/pm_netlink.c
>> +++ b/net/mptcp/pm_netlink.c
> 
> (...)
> 
>> @@ -324,12 +325,12 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
>>  	struct sock *sk = (struct sock *)msk;
>>  
>>  	spin_lock_bh(&msk->pm.lock);
>> -	entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
>> -	if (entry && (!check_id || entry->addr.id == addr->id))
>> +	entry = mptcp_lookup_anno_list_by_saddr(msk, addr, check_id);
>> +	if (entry)
>>  		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
>>  	spin_unlock_bh(&msk->pm.lock);
>>  
>> -	if (entry && (!check_id || entry->addr.id == addr->id))
>> +	if (entry)
> 
> I probably missed something, but why not only do the modification here
> as suggested in the v1 instead of changing the signature of
> mptcp_lookup_anno_list_by_saddr() and introducing
> mptcp_addresses_equal_check_id() which is indirectly only used once from
> here?
> 
> 
>   bool stop_retrans;
> 
>   (...)
> 
>   spin_lock_bh(&msk->pm.lock);
>   entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
>   stop_retrans = entry && (!check_id || entry->addr.id == addr->id);
>   if (stop_retrans)
>       entry->retrans_times = ADD_ADDR_RETRANS_MAX;
>   spin_unlock_bh(&msk->pm.lock);
> 
>   if (stop_retrans)
>       sk_stop_timer_sync(sk, &entry->add_timer);
> 
> The refactoring would make sense if you have multiple users of this long
> conditions. Here, it looks like it is adding more complexity to get
> functions more generic, with more checks, just to handle one particular
> case that can be extracted here, no?

Regarding this patch that is still tracked on Patchwork, just to know
what to do with it: do you plan to send a new version with the suggested
modification here above or should we just drop the patch from Patchwork?

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

Re: [PATCH mptcp-next v2 6/7] mptcp: add check_id for lookup_anno_list_by_saddr

[Geliang Tang]

Hi Matt,

On Thu, 2024-03-28 at 19:27 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 26/02/2024 11:07, Matthieu Baerts wrote:
> > Hi Geliang,
> > 
> > On 22/02/2024 09:03, Geliang Tang wrote:
> > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > 
> > > This patch adds a new helper mptcp_addresses_equal_check_id() to
> > > test the
> > > address ids, as well as the address. This can be used to test if
> > > the two
> > > given addresses are identically equal, they have both the same
> > > address and
> > > the same address id.
> > > 
> > > Add a new parameter check_id for
> > > mptcp_lookup_anno_list_by_saddr(), pass
> > > it to mptcp_addresses_equal_check_id(). Then in
> > > mptcp_pm_del_add_timer(),
> > > the input parameter check_id can be passed as the new parameter
> > > into
> > > mptcp_lookup_anno_list_by_saddr(). After this, this condition:
> > > 
> > >         (!check_id || entry->addr.id == addr->id)
> > > 
> > > can be dropped, only test if 'entry' is NULL is enough.
> > 
> > Or use an extra variable? Please see below.
> > 
> > (...)
> > 
> > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > > index 5c17d39146ea..7d755c0a32bb 100644
> > > --- a/net/mptcp/pm_netlink.c
> > > +++ b/net/mptcp/pm_netlink.c
> > 
> > (...)
> > 
> > > @@ -324,12 +325,12 @@ mptcp_pm_del_add_timer(struct mptcp_sock
> > > *msk,
> > >  	struct sock *sk = (struct sock *)msk;
> > >  
> > >  	spin_lock_bh(&msk->pm.lock);
> > > -	entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
> > > -	if (entry && (!check_id || entry->addr.id == addr->id))
> > > +	entry = mptcp_lookup_anno_list_by_saddr(msk, addr,
> > > check_id);
> > > +	if (entry)
> > >  		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
> > >  	spin_unlock_bh(&msk->pm.lock);
> > >  
> > > -	if (entry && (!check_id || entry->addr.id == addr->id))
> > > +	if (entry)
> > 
> > I probably missed something, but why not only do the modification
> > here
> > as suggested in the v1 instead of changing the signature of
> > mptcp_lookup_anno_list_by_saddr() and introducing
> > mptcp_addresses_equal_check_id() which is indirectly only used once
> > from
> > here?
> > 
> > 
> >   bool stop_retrans;
> > 
> >   (...)
> > 
> >   spin_lock_bh(&msk->pm.lock);
> >   entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
> >   stop_retrans = entry && (!check_id || entry->addr.id == addr-
> > >id);
> >   if (stop_retrans)
> >       entry->retrans_times = ADD_ADDR_RETRANS_MAX;
> >   spin_unlock_bh(&msk->pm.lock);
> > 
> >   if (stop_retrans)
> >       sk_stop_timer_sync(sk, &entry->add_timer);
> > 
> > The refactoring would make sense if you have multiple users of this
> > long
> > conditions. Here, it looks like it is adding more complexity to get
> > functions more generic, with more checks, just to handle one
> > particular
> > case that can be extracted here, no?
> 
> Regarding this patch that is still tracked on Patchwork, just to know
> what to do with it: do you plan to send a new version with the
> suggested
> modification here above or should we just drop the patch from
> Patchwork?

Please drop this patch from Patchwork.

Thanks,
-Geliang

> 
> Cheers,
> Matt