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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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.
Neil
On 3/8/13, Neil Horman <[email protected]> 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 <[email protected]>
>> ---
>> 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.
Thanks,
Xufeng
>
> Neil
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Mon, Mar 11, 2013 at 10:14:50AM +0800, Xufeng Zhang wrote:
> On 3/8/13, Neil Horman <[email protected]> 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 <[email protected]>
> >> ---
> >> 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.
*
*/
- 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) {
>>
>> 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
Aha, seems I have some misunderstanding previously, now I got your point.
Yeah, it's better to do the clean up by this way, and this fix looks fine to me,
but I didn't have a test case to test this, actually this problem was detected
by code review, so I would like to leave the rest of this work to
determine by you.
Thank you very much for your clarification!
Thanks,
Xufeng
>
>
> 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.
> *
> */
>
> - 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) {
>
On Tue, Mar 12, 2013 at 10:24:02AM +0800, Xufeng Zhang wrote:
> >>
> >> 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
>
> Aha, seems I have some misunderstanding previously, now I got your point.
> Yeah, it's better to do the clean up by this way, and this fix looks fine to me,
> but I didn't have a test case to test this, actually this problem was detected
> by code review, so I would like to leave the rest of this work to
> determine by you.
>
> Thank you very much for your clarification!
>
Ok, I'll try set up a test for this today
Neil
>
> Thanks,
> Xufeng
>
> >
> >
> > 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.
> > *
> > */
> >
> > - 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) {
> >
>
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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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.
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) {
>
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 <[email protected]> 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 <[email protected]>
> >>>>---
> >>>> 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) {
> >
>
>
On 03/12/2013 11:44 AM, Neil Horman wrote:
> 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 <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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
You also need to take special care if an active transport is removed.
The active path will get reassigned then. I am not sure if it is worth
doing all these changes to address a corner case optimization (CWR is a
corner case).
-vlad
>
>>
>> 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) {
>>>
>>
>>
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 <[email protected]>
> ---
> 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
>
Based on discussion with Vlad, it seems theres arguably some work to do on
access to the transport_addr_list before my solution is viable, so until I get
to that I'm acking this patch.
Acked-by: Neil Horman <[email protected]>
On 03/08/2013 02:39 AM, 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 <[email protected]>
> ---
> 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) {
>
Acked-by: Vlad Yasevich <[email protected]>
-vlad
From: Vlad Yasevich <[email protected]>
Date: Wed, 13 Mar 2013 09:52:48 -0400
> On 03/08/2013 02:39 AM, 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 <[email protected]>
...
> Acked-by: Neil Horman <[email protected]>
...
> Acked-by: Vlad Yasevich <[email protected]>
Applied, thanks everyone.