Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Arul Jeniston <arul.jeniston@gmail.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, arul_mc@dell.com
Subject: Re: [PATCH] FS: timerfd: Fix unexpected return value of timerfd_read function.
Date: Fri, 16 Aug 2019 22:25:17 +0530
Message-ID: <CACAVd4h05P2tWb7Eh1+3_0Cm7MkDNAt+SJVoBT4gErBfsBmsAQ@mail.gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.21.1908161224220.1873@nanos.tec.linutronix.de>

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

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-16  8:32 [PATCH] FS: timerfd: Fix unexpected return value of timerfd_read function. '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 patch fixes this problem. Signed-off-by: Arul Jeniston <arul.jeniston@gmail.com> arul.jeniston
2019-08-16  9:05 ` [PATCH] FS: timerfd: [Trimmed unreadable long subject line ] Thomas Gleixner
2019-08-16 10:22 ` [PATCH] FS: timerfd: Fix unexpected return value of timerfd_read function Arul Jeniston
2019-08-16 10:45   ` Thomas Gleixner
2019-08-16 16:55     ` Arul Jeniston [this message]
2019-08-16 17:00       ` Arul Jeniston
2019-08-16 21:17         ` Thomas Gleixner
     [not found]           ` <CACAVd4hT6QYtgtDsBcgy7c_s9WVBAH+1m0r5geBe7BUWJWYhbA@mail.gmail.com>
2019-08-17 19:23             ` Thomas Gleixner
2019-08-19  6:07               ` Arul Jeniston
2019-08-19  8:04                 ` Thomas Gleixner
2019-08-19 14:25                   ` Arul Jeniston
2019-08-19 14:52                     ` Thomas Gleixner
2019-08-19 15:26                       ` Arul Jeniston
2019-08-19 15:59                         ` Thomas Gleixner
     [not found]                           ` <CACAVd4iRN7=eq_B1+Yb-xcspU-Sg1dmMo_=VtLXXVPkjN1hY5Q@mail.gmail.com>
2019-08-19 18:29                             ` Thomas Gleixner
     [not found]                               ` <CACAVd4jAJ5QcOH=q=Q9kAz20X4_nAc7=vVU_gPWTS1UuiGK-fg@mail.gmail.com>
2019-08-20  8:49                                 ` Thomas Gleixner
2019-08-20  9:42                                   ` Arul Jeniston
2019-09-05  8:48                                     ` Arul Jeniston
2019-09-05 15:34                                       ` Thomas Gleixner
2019-09-06 16:36                                         ` Arul Jeniston
2019-09-07 14:38                                           ` Thomas Gleixner
2019-11-05  5:01                                             ` Arul Jeniston
2019-11-05 10:01                                               ` Thomas Gleixner
2019-11-06  3:38                                                 ` Arul Jeniston

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACAVd4h05P2tWb7Eh1+3_0Cm7MkDNAt+SJVoBT4gErBfsBmsAQ@mail.gmail.com \
    --to=arul.jeniston@gmail.com \
    --cc=arul_mc@dell.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git