Re: [PATCH net-next-2.6 v4 4/5] sctp: Add ASCONF operation on the single-homed host

[Wei Yongjun <yjwei@cn.fujitsu.com>]

> Hi Wei, thanks for your feedback, I'd like to ask about some points that I have a bit of hesitations before fix.  
> Please see in-line.  
>
>>>  * ADDIP 3.1.1 Address Configuration Change Chunk (ASCONF)
>>>  *      0                   1                   2                   3
>>> @@ -2744,11 +2799,24 @@ struct sctp_chunk *sctp_make_asconf_update_ip(struct sctp_association *asoc,
>>> 	int			addr_param_len = 0;
>>> 	int 			totallen = 0;
>>> 	int 			i;
>>> +	sctp_addip_param_t del_param; /* 8 Bytes (Type 0xC002, Len and CrrID) */
>>> +	struct sctp_af *del_af;
>>> +	int del_addr_param_len = 0;
>>> +	int del_paramlen = sizeof(sctp_addip_param_t);
>>> +	union sctp_addr_param del_addr_param; /* (v4) 8 Bytes, (v6) 20 Bytes */
>>> +	int			v4 = 0;
>>> +	int			v6 = 0;
>> you can reuse the af, param_len etc, no need to define new variables.
> This is for the situation that the application adds a new IPv4 and a new IPv6 address simultaneously, although it never happen in the auto_asconf process.  
> Since the af is overwritten to the last seen address in addrs, defining these variables is useful.  
> (Extracting existence of v4 and v6 parameters is possible, but I think it's overkill.  )
> So, how about remaining them?

I means why your need this check?

+        if ((asoc->asconf_addr_del_pending->sa.sa_family == AF_INET
+            && v4) ||
+            (asoc->asconf_addr_del_pending->sa.sa_family == AF_INET6
+            && v6)) {


Add IPv4 and Del IPv6 is possible if socket is AF_INET6 base.


>>> @@ -3361,6 +3486,35 @@ int sctp_process_asconf_ack(struct sctp_association *asoc,
>>> 		asconf_len -= length;
>>> 	}
>>>
>>> +	/* When the source address obviously changes to newly added one, we
>>> +	   reset the cwnd to re-probe the path condition
>> Since we do not do this when the host/peer add/del ip addresses,
>> remain the peer's cwnd etc. to what it is maybe better.
> What do you mean "host/peer add/del ip addresses" ?

I means if we delete the other address other than the last one
address, we do not reset the peer's information.

>> and this is unnecessary when you just change the address of
>> the interface.
> Yes, however, it is mostly impossible to distinguish that situation from change of the physical path.  
> So considering change of source address as change of path could make sense or fine-grain metric to reset congestion control parameter.    
> As one example, in FreeBSD implementation, congestion control parameter is reset when the last remaining address is changed. 

I got what your said. Can you split the peer reset part to an new patch?

This part:

+void
+sctp_path_check_and_react(struct sctp_association *asoc, struct sockaddr *sa)
+{
+	struct sctp_transport *trans;
+	int addrnum, family;
+	struct sctp_sockaddr_entry *saddr;
+	struct sctp_bind_addr *bp;
+	union sctp_addr *tmpaddr;
+
+	family = sa->sa_family;
+	bp = &asoc->base.bind_addr;
+	addrnum = 0;
+	/* count up the number of local addresses in the same family */
+	list_for_each_entry(saddr, &bp->address_list, list) {
+		if (saddr->a.sa.sa_family == family) {
+			tmpaddr = &saddr->a;
+			if (family == AF_INET6 &&
+			    ipv6_addr_type(&tmpaddr->v6.sin6_addr) &
+			    IPV6_ADDR_LINKLOCAL) {
+				continue;
+			}
+			addrnum++;
+		}
+	}


the address be added to the asoc should in the scope, refer to
sctp_in_scope(). So we may not mix link local with global
addresses.
If so, we may simply this patch to reset peer only when we recv
the asconf-ack and the asoc->src_out_of_asoc_ok == true.