| Message ID | 20220329021437.1196552-4-kishen.maloor@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | mptcp: APIs and self-tests for userspace path management | expand |
On Mon, 2022-03-28 at 22:14 -0400, Kishen Maloor wrote: > This change introduces a parallel path in the kernel for retrieving > the local id, flags, if_index for an addr entry in the context of > an MPTCP connection that's being managed by a userspace PM. The > userspace and in-kernel PM modes deviate in their procedures for > obtaining this information. I'm sorry, but I was not able to give feedback earlier. I really think it would be better do the 'if (mptcp_pm_is_kernel(msk))' in a thin layer, and add the new code into pm_userspace.c e.g. this patch has a relatively large diffstat because quite a bit of existing code is just re-indented, with the proposed schema that will be avoided. Thanks! Paolo
On Mon, 28 Mar 2022, Kishen Maloor wrote: > This change introduces a parallel path in the kernel for retrieving > the local id, flags, if_index for an addr entry in the context of > an MPTCP connection that's being managed by a userspace PM. The > userspace and in-kernel PM modes deviate in their procedures for > obtaining this information. > > Signed-off-by: Kishen Maloor <kishen.maloor@intel.com> > --- > net/mptcp/pm_netlink.c | 95 ++++++++++++++++++++++++++++-------------- > net/mptcp/protocol.h | 3 +- > net/mptcp/subflow.c | 2 +- > 3 files changed, 67 insertions(+), 33 deletions(-) > > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c > index 836f6df9f744..a258593f2cb1 100644 > --- a/net/mptcp/pm_netlink.c > +++ b/net/mptcp/pm_netlink.c > @@ -1036,31 +1036,47 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc) > > pernet = net_generic(sock_net((struct sock *)msk), pm_nl_pernet_id); > > - rcu_read_lock(); > - list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) { > - if (mptcp_addresses_equal(&entry->addr, &skc_local, entry->addr.port)) { > - ret = entry->addr.id; > - break; > + if (mptcp_pm_is_kernel(msk)) { > + rcu_read_lock(); > + list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) { > + if (mptcp_addresses_equal(&entry->addr, &skc_local, entry->addr.port)) { > + ret = entry->addr.id; > + break; > + } > } > + rcu_read_unlock(); > + if (ret >= 0) > + return ret; > + > + /* address not found, add to local list */ > + entry = kmalloc(sizeof(*entry), GFP_ATOMIC); > + if (!entry) > + return -ENOMEM; > + > + entry->addr = skc_local; > + entry->addr.id = 0; > + entry->addr.port = 0; > + entry->ifindex = 0; > + entry->flags = MPTCP_PM_ADDR_FLAG_IMPLICIT; > + entry->lsk = NULL; > + ret = mptcp_pm_nl_append_new_local_addr(pernet, entry); > + if (ret < 0) > + kfree(entry); > + } else if (mptcp_pm_is_userspace(msk)) { > + struct mptcp_pm_addr_entry new_entry; > + __be16 msk_sport = ((struct inet_sock *) > + inet_sk((struct sock *)msk))->inet_sport; > + > + memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry)); > + new_entry.addr = skc_local; > + new_entry.addr.id = 0; > + new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT; > + > + if (new_entry.addr.port == msk_sport) > + new_entry.addr.port = 0; > + > + ret = mptcp_userspace_pm_append_new_local_addr(msk, &new_entry); > } > - rcu_read_unlock(); > - if (ret >= 0) > - return ret; > - > - /* address not found, add to local list */ > - entry = kmalloc(sizeof(*entry), GFP_ATOMIC); > - if (!entry) > - return -ENOMEM; > - > - entry->addr = skc_local; > - entry->addr.id = 0; > - entry->addr.port = 0; > - entry->ifindex = 0; > - entry->flags = MPTCP_PM_ADDR_FLAG_IMPLICIT; > - entry->lsk = NULL; > - ret = mptcp_pm_nl_append_new_local_addr(pernet, entry); > - if (ret < 0) > - kfree(entry); > > return ret; > } > @@ -1298,22 +1314,39 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info) > return 0; > } > > -int mptcp_pm_get_flags_and_ifindex_by_id(struct net *net, unsigned int id, > +int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int id, > u8 *flags, int *ifindex) > { > - struct mptcp_pm_addr_entry *entry; > + struct mptcp_pm_addr_entry *entry, *match = NULL; > + struct sock *sk = (struct sock *)msk; > + struct net *net = sock_net(sk); > > *flags = 0; > *ifindex = 0; > > if (id) { > - rcu_read_lock(); > - entry = __lookup_addr_by_id(net_generic(net, pm_nl_pernet_id), id); > - if (entry) { > - *flags = entry->flags; > - *ifindex = entry->ifindex; > + if (mptcp_pm_is_kernel(msk)) { > + rcu_read_lock(); > + entry = __lookup_addr_by_id(net_generic(net, pm_nl_pernet_id), id); Another conflict here due to the "mptcp: add pm_nl_pernet helpers" commit, the changes in existing code would be avoided with Paolo's suggestion to add a thin layer to call this function (unmodified) or a separate version for the userspace PM. But also be sure to use the new pm_nl_pernet helpers in the new userspace PM code. -- Mat Martineau Intel
On 4/1/22 8:03 AM, Paolo Abeni wrote: > On Mon, 2022-03-28 at 22:14 -0400, Kishen Maloor wrote: >> This change introduces a parallel path in the kernel for retrieving >> the local id, flags, if_index for an addr entry in the context of >> an MPTCP connection that's being managed by a userspace PM. The >> userspace and in-kernel PM modes deviate in their procedures for >> obtaining this information. > > I'm sorry, but I was not able to give feedback earlier. I really think > it would be better do the 'if (mptcp_pm_is_kernel(msk))' in a thin > layer, and add the new code into pm_userspace.c > > e.g. this patch has a relatively large diffstat because quite a bit of > existing code is just re-indented, with the proposed schema that will > be avoided. I have moved code from the 2 functions that had explicit in-kernel vs userspace PM branches into matching functions inside pm_userspace.c. Hopefully that makes it cleaner. > > Thanks! > > Paolo >
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index 836f6df9f744..a258593f2cb1 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -1036,31 +1036,47 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc) pernet = net_generic(sock_net((struct sock *)msk), pm_nl_pernet_id); - rcu_read_lock(); - list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) { - if (mptcp_addresses_equal(&entry->addr, &skc_local, entry->addr.port)) { - ret = entry->addr.id; - break; + if (mptcp_pm_is_kernel(msk)) { + rcu_read_lock(); + list_for_each_entry_rcu(entry, &pernet->local_addr_list, list) { + if (mptcp_addresses_equal(&entry->addr, &skc_local, entry->addr.port)) { + ret = entry->addr.id; + break; + } } + rcu_read_unlock(); + if (ret >= 0) + return ret; + + /* address not found, add to local list */ + entry = kmalloc(sizeof(*entry), GFP_ATOMIC); + if (!entry) + return -ENOMEM; + + entry->addr = skc_local; + entry->addr.id = 0; + entry->addr.port = 0; + entry->ifindex = 0; + entry->flags = MPTCP_PM_ADDR_FLAG_IMPLICIT; + entry->lsk = NULL; + ret = mptcp_pm_nl_append_new_local_addr(pernet, entry); + if (ret < 0) + kfree(entry); + } else if (mptcp_pm_is_userspace(msk)) { + struct mptcp_pm_addr_entry new_entry; + __be16 msk_sport = ((struct inet_sock *) + inet_sk((struct sock *)msk))->inet_sport; + + memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry)); + new_entry.addr = skc_local; + new_entry.addr.id = 0; + new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT; + + if (new_entry.addr.port == msk_sport) + new_entry.addr.port = 0; + + ret = mptcp_userspace_pm_append_new_local_addr(msk, &new_entry); } - rcu_read_unlock(); - if (ret >= 0) - return ret; - - /* address not found, add to local list */ - entry = kmalloc(sizeof(*entry), GFP_ATOMIC); - if (!entry) - return -ENOMEM; - - entry->addr = skc_local; - entry->addr.id = 0; - entry->addr.port = 0; - entry->ifindex = 0; - entry->flags = MPTCP_PM_ADDR_FLAG_IMPLICIT; - entry->lsk = NULL; - ret = mptcp_pm_nl_append_new_local_addr(pernet, entry); - if (ret < 0) - kfree(entry); return ret; } @@ -1298,22 +1314,39 @@ static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info) return 0; } -int mptcp_pm_get_flags_and_ifindex_by_id(struct net *net, unsigned int id, +int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int id, u8 *flags, int *ifindex) { - struct mptcp_pm_addr_entry *entry; + struct mptcp_pm_addr_entry *entry, *match = NULL; + struct sock *sk = (struct sock *)msk; + struct net *net = sock_net(sk); *flags = 0; *ifindex = 0; if (id) { - rcu_read_lock(); - entry = __lookup_addr_by_id(net_generic(net, pm_nl_pernet_id), id); - if (entry) { - *flags = entry->flags; - *ifindex = entry->ifindex; + if (mptcp_pm_is_kernel(msk)) { + rcu_read_lock(); + entry = __lookup_addr_by_id(net_generic(net, pm_nl_pernet_id), id); + if (entry) { + *flags = entry->flags; + *ifindex = entry->ifindex; + } + rcu_read_unlock(); + } else { + spin_lock_bh(&msk->pm.lock); + list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) { + if (id == entry->addr.id) { + match = entry; + break; + } + } + spin_unlock_bh(&msk->pm.lock); + if (match) { + *flags = match->flags; + *ifindex = match->ifindex; + } } - rcu_read_unlock(); } return 0; diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index 40dabf9462a8..4ae3253d7f9a 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -775,7 +775,8 @@ 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_get_flags_and_ifindex_by_id(struct net *net, unsigned int id, +int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, + unsigned int id, u8 *flags, int *ifindex); int mptcp_pm_announce_addr(struct mptcp_sock *msk, diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index a0e7af33fb26..6d59336a8e1e 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -1468,7 +1468,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc, if (local_id) subflow_set_local_id(subflow, local_id); - mptcp_pm_get_flags_and_ifindex_by_id(sock_net(sk), local_id, + mptcp_pm_get_flags_and_ifindex_by_id(msk, local_id, &flags, &ifindex); subflow->remote_key = msk->remote_key; subflow->local_key = msk->local_key;
This change introduces a parallel path in the kernel for retrieving the local id, flags, if_index for an addr entry in the context of an MPTCP connection that's being managed by a userspace PM. The userspace and in-kernel PM modes deviate in their procedures for obtaining this information. Signed-off-by: Kishen Maloor <kishen.maloor@intel.com> --- net/mptcp/pm_netlink.c | 95 ++++++++++++++++++++++++++++-------------- net/mptcp/protocol.h | 3 +- net/mptcp/subflow.c | 2 +- 3 files changed, 67 insertions(+), 33 deletions(-)