Em Tue, Sep 25, 2007 at 10:58:41AM +0100, Gerrit Renker escreveu:
> Arnaldo -
>
> | Algorithm is fine, just use time_after when comparing jiffies based
> | timestamps, here:
> Many thanks for pointing this out - it was a stupid oversight. The interdiff
> to the previous patch is:
>
> * These Syncs are rate-limited as per RFC 4340, 7.5.4:
> * at most 1 / (dccp_sync_rate_limit * HZ) Syncs per second.
> */
> - if (now - dp->dccps_rate_last >= sysctl_dccp_sync_ratelimit) {
> + if (time_after(now, dp->dccps_rate_last
> + + sysctl_dccp_sync_ratelimit)) {
> dp->dccps_rate_last = now;
> /* ... */
>
> I have only compile-tested this, but verified twice on paper, and it looks
> correct
> (the `>=' has been replaced with `>', using `time_after_eq' does not seem
> necessary).
> Please find update below.
Thanks, some nits below:
>
> --- a/net/dccp/sysctl.c
> +++ b/net/dccp/sysctl.c
> @@ -18,6 +18,9 @@
> #error This file should not be compiled without CONFIG_SYSCTL defined
> #endif
>
> +/* rate-limit for syncs in reply to sequence-invalid packets; RFC 4340,
> 7.5.4 */
> +int sysctl_dccp_sync_ratelimit __read_mostly = HZ / 8;
Why the extra spaces/tabs before __read_mostly? Don't worry about this
one or the other ones you submitted so far, I'll eventually do a coding
style cleanup once the number of outstanding patches gets to some
manageable level, but please take this in consideration for your next
patches, ok? One more:
> --- a/net/dccp/proto.c
> +++ b/net/dccp/proto.c
> + if (time_after(now, dp->dccps_rate_last
> + + sysctl_dccp_sync_ratelimit)) {
In linux networking code what has been the most accepted form for
multiline expressions is:
if (time_after(now, (dp->dccps_rate_last +
sysctl_dccp_sync_ratelimit))) {
Either form produces the same code, but as the later is what I, David
and others are most confortable with and have been using for quite a
while, please try to get used to doing it this way, ok?
Thanks!
- Arnaldo
-
To unsubscribe from this list: send the line "unsubscribe dccp" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
