Message-ID: <cf106f99b8794bb38c16c7a5a83e128b@de.bosch.com>
Date:   Mon, 6 Aug 2018 11:39:32 +0000
From:   "Jonas Mark (BT-FIR/ENG1)" <Mark.Jonas@...bosch.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Bartosz Golaszewski <brgl@...ev.pl>
CC:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
        "WANG Xin (BT-FIR/ENG1-Zhu)" <Xin.Wang7@...bosch.com>,
        "Jonas Mark (BT-FIR/ENG1)" <Mark.Jonas@...bosch.com>
Subject: Re: [PATCH] eeprom: at24: Fix unexpected timeout under high load

Hi Andy,

> >> > -#define at24_loop_until_timeout(tout, op_time)                         \
> >> > -       for (tout = jiffies + msecs_to_jiffies(at24_write_timeout),     \
> >> > -            op_time = 0;                                               \
> >> > -            op_time ? time_before(op_time, tout) : true;               \
> >> > -            usleep_range(1000, 1500), op_time = jiffies)
> >>
> >> This one understandble and represents one operation.
> >
> > It just has the downside that it will not retry if the timeout is
> > reached after the usleep_range().
> >
> > If you have a system which combines high CPU load with repeated
> EEPROM
> > writes you will run into the following scenario:
> >
> > - System makes a successful regmap_bulk_write() to EEPROM.
> > - System wants to perform another write to EEPROM but EEPROM is still
> >   busy with the last write.
> > - Because of high CPU load the usleep_range() will sleep more than
> >   25 ms (at24_write_timeout).
> > - Within the over-long sleeping the EEPROM finished the previous write
> >   operation and is ready again.
> > - at24_loop_until_timeout() will detect timeout and won't try to write.
> 
> >
> > The scenario above happens very often on our system and we need a fix.
> 
> Thanks for explanation why. (it would be good partially move this to
> the commit message).

We will improve the commit message in the next revision of the patch.

> >> > +#define at24_loop_until_timeout_begin(tout, op_time)           \
> >> > +       tout = jiffies + msecs_to_jiffies(at24_write_timeout);  \
> >> > +       while (true) {                                          \
> >> > +               op_time = jiffies;
> >> > +
> >> > +#define at24_loop_until_timeout_end(tout, op_time)             \
> >> > +               if (time_before(tout, op_time))                 \
> >> > +                       break;                                  \
> >> > +               usleep_range(1000, 1500);                       \
> >> > +       }
> >>
> >> Besides `while (true)`, which is a red flag for timeout loops,
> >> these are done in an hack way. Just open code them in both cases, or
> >> rewrite original one to keel it's semantics.
> >
> > I have to admit that I am not sure what you mean.
> >
> > We kept the macro-style of the loop because we assumed it was good
> > style in this context.
> 
> No way. It's a bad style when you have a macro like you proposing. It
> would give you a bottle of sparkling bugs.
> 
> > What does "keel it's semantics" mean?
> 
> Macro should be standalone piece of code which does something from A
> to Z, not from A-K when you need a complementary macro to do L-Z
> parts.

I agree, we will do it without a macro then.

> > With "open code them in both cases" do you mean to rid of the macro
> > and to directly write the loop into the code? Does the following
> > match your proposals?
> >
> > ret = 0;
> > tout = jiffies + msecs_to_jiffies(at24_write_timeout);
> > do {
> >         if (ret)
> >                 usleep_range(1000, 1500);
> >
> >         read_time = jiffies;
> >
> >         ret = regmap_bulk_read(regmap, offset, buf, count);
> >         dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n",
> >                 count, offset, ret, jiffies);
> >         if (!ret)
> >                 return count;
> > } while (!time_before(tout, read_time))
> 
> Yes, though, please, look at the examples in the existing code and
> make it slightly better
> 
> timeout = ...
> do {
> ret = ...
> if (ret) // or if (!ret)
>  ...
> 
> usleep_range(...);
> } while(time_before(...));

When working on the problem we had an intermediate result were we
came to the same solution as your proposal showed:

tout = jiffies + msecs_to_jiffies(at24_write_timeout);
do {
         read_time = jiffies;

         ret = regmap_bulk_read(regmap, offset, buf, count);
         dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n",
                 count, offset, ret, jiffies);
         if (!ret)
                 return count;

         usleep_range(1000, 1500);
} while (!time_before(tout, read_time))

The advantage of this code is that the usleep_range() is unconditional.

(In my older proposal a "if (ret)" condition is required to make sure
that there is not sleep at the very first iteration but only at
follow-up iterations where regmap_bulk_read() failed.)

The disadvantage of the new proposal is that in case of a timeout one
more unnecessary sleep is made. Is that acceptable?

An alternative would be to duplicate the regmap_bulk_read() and the
debugging code outside the loop.

tout = jiffies + msecs_to_jiffies(at24_write_timeout);
read_time = jiffies;

ret = regmap_bulk_read(regmap, offset, buf, count);
dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n",
	count, offset, ret, jiffies);

while (ret && !time_before(tout, read_time))
	usleep_range(1000, 1500);

	/*
	 * The timestamp shall be taken before sleep and the actual
	 * operation to avoid a premature timeout in case of high CPU load.
	 */
	read_time = jiffies;

	ret = regmap_bulk_read(regmap, offset, buf, count);
	dev_dbg(&client->dev, "read retry %zu@%d --> %d (%ld)\n",
		count, offset, ret, jiffies);
}

if (!ret)
	return count;

Is this preferable?

Greetings,
Mark

Building Technologies, Panel Software Fire (BT-FIR/ENG1) 
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com

Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118 
Aufsichtsratsvorsitzender: Stefan Hartung; Geschäftsführung: Tanja Rückert, Andreas Bartz, Thomas Quante, Bernhard Schuster

Powered by blists - more mailing lists