All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
To: David Laight <David.Laight@ACULAB.COM>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Cc: Neil Horman <nhorman@tuxdriver.com>,
	Vlad Yasevich <vyasevich@gmail.com>,
	"linux-sctp@vger.kernel.org" <linux-sctp@vger.kernel.org>
Subject: Re: [PATCH] sctp: avoid refreshing heartbeat timer too often
Date: Wed, 30 Mar 2016 12:13:18 +0000	[thread overview]
Message-ID: <56FBC2DE.3000207@gmail.com> (raw)
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D54DBFFCD@AcuExch.aculab.com>

Em 30-03-2016 06:37, David Laight escreveu:
> From: Marcelo Ricardo Leitner
>> Sent: 29 March 2016 14:42
>>
>> Currently on high rate SCTP streams the heartbeat timer refresh can
>> consume quite a lot of resources as timer updates are costly and it
>> contains a random factor, which a) is also costly and b) invalidates
>> mod_timer() optimization for not editing a timer to the same value.
>> It may even cause the timer to be slightly advanced, for no good reason.
>
> Interesting thoughts:
> 1) Is it necessary to use a different 'random factor' until the timer actually
>     expires?

I don't understand you fully here, but we have to have a random factor 
on timer expire. As noted by Daniel Borkmann on his commit 8f61059a96c2 
("net: sctp: improve timer slack calculation for transport HBs"):

     RFC4960, section 8.3 says:

       On an idle destination address that is allowed to heartbeat,
       it is recommended that a HEARTBEAT chunk is sent once per RTO
       of that destination address plus the protocol parameter
       'HB.interval', with jittering of +/- 50% of the RTO value,
       and exponential backoff of the RTO if the previous HEARTBEAT
       is unanswered.

Previous to his commit, it was using a random factor based on jiffies.

This patch then assumes that random_A+2 is just as random as random_B as 
long as it is within the allowed range, avoiding the unnecessary updates.

> 2) It might be better to allow the heartbeat timer to expire, on expiry work
>     out the new interval based on when the last 'refresh' was done.

Cool, I thought about this too. It would introduce some extra complexity 
that is not really worth I think, specially because now we may be doing 
more timer updates even with this patch but it's not triggering any wake 
ups and we would need at least 2 wake ups then: one for the first 
timeout event, and then re-schedule the timer for the next updated one, 
and maybe again, and again.. less timer updates but more wake ups, one 
at every heartbeat interval even on a busy transport. Seems it's cheaper 
to just update the timer then.

Thanks,
Marcelo


WARNING: multiple messages have this Message-ID (diff)
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
To: David Laight <David.Laight@ACULAB.COM>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Cc: Neil Horman <nhorman@tuxdriver.com>,
	Vlad Yasevich <vyasevich@gmail.com>,
	"linux-sctp@vger.kernel.org" <linux-sctp@vger.kernel.org>
Subject: Re: [PATCH] sctp: avoid refreshing heartbeat timer too often
Date: Wed, 30 Mar 2016 09:13:18 -0300	[thread overview]
Message-ID: <56FBC2DE.3000207@gmail.com> (raw)
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D54DBFFCD@AcuExch.aculab.com>

Em 30-03-2016 06:37, David Laight escreveu:
> From: Marcelo Ricardo Leitner
>> Sent: 29 March 2016 14:42
>>
>> Currently on high rate SCTP streams the heartbeat timer refresh can
>> consume quite a lot of resources as timer updates are costly and it
>> contains a random factor, which a) is also costly and b) invalidates
>> mod_timer() optimization for not editing a timer to the same value.
>> It may even cause the timer to be slightly advanced, for no good reason.
>
> Interesting thoughts:
> 1) Is it necessary to use a different 'random factor' until the timer actually
>     expires?

I don't understand you fully here, but we have to have a random factor 
on timer expire. As noted by Daniel Borkmann on his commit 8f61059a96c2 
("net: sctp: improve timer slack calculation for transport HBs"):

     RFC4960, section 8.3 says:

       On an idle destination address that is allowed to heartbeat,
       it is recommended that a HEARTBEAT chunk is sent once per RTO
       of that destination address plus the protocol parameter
       'HB.interval', with jittering of +/- 50% of the RTO value,
       and exponential backoff of the RTO if the previous HEARTBEAT
       is unanswered.

Previous to his commit, it was using a random factor based on jiffies.

This patch then assumes that random_A+2 is just as random as random_B as 
long as it is within the allowed range, avoiding the unnecessary updates.

> 2) It might be better to allow the heartbeat timer to expire, on expiry work
>     out the new interval based on when the last 'refresh' was done.

Cool, I thought about this too. It would introduce some extra complexity 
that is not really worth I think, specially because now we may be doing 
more timer updates even with this patch but it's not triggering any wake 
ups and we would need at least 2 wake ups then: one for the first 
timeout event, and then re-schedule the timer for the next updated one, 
and maybe again, and again.. less timer updates but more wake ups, one 
at every heartbeat interval even on a busy transport. Seems it's cheaper 
to just update the timer then.

Thanks,
Marcelo

  reply	other threads:[~2016-03-30 12:13 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-29 13:41 [PATCH] sctp: avoid refreshing heartbeat timer too often Marcelo Ricardo Leitner
2016-03-29 13:41 ` Marcelo Ricardo Leitner
2016-03-30  9:37 ` David Laight
2016-03-30 12:13   ` Marcelo Ricardo Leitner [this message]
2016-03-30 12:13     ` Marcelo Ricardo Leitner
2016-03-31 11:16     ` David Laight
2016-03-31 21:25       ` 'Marcelo Ricardo Leitner'
2016-03-31 21:25         ` 'Marcelo Ricardo Leitner'
2016-04-01  0:24         ` Marcelo Ricardo Leitner
2016-04-01  0:24           ` Marcelo Ricardo Leitner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56FBC2DE.3000207@gmail.com \
    --to=marcelo.leitner@gmail.com \
    --cc=David.Laight@ACULAB.COM \
    --cc=linux-sctp@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=nhorman@tuxdriver.com \
    --cc=vyasevich@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.