Re: [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Mon, Oct 14, 2019 at 02:14:47PM +0800, Xin Long wrote:
> This is a new feature defined in section 5 of rfc7829: "Primary Path
> Switchover". By introducing a new tunable parameter:
> 
>   Primary.Switchover.Max.Retrans (PSMR)
> 
> The primary path will be changed to another active path when the path
> error counter on the old primary path exceeds PSMR, so that "the SCTP
> sender is allowed to continue data transmission on a new working path
> even when the old primary destination address becomes active again".
> 
> This patch is to add this tunable parameter, 'ps_retrans' per netns,
> sock, asoc and transport. It also allows a user to change ps_retrans
> per netns by sysctl, and ps_retrans per sock/asoc/transport will be
> initialized with it.
> 
> The check will be done in sctp_do_8_2_transport_strike() when this
> feature is enabled.
> 
> Note this feature is disabled by initializing 'ps_retrans' per netns
> as 0xffff by default, and its value can't be less than 'pf_retrans'
> when changing by sysctl.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/netns/sctp.h   |  6 ++++++
>  include/net/sctp/structs.h | 11 ++++++++---
>  net/sctp/associola.c       |  3 +++
>  net/sctp/protocol.c        |  3 +++
>  net/sctp/sm_sideeffect.c   |  5 +++++
>  net/sctp/socket.c          |  1 +
>  net/sctp/sysctl.c          |  9 +++++++++
>  7 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h
> index f97d342..c41ffdf 100644
> --- a/include/net/netns/sctp.h
> +++ b/include/net/netns/sctp.h
> @@ -89,6 +89,12 @@ struct netns_sctp {
>  	 */
>  	int pf_retrans;
>  
> +	/* Primary.Switchover.Max.Retrans sysctl value
> +	 * taken from:
> +	 * https://tools.ietf.org/html/rfc7829
> +	 */
> +	int ps_retrans;
> +
>  	/*
>  	 * Disable Potentially-Failed feature, the feature is enabled by default
>  	 * pf_enable	-  0  : disable pf
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 9a43738..3cc913f 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -184,7 +184,8 @@ struct sctp_sock {
>  	__u32 flowlabel;
>  	__u8  dscp;
>  
> -	int pf_retrans;
> +	__u16 pf_retrans;
> +	__u16 ps_retrans;
>  
>  	/* The initial Path MTU to use for new associations. */
>  	__u32 pathmtu;
> @@ -897,7 +898,9 @@ struct sctp_transport {
>  	 * and will be initialized from the assocs value.  This can be changed
>  	 * using the SCTP_PEER_ADDR_THLDS socket option
>  	 */
> -	int pf_retrans;
> +	__u16 pf_retrans;
> +	/* Used for primary path switchover. */
> +	__u16 ps_retrans;
>  	/* PMTU	      : The current known path MTU.  */
>  	__u32 pathmtu;
>  
> @@ -1773,7 +1776,9 @@ struct sctp_association {
>  	 * and will be initialized from the assocs value.  This can be
>  	 * changed using the SCTP_PEER_ADDR_THLDS socket option
>  	 */
> -	int pf_retrans;
> +	__u16 pf_retrans;
> +	/* Used for primary path switchover. */
> +	__u16 ps_retrans;
>  
>  	/* Maximum number of times the endpoint will retransmit INIT  */
>  	__u16 max_init_attempts;
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 46763c5..a839244 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -86,6 +86,7 @@ static struct sctp_association *sctp_association_init(
>  	 */
>  	asoc->max_retrans = sp->assocparams.sasoc_asocmaxrxt;
>  	asoc->pf_retrans  = sp->pf_retrans;
> +	asoc->ps_retrans  = sp->ps_retrans;
>  	asoc->pf_expose   = sp->pf_expose;
>  
>  	asoc->rto_initial = msecs_to_jiffies(sp->rtoinfo.srto_initial);
> @@ -628,6 +629,8 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc,
>  
>  	/* And the partial failure retrans threshold */
>  	peer->pf_retrans = asoc->pf_retrans;
> +	/* And the primary path switchover retrans threshold */
> +	peer->ps_retrans = asoc->ps_retrans;
>  
>  	/* Initialize the peer's SACK delay timeout based on the
>  	 * association configured value.
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index a18c7c4..ea1de9b 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -1217,6 +1217,9 @@ static int __net_init sctp_defaults_init(struct net *net)
>  	/* Max.Burst		    - 4 */
>  	net->sctp.max_burst			= SCTP_DEFAULT_MAX_BURST;
>  
> +	/* Disable of Primary Path Switchover by default */
> +	net->sctp.ps_retrans = 0xffff;
> +
>  	/* Enable pf state by default */
>  	net->sctp.pf_enable = 1;
>  
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index e52b212..acd737d 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -567,6 +567,11 @@ static void sctp_do_8_2_transport_strike(struct sctp_cmd_seq *commands,
>  					     SCTP_FAILED_THRESHOLD);
>  	}
>  
> +	if (transport->error_count > transport->ps_retrans &&
> +	    asoc->peer.primary_path == transport &&
> +	    asoc->peer.active_path != transport)
> +		sctp_assoc_set_primary(asoc, asoc->peer.active_path);
> +
>  	/* E2) For the destination address for which the timer
>  	 * expires, set RTO <- RTO * 2 ("back off the timer").  The
>  	 * maximum value discussed in rule C7 above (RTO.max) may be
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index eccd689..38d102b 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -5078,6 +5078,7 @@ static int sctp_init_sock(struct sock *sk)
>  	sp->hbinterval  = net->sctp.hb_interval;
>  	sp->pathmaxrxt  = net->sctp.max_retrans_path;
>  	sp->pf_retrans  = net->sctp.pf_retrans;
> +	sp->ps_retrans  = net->sctp.ps_retrans;
>  	sp->pf_expose   = net->sctp.pf_expose;
>  	sp->pathmtu     = 0; /* allow default discovery */
>  	sp->sackdelay   = net->sctp.sack_timeout;
> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> index 5d1ad44..adf264a 100644
> --- a/net/sctp/sysctl.c
> +++ b/net/sctp/sysctl.c
> @@ -213,6 +213,15 @@ static struct ctl_table sctp_net_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= proc_dointvec_minmax,
>  		.extra1		= SYSCTL_ZERO,
> +		.extra2		= &init_net.sctp.ps_retrans,
> +	},
> +	{
> +		.procname	= "ps_retrans",
> +		.data		= &init_net.sctp.ps_retrans,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &init_net.sctp.pf_retrans,
>  		.extra2		= SYSCTL_INT_MAX,

pf_retrans got shrunk to 16 bits on other structs (why? I'm thinking
it was just to keep the structs size in check). INT_MAX doesn't make
sense anymore then and in fact it can lead to unexpected behavior.
Like, setting 0x10000 becoming effectively 0.
(yes, usage of >16bits values are probably unreasoable, but..)

>  	},
>  	{
> -- 
> 2.1.0
>