lkml.org 
[lkml]   [2009]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 02/20] blkio: Change CFQ to use CFS like queue time stamps
    From
    On Wed, Nov 4, 2009 at 5:37 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
    > On Wed, Nov 04, 2009 at 09:30:34AM -0500, Jeff Moyer wrote:
    >> Vivek Goyal <vgoyal@redhat.com> writes:
    >>
    >
    > Thanks for the review Jeff.
    >
    >> > o Currently CFQ provides priority scaled time slices to processes. If a process
    >> >   does not use the time slice, either because process did not have sufficient
    >> >   IO to do or because think time of process is large and CFQ decided to disable
    >> >   idling, then processes looses it time slice share.
    >>                            ^^^^^^
    >> loses
    >>
    should be 'process loses'

    >> > +static inline u64 max_vdisktime(u64 min_vdisktime, u64 vdisktime)
    >> > +{
    >> > +   s64 delta = (s64)(vdisktime - min_vdisktime);
    >> > +   if (delta > 0)
    >> > +           min_vdisktime = vdisktime;
    >> > +
    >> > +   return min_vdisktime;
    >> > +}
    >> > +
    >> > +static inline u64 min_vdisktime(u64 min_vdisktime, u64 vdisktime)
    >> > +{
    >> > +   s64 delta = (s64)(vdisktime - min_vdisktime);
    >> > +   if (delta < 0)
    >> > +           min_vdisktime = vdisktime;
    >> > +
    >> > +   return min_vdisktime;
    >> > +}
    >>
    >> Is there a reason you've reimplemented min and max?
    >
    > I think you are referring to min_t and max_t. Will these macros take care
    > of wrapping too?
    >
    > For example, if I used min_t(u64, A, B), then unsigned comparision will
    > not work right wrapping has just taken place for any of the A or B. So if
    > A=-1 and B=2, then min_t() would return B as minimum. This is not right
    > in our case.
    >
    > If we do signed comparison (min_t(s64, A, B)), that also seems to be
    > broken in another case where a value of variable moves from 63bits to 64bits,
    > (A=0x7fffffffffffffff, B=0x8000000000000000). Above will return B as minimum but
    > in our scanario, vdisktime will progress from 0x7fffffffffffffff to
    > 0x8000000000000000 and A should be returned as minimum (unsigned
    > comparison).
    >
    > Hence I took these difnitions from CFS.
    If those are times (measured in jiffies), why are you using u64? You
    could use unsigned long and time_before/time_after, that perform the
    proper wrap checking.


    Corrado
    --
    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: 2009-11-04 19:03    [W:4.161 / U:0.396 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site