Re: [PATCH] FS: timerfd: Fix unexpected return value of timerfd_read function.

[Arul Jeniston <arul.jeniston@gmail.com>]

Hi tglx,

Thank you for your comments.
Please find my commend in-lined

On Fri, Aug 16, 2019 at 4:15 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Arul,
>
> On Fri, 16 Aug 2019, Arul Jeniston wrote:
>
> > Subject: [PATCH] FS: timerfd: Fix unexpected return value of timerfd_read function.
>
> The prefix is not 'FS: timerfd:'
>
> 1) The usual prefix for fs/* is: 'fs:' but...
>
> 2) git log fs/timerfd.c gives you a pretty good hint for the proper
>    prefix. Look at the commits which actually do functional changes to that
>    file, not at those which do (sub)system wide cleanups/adjustments.
>
> Also 'timerfd_read function' can be written as 'timerfd_read()' which
> spares the redundant function and clearly marks it as function via the
> brackets.
>
> > 'hrtimer_forward_now()' returns zero due to bigger backward time drift.
> > This causes timerfd_read to return 0. As per man page, read on timerfd
> >  is not expected to return 0.
> > This problem is well explained in https://lkml.org/lkml/2019/7/31/442
>
> 1) The explanation needs to be in the changelog itself. Links can point to
>    discussions, bug-reports which have supplementary information.
>
> 2) Please do not use lkml.org links.
>
> Again: Please read and follow Documentation/process/submitting-patches.rst
>
> > . This patch fixes this problem.
> > Signed-off-by: Arul Jeniston <arul.jeniston@gmail.com>
>
> Missing empty line before Signed-off-by. Please use git-log to see how
> changelogs are properly formatted.
>
> Also: 'This patch fixes this problem' is not helpful at all. Again see the
> document I already pointed you to.
>

Agreed. Would incorporate all the above comments.

> > ---
> >  fs/timerfd.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/timerfd.c b/fs/timerfd.c
> > index 6a6fc8aa1de7..f5094e070e9a 100644
> > --- a/fs/timerfd.c
> > +++ b/fs/timerfd.c
> > @@ -284,8 +284,16 @@ static ssize_t timerfd_read(struct file *file,
> > char __user *buf, size_t count,
> >                                         &ctx->t.alarm, ctx->tintv) - 1;
> >                                 alarm_restart(&ctx->t.alarm);
> >                         } else {
> > -                               ticks += hrtimer_forward_now(&ctx->t.tmr,
> > -                                                            ctx->tintv) - 1;
> > +                               u64 nooftimeo = hrtimer_forward_now(&ctx->t.tmr,
> > +                                                                ctx->tintv);
>
> nooftimeo is pretty non-intuitive. The function documentation of
> hrtimer_forward_now() says:
>
>       Returns the number of overruns.
>
> So the obvious variable name is overruns, right?
>

Agreed. Would change the variable name to overruns.

> > +                               /*
> > +                                * ticks shouldn't become zero at this point.
> > +                                * Ignore if hrtimer_forward_now returns 0
> > +                                * due to larger backward time drift.
>
> Again. This explanation does not make any sense at all.
>
> Time does not go backwards, except if it is CLOCK_REALTIME which can be set
> backwards via clock_settime() or settimeofday().
>
> > +                                */
> > +                               if (likely(nooftimeo)) {
> > +                                       ticks += nooftimeo - 1;
> > +                               }
>
> Again: Pointless brackets.
>
> If you disagree with my review comment, then tell me in a reply. If not,
> then fix it. If you decide to ignore my comments, then don't wonder if I
> ignore your patches.
>

We use CLOCK_REALTIME while creating timer_fd.
Can read() on timerfd return 0 when the clock is set to CLOCK_REALTIME?

We have Intel rangely 4 cpu system running debian stretch linux
kernel. The current clock source is set to tsc. During our testing, we
observed the time drifts backward occasionally. Through kernel
instrumentation, we observed, sometimes clocksource_delta() finds the
current time lesser than last time. and returns 0 delta.
This causes  the following code flow to return 0
 ktime_get()-->timekeeping_get_ns()-->timekeeping_get_delta()-->clocksource_delta()

> Thanks,
>
>         tglx