| Message ID | 6a0fc4027e27a2bdbbaea2997a5489399ea0fc7d.1729588019.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | BPF path manager | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 116 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/build | warning | Build error with: make C=1 net/mptcp/bpf.o |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf-normal__only_bpftest_all_ | success | Success! ✅ |
| matttbe/KVM_Validation__btf-debug__only_bpftest_all_ | success | Success! ✅ |
Hi Geliang, (I'm still reviewing the series, thank you for your patience. Please wait for me to finish it before sending a v3.) On 22/10/2024 11:14, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > Three path manager wrappers, mptcp_pm_get_addr(), mptcp_pm_dump_addr() and > mptcp_pm_set_flags() are used to switch the interfaces between in-kernel > PM and userspace PM. These wrappers are defined in pm.c but only used in > pm_netlink.c. It makes more sense to move them to pm_netlink.c and make > them all static. These three helpers are calling userspace PM code, that looks "strange" to do that from pm_netlink. I didn't check, but would it not make more sense to move all the _doit() functions calling these helpers to pm.c? Also, do we still these helpers you were moving? Can we not instead remove them, and move their code directly in the _doit() functions if you see what I mean? int mptcp_pm_nl_get_addr_doit(...) { if (info->attrs[MPTCP_PM_ATTR_TOKEN]) return mptcp_userspace_pm_get_addr(skb, info); return mptcp_pm_nl_get_addr(skb, info); } (in pm.c) WDYT? Cheers, Matt
On Wed, 2024-10-30 at 19:31 +0100, Matthieu Baerts wrote: > Hi Geliang, > > (I'm still reviewing the series, thank you for your patience. Please > wait for me to finish it before sending a v3.) > > On 22/10/2024 11:14, Geliang Tang wrote: > > From: Geliang Tang <tanggeliang@kylinos.cn> > > > > Three path manager wrappers, mptcp_pm_get_addr(), > > mptcp_pm_dump_addr() and > > mptcp_pm_set_flags() are used to switch the interfaces between in- > > kernel > > PM and userspace PM. These wrappers are defined in pm.c but only > > used in > > pm_netlink.c. It makes more sense to move them to pm_netlink.c and > > make > > them all static. > > These three helpers are calling userspace PM code, that looks > "strange" > to do that from pm_netlink. I agree. > > I didn't check, but would it not make more sense to move all the > _doit() > functions calling these helpers to pm.c? Also, do we still these > helpers > you were moving? Can we not instead remove them, and move their code > directly in the _doit() functions if you see what I mean? > > int mptcp_pm_nl_get_addr_doit(...) > { > if (info->attrs[MPTCP_PM_ATTR_TOKEN]) > return mptcp_userspace_pm_get_addr(skb, info); > return mptcp_pm_nl_get_addr(skb, info); > } > > (in pm.c) > > WDYT? I prefer to drop this patch and keep the code as is. Thanks, -Geliang > > Cheers, > Matt
Hi Geliang, On 31/10/2024 08:58, Geliang Tang wrote: > On Wed, 2024-10-30 at 19:31 +0100, Matthieu Baerts wrote: >> Hi Geliang, >> >> (I'm still reviewing the series, thank you for your patience. Please >> wait for me to finish it before sending a v3.) >> >> On 22/10/2024 11:14, Geliang Tang wrote: >>> From: Geliang Tang <tanggeliang@kylinos.cn> >>> >>> Three path manager wrappers, mptcp_pm_get_addr(), >>> mptcp_pm_dump_addr() and >>> mptcp_pm_set_flags() are used to switch the interfaces between in- >>> kernel >>> PM and userspace PM. These wrappers are defined in pm.c but only >>> used in >>> pm_netlink.c. It makes more sense to move them to pm_netlink.c and >>> make >>> them all static. >> >> These three helpers are calling userspace PM code, that looks >> "strange" >> to do that from pm_netlink. > > I agree. > >> >> I didn't check, but would it not make more sense to move all the >> _doit() >> functions calling these helpers to pm.c? Also, do we still these >> helpers >> you were moving? Can we not instead remove them, and move their code >> directly in the _doit() functions if you see what I mean? >> >> int mptcp_pm_nl_get_addr_doit(...) >> { >> if (info->attrs[MPTCP_PM_ATTR_TOKEN]) >> return mptcp_userspace_pm_get_addr(skb, info); >> return mptcp_pm_nl_get_addr(skb, info); >> } >> >> (in pm.c) >> >> WDYT? > > I prefer to drop this patch and keep the code as is. Up to you. I think it still makes sense not to have a "public" function simply calling another "public" one with the same arguments. But the code can also stay like that for the moment, no problem. Cheers, Matt
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index f3d354a72c94..f5725c00eb70 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -433,29 +433,6 @@ bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc) return mptcp_pm_nl_is_backup(msk, &skc_local); } -int mptcp_pm_get_addr(struct sk_buff *skb, struct genl_info *info) -{ - if (info->attrs[MPTCP_PM_ATTR_TOKEN]) - return mptcp_userspace_pm_get_addr(skb, info); - return mptcp_pm_nl_get_addr(skb, info); -} - -int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb) -{ - const struct genl_info *info = genl_info_dump(cb); - - if (info->attrs[MPTCP_PM_ATTR_TOKEN]) - return mptcp_userspace_pm_dump_addr(msg, cb); - return mptcp_pm_nl_dump_addr(msg, cb); -} - -int mptcp_pm_set_flags(struct sk_buff *skb, struct genl_info *info) -{ - 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) { struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk); diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index a60a6fc04bf4..8b4815d0df53 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -1792,7 +1792,7 @@ int mptcp_nl_fill_addr(struct sk_buff *skb, return -EMSGSIZE; } -int mptcp_pm_nl_get_addr(struct sk_buff *skb, struct genl_info *info) +static int mptcp_pm_nl_get_addr(struct sk_buff *skb, struct genl_info *info) { struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR]; struct pm_nl_pernet *pernet = genl_info_pm_nl(info); @@ -1842,13 +1842,20 @@ int mptcp_pm_nl_get_addr(struct sk_buff *skb, struct genl_info *info) return ret; } +static int mptcp_pm_get_addr(struct sk_buff *skb, struct genl_info *info) +{ + if (info->attrs[MPTCP_PM_ATTR_TOKEN]) + return mptcp_userspace_pm_get_addr(skb, info); + return mptcp_pm_nl_get_addr(skb, info); +} + int mptcp_pm_nl_get_addr_doit(struct sk_buff *skb, struct genl_info *info) { return mptcp_pm_get_addr(skb, info); } -int mptcp_pm_nl_dump_addr(struct sk_buff *msg, - struct netlink_callback *cb) +static int mptcp_pm_nl_dump_addr(struct sk_buff *msg, + struct netlink_callback *cb) { struct net *net = sock_net(msg->sk); struct mptcp_pm_addr_entry *entry; @@ -1890,6 +1897,15 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg, return msg->len; } +static int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb) +{ + const struct genl_info *info = genl_info_dump(cb); + + if (info->attrs[MPTCP_PM_ATTR_TOKEN]) + return mptcp_userspace_pm_dump_addr(msg, cb); + return mptcp_pm_nl_dump_addr(msg, cb); +} + int mptcp_pm_nl_get_addr_dumpit(struct sk_buff *msg, struct netlink_callback *cb) { @@ -2011,7 +2027,7 @@ static int mptcp_nl_set_flags(struct net *net, return ret; } -int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info) +static int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info) { struct mptcp_pm_addr_entry addr = { .addr = { .family = AF_UNSPEC }, }; struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR]; @@ -2065,6 +2081,13 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info) return 0; } +static int mptcp_pm_set_flags(struct sk_buff *skb, struct genl_info *info) +{ + if (info->attrs[MPTCP_PM_ATTR_TOKEN]) + return mptcp_userspace_pm_set_flags(skb, info); + return mptcp_pm_nl_set_flags(skb, info); +} + int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct genl_info *info) { return mptcp_pm_set_flags(skb, info); diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index ce3b12778b3f..b6e66bfca2e5 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -1036,8 +1036,6 @@ mptcp_pm_del_add_timer(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); -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, @@ -1128,13 +1126,8 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_in bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc); bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc); bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc); -int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb); -int mptcp_pm_nl_dump_addr(struct sk_buff *msg, - struct netlink_callback *cb); int mptcp_userspace_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb); -int mptcp_pm_get_addr(struct sk_buff *skb, struct genl_info *info); -int mptcp_pm_nl_get_addr(struct sk_buff *skb, struct genl_info *info); int mptcp_userspace_pm_get_addr(struct sk_buff *skb, struct genl_info *info);