| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 12 Mar 2013 12:00:32 -0400
From: Vlad Yasevich <vyasevich@...il.com>
To: Neil Horman <nhorman@...driver.com>
CC: Xufeng Zhang <xufengzhang.main@...il.com>,
Xufeng Zhang <xufeng.zhang@...driver.com>, davem@...emloft.net,
linux-sctp@...r.kernel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sctp: don't break the loop while meeting the active_path
so as to find the matched transport
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 <nhorman@...driver.com> 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@...driver.com>
>>>>>> ---
>>>>>> 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) {
>>>
>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists