RE: [PATCH V3] suspend: enable freeze timeout configuration through sys

Liu, Chuansheng
2013-02-01T02:02:27Z

> >>>>>> -#define TIMEOUT (20 * HZ)
> >>>>>> +unsigned int __read_mostly sys_freeze_process_timeout_msecs =
> >> 20000;
> >>>>>
> >>>>> 20000 does not mean 20 seconds since we can select HZ other than
> 1000.
> >>>>> So (20 * HZ) is better than 20000.
> >>>>>
> >>>> [Li, Fei]
> >>>> Are you sure about this, (20*HZ) better than 20000, or you mean 20 *
> >> MSEC_PER_SEC?
> >>> Yasuaki mean HZ value will not always be 1000.The value of HZ differs for
> >> each
> >>> supported architecture. In fact, on some supported architectures,
> >>> it even differs between machine types.
> >>> When writing kernel code, never assume that HZ has any given value.
> >>> Right now you are assuming that the delay will be always 20 seconds
> because
> >> of
> >>> your assumption of HZ.
> >>
> >> That's correct, the initial value should be 20 * HZ (i.e. as before).
> > [Li, Fei]
> > Yes, you are right, and IMHO it's already as this in the patch,
> > as 20 * HZ == msecs_to_jiffies(20000), with the current definition
> MSEC_PER_SEC
> > of 1000L. I'll update the default value as 20 * MSEC_PER_SEC in patch V4.
>
> 20 * MSEC_PER_SEC is not 20 seconds. In Linux, 1 * HZ is 1 seconds.
Sure.

> Thus,
> - If HZ is defined as 1000, 1000 is 1 seconds.
> - If HZ is defined as 250, 250 is 1 seconds.
>
> 20 * MSEC_PER_SEC is always 20000.

> Thus,
> - If HZ is defined as 1000, 20 * MSEC_PER_SEC is 20 seconds.
> - If HZ is defined as 250, 20 * MSEC_PER_SEC is 80 seconds.
Could you check the patch again?
The patch are using msecs_to_jiffies(20 * MSEC_PER_SEC), not 20 * MSEC_PER_SEC.
msecs_to_jiffies() has done the conversion from MSEC_PER_SEC to HZ.

>
> So you should use 20 * HZ if you define timeout at 20 seconds.
>
> Thanks,
> Yasuaki Ishimatsu.