lkml.org 
[lkml]   [2007]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: bad networking related lag in v2.6.22-rc2
    Linus Torvalds wrote:
    > There appear to be other obvious problems in the recent "cleanups" in this
    > area..
    >
    > Look at
    >
    > psched_tdiff_bounded(psched_time_t tv1, psched_time_t tv2, psched_time_t bound)
    > {
    > return min(tv1 - tv2, bound);
    > }
    >
    > and compare it to the previous code:
    >
    > #define PSCHED_TDIFF_SAFE(tv1, tv2, bound) \
    > min_t(long long, (tv1) - (tv2), bound)
    >
    > and ponder how that "trivial cleanup" totally broke the thing.
    >
    > Hint: "psched_time_t" is an "u64". What does that mean for
    >
    > min(tv1 - tv2, bound);
    >
    > again, when "tv2" is larger than tv1. It _used_ to return a negative
    > value. Now it returns a positive "bound" upper bound, because "tv1-tv2"
    > will be used as a huge unsigned (and thus _positive_) integer. And was
    > that accidental, or done on purpose?
    >
    > Sounds accidental to me, since you then want to return a "psched_tdiff_t",
    > which is typedeffed to be "long".
    >
    > Doesn't sound very safe to me, especially since the commit message for
    > this is "[NET_SCHED]: turn PSCHED_TDIFF_SAFE into inline function", and
    > there's no indication that anybody realized that it changed semantics in
    > the process.


    I did realize it, but tv2 > tv1 can't happen and makes no sense for
    the users of this function. I probably should have provided a more
    detailed changelog entry.

    > Hmm? What _should_ that thing do?


    It is used to calculate the amount of tokens a tocken bucket has
    accumulated since the last refill, thus we always have tv1 >= tv2
    (modulo ktime wraps). In fact tv2 > tv1 was never properly
    supported. This macro would have returned the negative long long
    value, but all users assign it to a psched_tdiff_t (long), so
    depending on the exact values, it might still be interpreted as a
    large positive value. Additionally there was a second implementation
    for the gettimeofday clocksource that didn't return the negative
    difference but the bound value.

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-05-23 19:23    [W:3.741 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site