Re: [PATCH 02/20] blkio: Change CFQ to use CFS like queue time stamps

[Corrado Zoccolo <czoccolo@gmail.com>]

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