Re: [PATCH] sctp: don't break the loop while meeting the active_pathso as to find the matched transport

[Neil Horman]

On Tue, Mar 12, 2013 at 08:11:34AM -0400, Vlad Yasevich wrote:
> On 03/11/2013 09:31 AM, Neil Horman wrote:
> >On Mon, Mar 11, 2013 at 10:14:50AM +0800, Xufeng Zhang wrote:
> >>On 3/8/13, Neil Horman <nhorman@xxxxxxxxxxxxx> wrote:
> >>>On Fri, Mar 08, 2013 at 03:39:37PM +0800, Xufeng Zhang wrote:
> >>>>sctp_assoc_lookup_tsn() function searchs which transport a certain TSN
> >>>>was sent on, if not found in the active_path transport, then go search
> >>>>all the other transports in the peer's transport_addr_list, however, we
> >>>>should continue to the next entry rather than break the loop when meet
> >>>>the active_path transport.
> >>>>
> >>>>Signed-off-by: Xufeng Zhang <xufeng.zhang@xxxxxxxxxxxxx>
> >>>>---
> >>>>  net/sctp/associola.c |    2 +-
> >>>>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>>>
> >>>>diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> >>>>index 43cd0dd..d2709e2 100644
> >>>>--- a/net/sctp/associola.c
> >>>>+++ b/net/sctp/associola.c
> >>>>@@ -1079,7 +1079,7 @@ struct sctp_transport *sctp_assoc_lookup_tsn(struct
> >>>>sctp_association *asoc,
> >>>>  			transports) {
> >>>>
> >>>>  		if (transport == active)
> >>>>-			break;
> >>>>+			continue;
> >>>>  		list_for_each_entry(chunk, &transport->transmitted,
> >>>>  				transmitted_list) {
> >>>>  			if (key == chunk->subh.data_hdr->tsn) {
> >>>>--
> >>>>1.7.0.2
> >>>>
> >>>>
> >>>
> >>>This works, but what might be better would be if we did a move to front
> >>>heuristic in sctp_assoc_set_primary.  E.g. when we set the active_path, move
> >>>the
> >>>requisite transport to the front of the transport_addr_list.  If we did
> >>>that,
> >>>then we could just do one for loop in sctp_assoc_lookup_tsn and wind up
> >>>implicitly check the active path first without having to check it seprately
> >>>and
> >>>skip it in the second for loop.
> >>
> >>Thanks for your review, Neil!
> >>
> >>I know what you mean, yes, it's most probably that the searched TSN was
> >>transmitted in the currently active_path, but what should we do if not.
> >>
> >>Check the comment in sctp_assoc_lookup_tsn() function:
> >>/* Let's be hopeful and check the active_path first. */
> >>/* If not found, go search all the other transports. */
> >>
> >>It has checked the active_path first and then traverse all the other
> >>transports in
> >>the transport_addr_list except the active_path.
> >>
> >>So what I want to fix here is the inconsistency between the function
> >>should do and
> >>the code actually does.
> >>
> >I understand what you're doing, and I agree that the fix is functional (Hence
> >my "This works" statement in my last note).  What I'm suggesting is that, since
> >you're messing about in that code anyway that you clean it up while your at it,
> >so that we don't need to have the if (transport == active) check at all.  We
> >trade in some extra work in a non-critical path (sctp_assoc_set_primary), for
> >the removal of an extra for loop operation and a conditional check in a much
> >hotter path.  Something like this (completely untested), is what I was thinking
> >
> >
> >diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> >index 43cd0dd..8ae873c 100644
> >--- a/net/sctp/associola.c
> >+++ b/net/sctp/associola.c
> >@@ -505,6 +505,9 @@ void sctp_assoc_set_primary(struct sctp_association *asoc,
> >
> >  	asoc->peer.primary_path = transport;
> >
> >+	list_del_rcu(&transport->transports);
> >+	list_add_rcu(&transport->transports, &asoc->peer.transport_addr_list);
> >+
> >  	/* Set a default msg_name for events. */
> >  	memcpy(&asoc->peer.primary_addr, &transport->ipaddr,
> >  	       sizeof(union sctp_addr));
> >@@ -1040,7 +1043,6 @@ struct sctp_chunk *sctp_get_ecne_prepend(struct sctp_association *asoc)
> >  struct sctp_transport *sctp_assoc_lookup_tsn(struct sctp_association *asoc,
> >  					     __u32 tsn)
> >  {
> >-	struct sctp_transport *active;
> >  	struct sctp_transport *match;
> >  	struct sctp_transport *transport;
> >  	struct sctp_chunk *chunk;
> >@@ -1057,29 +1059,16 @@ struct sctp_transport *sctp_assoc_lookup_tsn(struct sctp_association *asoc,
> >  	 * The general strategy is to search each transport's transmitted
> >  	 * list.   Return which transport this TSN lives on.
> >  	 *
> >-	 * Let's be hopeful and check the active_path first.
> >-	 * Another optimization would be to know if there is only one
> >-	 * outbound path and not have to look for the TSN at all.
> >+	 * Note, that sctp_assoc_set_primary does a move to front operation
> >+	 * on the active_path transport, so this code implicitly checks
> >+	 * the active_path first, as we most commonly expect to find our TSN
> >+	 * there.
> >  	 *
> >  	 */
> 
> Neil, active_patch != primary_path all the time.  In fact, when you
> have path primary path failure, active path will change while
> primary
> may only change when the user says so.
Thats a good point, thank you Vlad.  We would need to only update the
transport_addr_list in set_primary if its state is ACTIVE or UNKNOWN.  We would
also need to update it if the active path changes in
sctp_assoc_control_transport, if the new active path is different from the old.
Both of those paths however are not intended to be run frequently, so I think
this is still a viable optimization.  I'm working on it now.
Neil

> 
> So, you may still get into a situation where primary and active
> paths are different.
> 
> The optimization here may not work at all under those circumstances.
> 
> -vlad
> 
> >
> >-	active = asoc->peer.active_path;
> >-
> >-	list_for_each_entry(chunk, &active->transmitted,
> >-			transmitted_list) {
> >-
> >-		if (key == chunk->subh.data_hdr->tsn) {
> >-			match = active;
> >-			goto out;
> >-		}
> >-	}
> >-
> >-	/* If not found, go search all the other transports. */
> >  	list_for_each_entry(transport, &asoc->peer.transport_addr_list,
> >  			transports) {
> >
> >-		if (transport == active)
> >-			break;
> >  		list_for_each_entry(chunk, &transport->transmitted,
> >  				transmitted_list) {
> >  			if (key == chunk->subh.data_hdr->tsn) {
> >
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/