| Message ID | 2f68d8851458f6f5acfd9a6ea549f3b73029d4bc.1740019794.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | BPF path manager, part 4 | expand |
| Context | Check | Description |
|---|---|---|
| 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_ | warning | Unstable: 1 failed test(s): bpftest_test_progs-no_alu32_mptcp |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 58 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/build | success | Build and static analysis OK |
Hi Geliang, On 20/02/2025 03:57, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > Generally, in the path manager interfaces, the local address is defined > as an mptcp_pm_addr_entry type address, while the remote address is > defined as an mptcp_addr_info type one: > > (struct mptcp_pm_addr_entry *local, struct mptcp_addr_info *remote) > > In order to make these interfaces more flexible and extensible, a struct > mptcp_pm_param is defined here to pass parameters. "entry" can be used > as the local address entry, and "addr" can be used as the remote address. Mmh, it is not clear to me why you cannot use only the parameters that are needed per interfaces, e.g. "local" and "remote". These parameters are not even always needed, e.g. when an address has been announced, "remote" doesn't make sense here. Same for get_local_id and get_priority. Similarly, when an address has been removed, only the ID is needed for the PM, etc. I'm not convinced by the "flexible and extensible" reasons for the future: "local" and "remote" will likely not change. If we want to add more parameters, will we require them for all interfaces? Also, if you use similar signatures as the ones being used, no need to do many changes in the current in-kernel and userspace PM, e.g. no need to introduce the new helpers in patches 5 and 6. Instead, these patches could directly set the available helpers, and we can reduce the number of patches, e.g. patches 8 and 9 (and more) could all come in one, no? Plus, it feels strange to copy data from one structure to another just to pass parameters, no? Are you sure we need this patch? (and the next one?) > Also add a new helper mptcp_pm_param_set_contexts() to set a struct > mptcp_pm_param type parameter. > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > --- > include/net/mptcp.h | 13 +++++++++++++ > net/mptcp/pm.c | 10 ++++++++++ > net/mptcp/protocol.h | 11 +++-------- > 3 files changed, 26 insertions(+), 8 deletions(-) > > diff --git a/include/net/mptcp.h b/include/net/mptcp.h > index 72d6e6597add..a41d6c74760f 100644 > --- a/include/net/mptcp.h > +++ b/include/net/mptcp.h > @@ -121,6 +121,19 @@ struct mptcp_sched_ops { > void (*release)(struct mptcp_sock *msk); > } ____cacheline_aligned_in_smp; > > +struct mptcp_pm_addr_entry { > + struct list_head list; > + struct mptcp_addr_info addr; > + u8 flags; > + int ifindex; > + struct socket *lsk; > +}; > + > +struct mptcp_pm_param { > + struct mptcp_pm_addr_entry entry; > + struct mptcp_addr_info addr; If this structure is really needed, it seems clearer to use "local" and "remote" instead of "entry" and "addr". Cheers, Matt
diff --git a/include/net/mptcp.h b/include/net/mptcp.h index 72d6e6597add..a41d6c74760f 100644 --- a/include/net/mptcp.h +++ b/include/net/mptcp.h @@ -121,6 +121,19 @@ struct mptcp_sched_ops { void (*release)(struct mptcp_sock *msk); } ____cacheline_aligned_in_smp; +struct mptcp_pm_addr_entry { + struct list_head list; + struct mptcp_addr_info addr; + u8 flags; + int ifindex; + struct socket *lsk; +}; + +struct mptcp_pm_param { + struct mptcp_pm_addr_entry entry; + struct mptcp_addr_info addr; +}; + #ifdef CONFIG_MPTCP void mptcp_init(void); diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index 94620ab172b7..6a504c870e1a 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -401,6 +401,16 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining, return ret; } +void mptcp_pm_param_set_contexts(struct mptcp_pm_param *param, + const struct mptcp_pm_addr_entry *entry, + const struct mptcp_addr_info *addr) +{ + if (entry) + param->entry = *entry; + if (addr) + param->addr = *addr; +} + int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc) { struct mptcp_pm_addr_entry skc_local; diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index ef1d43406f9b..dbcf4b84e0f0 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -246,14 +246,6 @@ struct mptcp_pm_local { int ifindex; }; -struct mptcp_pm_addr_entry { - struct list_head list; - struct mptcp_addr_info addr; - u8 flags; - int ifindex; - struct socket *lsk; -}; - struct mptcp_data_frag { struct list_head list; u64 data_seq; @@ -1125,6 +1117,9 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, const struct sk_buff *skb, bool *drop_other_suboptions); bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining, struct mptcp_rm_list *rm_list); +void mptcp_pm_param_set_contexts(struct mptcp_pm_param *param, + const struct mptcp_pm_addr_entry *entry, + const struct mptcp_addr_info *addr); int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc); int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_pm_addr_entry *skc);