[v5,mptcp-next,3/3] mptcp: retransmit ADD_ADDR when timeout

This patch implemented the retransmition of ADD_ADDR when no ADD_ADDR echo
is received. It added a timer with the announced address. When timeout
occurs, ADD_ADDR will be retransmitted.

Suggested-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 net/mptcp/options.c    |   1 +
 net/mptcp/pm_netlink.c | 113 ++++++++++++++++++++++++++++++++++-------
 net/mptcp/protocol.h   |   3 ++
 3 files changed, 98 insertions(+), 19 deletions(-)

Hi,

Thank you for the prompt turn-over. There is still a somewhat relevant
item - the last comment in this patch. The others comments are mostly
nit picking, but nice to have.

On Mon, 2020-09-21 at 15:30 +0800, Geliang Tang wrote:
> This patch implemented the retransmition of ADD_ADDR when no ADD_ADDR
> echo
> is received. It added a timer with the announced address. When
> timeout
> occurs, ADD_ADDR will be retransmitted.
> 
> Suggested-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>  net/mptcp/options.c    |   1 +
>  net/mptcp/pm_netlink.c | 113 ++++++++++++++++++++++++++++++++++-----
> --
>  net/mptcp/protocol.h   |   3 ++
>  3 files changed, 98 insertions(+), 19 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 171039cbe9c4..14a290fae767 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -893,6 +893,7 @@ void mptcp_incoming_options(struct sock *sk,
> struct sk_buff *skb,
>  			mptcp_pm_add_addr_received(msk, &addr);
>  			MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_ADDADDR);
>  		} else {
> +			mptcp_pm_del_add_timer(msk, &addr);
>  			MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_ECHOADD);
>  		}
>  		mp_opt.add_addr = 0;
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 701972b55a45..6fad2cc4cf5d 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -31,6 +31,9 @@ struct mptcp_pm_addr_entry {
>  struct mptcp_pm_add_entry {
>  	struct list_head	list;
>  	struct mptcp_addr_info	addr;
> +	struct timer_list	add_timer;
> +	struct mptcp_sock	*sock;
> +	u8			retrans_times;
>  };
>  
>  struct pm_nl_pernet {
> @@ -46,6 +49,7 @@ struct pm_nl_pernet {
>  };
>  
>  #define MPTCP_PM_ADDR_MAX	8
> +#define ADD_ADDR_RETRANS_MAX	3
>  
>  static bool addresses_equal(const struct mptcp_addr_info *a,
>  			    struct mptcp_addr_info *b, bool use_port)
> @@ -183,23 +187,83 @@ static void check_work_pending(struct
> mptcp_sock *msk)
>  		WRITE_ONCE(msk->pm.work_pending, false);
>  }
>  
> -static bool lookup_anno_list_by_saddr(struct mptcp_sock *msk,
> -				      struct mptcp_addr_info *addr)
> +static struct mptcp_pm_add_entry *
> +lookup_anno_list_by_saddr(struct mptcp_sock *msk,
> +			  struct mptcp_addr_info *addr)
>  {
> -	struct mptcp_pm_add_entry *entry;
> +	struct mptcp_pm_add_entry *entry, *ret = NULL;
>  
>  	list_for_each_entry(entry, &msk->pm.anno_list, list) {
> -		if (addresses_equal(&entry->addr, addr, false))
> -			return true;
> +		if (addresses_equal(&entry->addr, addr, false)) {
> +			ret = entry;
> +			break;

Or simply:
			return entry;

No braches, no break, no local variable.
> +		}
>  	}
>  
> -	return false;
> +	return ret;

[with the above change]
	return NULL;
> +}
> +
> +static void mptcp_pm_add_timer(struct timer_list *timer)
> +{
> +	struct mptcp_pm_add_entry *entry = from_timer(entry, timer,
> add_timer);
> +	struct mptcp_sock *msk = entry->sock;
> +	struct sock *sk = (struct sock *)msk;
> +
> +	pr_debug("msk=%p", msk);
> +
> +	if (!msk)
> +		return;
> +
> +	if (sk->sk_state == TCP_CLOSE)

Since this is a lockless context (sorry for not noticing it before):

	if (inet_sk_state_load(sk))

> +		return;
> +
> +	if (!entry->addr.id)
> +		return;
> +
> +	if (mptcp_pm_should_add_signal(msk)) {
> +		sk_reset_timer(sk, timer, jiffies + TCP_RTO_MAX / 8);
> +		goto out;
> +	}
> +
> +	spin_lock_bh(&msk->pm.lock);
> +
> +	if (!mptcp_pm_should_add_signal(msk)) {
> +		pr_debug("retransmit ADD_ADDR id=%d", entry->addr.id);
> +		mptcp_pm_announce_addr(msk, &entry->addr, false);
> +		entry->retrans_times++;
> +	}
> +
> +	if (entry->retrans_times < ADD_ADDR_RETRANS_MAX)
> +		sk_reset_timer(sk, timer, jiffies + TCP_RTO_MAX);
> +
> +	spin_unlock_bh(&msk->pm.lock);
> +
> +out:
> +	__sock_put(sk);
> +}
> +
> +struct mptcp_pm_add_entry *
> +mptcp_pm_del_add_timer(struct mptcp_sock *msk,
> +		       struct mptcp_addr_info *addr)
> +{
> +	struct mptcp_pm_add_entry *entry;
> +	struct sock *sk = (struct sock *)msk;
> +
> +	spin_lock_bh(&msk->pm.lock);
> +	entry = lookup_anno_list_by_saddr(msk, addr);

here it's additionally needed:

	entry->retrans_times = ADD_ADDR_RETRANS_MAX;

to prevent the timer re-scheduling, or bad things could happen!


Cheers,

Paolo

Hi Geliang, Paolo

On 21/09/2020 12:24, Paolo Abeni wrote:
> Hi,
> 
> Thank you for the prompt turn-over. There is still a somewhat relevant
> item - the last comment in this patch. The others comments are mostly
> nit picking, but nice to have.
> 
> On Mon, 2020-09-21 at 15:30 +0800, Geliang Tang wrote:
>> This patch implemented the retransmition of ADD_ADDR when no ADD_ADDR
>> echo
>> is received. It added a timer with the announced address. When
>> timeout
>> occurs, ADD_ADDR will be retransmitted.
>>
>> Suggested-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
>> Signed-off-by: Geliang Tang <geliangtang@gmail.com>

As discussed on IRC, I just applied this patch + the "squash to" one:

- a972ed5d1ec5: mptcp: add struct mptcp_pm_add_entry
- 39a25e21cbca: mptcp: add sk_stop_timer_sync helper
- e83b080276ea: mptcp: retransmit ADD_ADDR when timeout

Paolo's ACK has been added to these three patches. I applied these 
patches after Geliang' selftests patches because the timeout feature is 
not validated by any kselftests.

- fe8df13fc5d3..8c5acf4c1ab3: result

Note that it would maybe be more interesting to add packetdrill test to 
validate this feature.

Tests + export are going to be started soon.

Cheers,
Matt

Hi Matt,

Matthieu Baerts <matthieu.baerts@tessares.net> 于2020年9月22日周二 上午1:53写道：
>
> Hi Geliang, Paolo
>
> On 21/09/2020 12:24, Paolo Abeni wrote:
> > Hi,
> >
> > Thank you for the prompt turn-over. There is still a somewhat relevant
> > item - the last comment in this patch. The others comments are mostly
> > nit picking, but nice to have.
> >
> > On Mon, 2020-09-21 at 15:30 +0800, Geliang Tang wrote:
> >> This patch implemented the retransmition of ADD_ADDR when no ADD_ADDR
> >> echo
> >> is received. It added a timer with the announced address. When
> >> timeout
> >> occurs, ADD_ADDR will be retransmitted.
> >>
> >> Suggested-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> >> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> >> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
>
> As discussed on IRC, I just applied this patch + the "squash to" one:
>
> - a972ed5d1ec5: mptcp: add struct mptcp_pm_add_entry
> - 39a25e21cbca: mptcp: add sk_stop_timer_sync helper
> - e83b080276ea: mptcp: retransmit ADD_ADDR when timeout
>
> Paolo's ACK has been added to these three patches. I applied these
> patches after Geliang' selftests patches because the timeout feature is
> not validated by any kselftests.

Thanks, as these patches have been applied, I think both issue #49 (ADD_ADDR:
echo bit support) and issue #50 (REMOVE_ADDR support) could be closed.

And I'll write the selftest case of these patches later.

-Geliang

>
> - fe8df13fc5d3..8c5acf4c1ab3: result
>
> Note that it would maybe be more interesting to add packetdrill test to
> validate this feature.
>
> Tests + export are going to be started soon.
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net