Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Wed, 4 Oct 2023 14:39:32 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org, Todd Brandt <todd.e.brandt@intel.com>
Subject: Re: [PATCH printk] printk: flush consoles before checking progress
Message-ID: <ZR1dBAOMmMUNgY6Q@alley>
References: <20230929113233.863824-1-john.ogness@linutronix.de>
 <ZRrAFS3cELj1DDN2@alley>
 <87h6n64rcs.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87h6n64rcs.fsf@jogness.linutronix.de>
Precedence: bulk

On Wed 2023-10-04 12:31:07, John Ogness wrote:
> On 2023-10-02, Petr Mladek <pmladek@suse.com> wrote:
> > I was about to push this patch and ran checkpatch.pl. It warned about
> >
> > WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.rst
> > #73: FILE: kernel/printk/printk.c:3782:
> > +               msleep(1);
> >
> > And indeed, Documentation/timers/timers-howto.rst says that msleep()
> > might sleep longer that expected for <20ms delays. I guess that
> > it is somehow related to jiffies, HZ, and load on the system.
> >
> > I think that we need to count jiffies here.
> 
> Agreed. The @timeout_ms parameter should be respected.
> 
> > Something like:
> >
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index db81b68d7f14..6ea500d95fd9 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3772,24 +3773,19 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> >  		console_srcu_read_unlock(cookie);
> >  
> >  		if (diff != last_diff && reset_on_progress)
> > -			remaining = timeout_ms;
> > +			timeout_end = jiffies + timeout_jiffies;
> >  
> >  		console_unlock();
> >  
> >  		/* Note: @diff is 0 if there are no usable consoles. */
> > -		if (diff == 0 || remaining == 0)
> > +		if (diff == 0)
> >  			break;
> >  
> > -		if (remaining < 0) {
> > -			/* no timeout limit */
> > -			msleep(100);
> > -		} else if (remaining < 100) {
> > -			msleep(remaining);
> > -			remaining = 0;
> > -		} else {
> > -			msleep(100);
> > -			remaining -= 100;
> > -		}
> > +		/* Negative timeout means an infinite wait. */
> > +		if (timeout_ms >= 0 && time_after_eq(jiffies, timeout_end))
> > +			break;
> > +
> > +		msleep(2000 / HZ);
> 
> Is there really any advantage to this? I would just do msleep(1) and let
> msleep round up. Everything else (tracking via jiffies) looks fine to me.

It was attempt to synchronize it with a scheduler tick. I saw it somewhere ;-)
But you are right. Let's keep it simple and use msleep(1).


> >  		last_diff = diff;
> >  	}
> >
> > And we should do this in a separate patch. It seems that sleeping
> > is a bigger magic than I expected.
> 
> Agreed.

Are you going to prepare them or should I?

Best Regards,
Petr