Hi Geliang,
On 22/10/2024 11:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Like __lookup_addr() helper in pm_netlink.c, a new helper
> mptcp_userspace_pm_lookup_addr() is also defined in pm_userspace.c.
> It looks up the corresponding mptcp_pm_addr_entry address in
> userspace_pm_local_addr_list through the passed "addr" parameter
> and returns it.
>
> This helper can be used in mptcp_userspace_pm_delete_local_addr(),
> mptcp_userspace_pm_get_local_id() and mptcp_userspace_pm_is_backup()
> to simplify the code.
>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> net/mptcp/pm_userspace.c | 56 +++++++++++++++++++++-------------------
> 1 file changed, 29 insertions(+), 27 deletions(-)
>
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 3fb5713cd988..ce0f7131c701 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -26,6 +26,18 @@ void mptcp_free_local_addr_list(struct mptcp_sock *msk)
> }
> }
>
> +static struct mptcp_pm_addr_entry *
> +mptcp_userspace_pm_lookup_addr(struct mptcp_sock *msk, const struct mptcp_addr_info *addr)
When possible, can you try to limit to 80 chars per line?
See: https://github.com/linux-netdev/nipa/pull/41
Using more than 80 is allowed, but it should be restricted to cases
where using less than 80 chars affects the readability, e.g. not to
break 'entry->flags & MY_SPECIFIC_FLAG' in two lines, etc. The idea is
not to abuse of that.
Here for example, it is easy to go to the new line after the ','.
> +{
> + struct mptcp_pm_addr_entry *entry, *tmp;
> +
> + mptcp_for_each_address_safe(msk, entry, tmp) {
Why do you need the '_safe' alternative here? You only return an entry
from the list, and you stop: no need to continue after having modified
the list here as far as I can see, no?
Also, something very important: here you are presenting the modification
as a simple refactoring, but it does change the behaviour: the '_safe'
version is used everywhere, which was not the case before. When you do
something like that, please mention it in the commit message! Without
that, a reviewer might not notice it "OK, just a refactoring", and
developers might wonder later why this was done. I then recommend to
always add either something like:
- "No behaviour change intended here."
- or "Please note that now <something different is done> for <these
cases>, but that's OK to do so <because ...>."
> + if (mptcp_addresses_equal(&entry->addr, addr, false))
> + return entry;
> + }
> + return NULL;
> +}
> +
> static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
> struct mptcp_pm_addr_entry *entry,
> bool needs_id)
> @@ -90,22 +102,20 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
> static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
> struct mptcp_pm_addr_entry *addr)
> {
> - struct mptcp_pm_addr_entry *entry, *tmp;
> struct sock *sk = (struct sock *)msk;
> + struct mptcp_pm_addr_entry *entry;
>
> - mptcp_for_each_address_safe(msk, entry, tmp) {
> - if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
> - /* TODO: a refcount is needed because the entry can
> - * be used multiple times (e.g. fullmesh mode).
> - */
> - list_del_rcu(&entry->list);
> - sock_kfree_s(sk, entry, sizeof(*entry));
> - msk->pm.local_addr_used--;
> - return 0;
> - }
> - }
> -
> - return -EINVAL;
> + entry = mptcp_userspace_pm_lookup_addr(msk, &addr->addr);
> + if (!entry)
> + return -EINVAL;
> +
> + /* TODO: a refcount is needed because the entry can
> + * be used multiple times (e.g. fullmesh mode).
> + */
(Not related to this commit: I wonder if the TODO still makes sense. We
had some discussions with Mat, and I think the conclusion was that it
was OK, but I don't remember why)
> + list_del_rcu(&entry->list);
> + sock_kfree_s(sk, entry, sizeof(*entry));
> + msk->pm.local_addr_used--;
> + return 0;
> }
>
> static struct mptcp_pm_addr_entry *
> @@ -123,17 +133,12 @@ mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
> int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
> struct mptcp_addr_info *skc)
> {
> - struct mptcp_pm_addr_entry *entry = NULL, *e, new_entry;
> + struct mptcp_pm_addr_entry *entry = NULL, new_entry;
> __be16 msk_sport = ((struct inet_sock *)
> inet_sk((struct sock *)msk))->inet_sport;
>
> spin_lock_bh(&msk->pm.lock);
> - mptcp_for_each_address(msk, e) {
> - if (mptcp_addresses_equal(&e->addr, skc, false)) {
> - entry = e;
> - break;
> - }
> - }
> + entry = mptcp_userspace_pm_lookup_addr(msk, skc);
> spin_unlock_bh(&msk->pm.lock);
> if (entry)
> return entry->addr.id;
> @@ -156,12 +161,9 @@ bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
> bool backup = false;
>
> spin_lock_bh(&msk->pm.lock);
> - mptcp_for_each_address(msk, entry) {
> - if (mptcp_addresses_equal(&entry->addr, skc, false)) {
> - backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> - break;
> - }
> - }
> + entry = mptcp_userspace_pm_lookup_addr(msk, skc);
> + if (entry)
> + backup = !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
> spin_unlock_bh(&msk->pm.lock);
>
> return backup;
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.