[PATCH v2] UBI: only read necessary size when reading the VID header

[PATCH v2] UBI: only read necessary size when reading the VID header

[Sascha Hauer]

When reading the vid hdr from the device UBI always reads a whole
page. Instead, read only the data we actually need and speed up
attachment of UBI devices by potentially making use of reading
subpages if the NAND driver supports it.

Since the VID header may be at offset vid_hdr_shift in the page and
we can only read from the beginning of a page we have to add that
offset to the read size.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---

change since v1:
- properly handle vid_hdr_shift != 0

 drivers/mtd/ubi/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index 10cf3b5..ff8cafe 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -1019,7 +1019,7 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
 
 	p = (char *)vid_hdr - ubi->vid_hdr_shift;
 	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
-			  ubi->vid_hdr_alsize);
+			  ubi->vid_hdr_shift + UBI_VID_HDR_SIZE);
 	if (read_err && read_err != UBI_IO_BITFLIPS && !mtd_is_eccerr(read_err))
 		return read_err;
 
-- 
2.8.1

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Richard Weinberger]

Am 28.06.2016 um 13:51 schrieb Sascha Hauer:
> When reading the vid hdr from the device UBI always reads a whole
> page. Instead, read only the data we actually need and speed up
> attachment of UBI devices by potentially making use of reading
> subpages if the NAND driver supports it.
> 
> Since the VID header may be at offset vid_hdr_shift in the page and
> we can only read from the beginning of a page we have to add that
> offset to the read size.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Looks good, queued for v4.8. :-)

Thanks,
//richard

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Artem Bityutskiy]

On Tue, 2016-06-28 at 13:51 +0200, Sascha Hauer wrote:
> When reading the vid hdr from the device UBI always reads a whole
> page. Instead, read only the data we actually need and speed up
> attachment of UBI devices by potentially making use of reading
> subpages if the NAND driver supports it.
> 
> Since the VID header may be at offset vid_hdr_shift in the page and
> we can only read from the beginning of a page we have to add that
> offset to the read size.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> 
> change since v1:
> - properly handle vid_hdr_shift != 0
> 
>  drivers/mtd/ubi/io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index 10cf3b5..ff8cafe 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -1019,7 +1019,7 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi,
> int pnum,
>  
>  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
>  	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
> -			  ubi->vid_hdr_alsize);
> +			  ubi->vid_hdr_shift + UBI_VID_HDR_SIZE);

There are pages and subpages underneath. Let me use old sizes. Say,
2KiB pages, and 512KiB subpages.

If the VID header is in the first subpage, the MTD level probably needs
to read the entire subpage anyway, because of per-subpage ECC. This is
why we give it a buffer of subpage size.

Now if you give it a buffer of smaller size, say, 256 bytes, MTD will
have to read the subpage into its own buffer first, validate ECC, then
copy 256 bytes from the internal buffer to your buffer. Compare this
with MTD copying data directly to your buffer.

This was the original idea, and there was measurable difference on real
setups. So this is actually a read speed optimization for those old
setups.

Therefore, unless I misunderstood this patch - it introduces a
regression to those old setups at least (forces MTD to use an
intermediate buffer rather than copy data from NAND directly to the
buffer supplied by UBI)

Now, one can argue this depends on how the underlying MTD driver works,
this is true. One may argue that UBI should not make assumptions about
the aspects of the MTD level like that - true as well. But that would
need a bit better fix then.

I am inclined to Nack, but I feel like I may miss something, so just
letting you know instead.

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Richard Weinberger]

Am 28.06.2016 um 15:00 schrieb Artem Bityutskiy:
> On Tue, 2016-06-28 at 13:51 +0200, Sascha Hauer wrote:
>> When reading the vid hdr from the device UBI always reads a whole
>> page. Instead, read only the data we actually need and speed up
>> attachment of UBI devices by potentially making use of reading
>> subpages if the NAND driver supports it.
>>
>> Since the VID header may be at offset vid_hdr_shift in the page and
>> we can only read from the beginning of a page we have to add that
>> offset to the read size.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> ---
>>
>> change since v1:
>> - properly handle vid_hdr_shift != 0
>>
>>  drivers/mtd/ubi/io.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
>> index 10cf3b5..ff8cafe 100644
>> --- a/drivers/mtd/ubi/io.c
>> +++ b/drivers/mtd/ubi/io.c
>> @@ -1019,7 +1019,7 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi,
>> int pnum,
>>  
>>  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
>>  	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
>> -			  ubi->vid_hdr_alsize);
>> +			  ubi->vid_hdr_shift + UBI_VID_HDR_SIZE);
> 
> There are pages and subpages underneath. Let me use old sizes. Say,
> 2KiB pages, and 512KiB subpages.
> 
> If the VID header is in the first subpage, the MTD level probably needs
> to read the entire subpage anyway, because of per-subpage ECC. This is
> why we give it a buffer of subpage size.
> 
> Now if you give it a buffer of smaller size, say, 256 bytes, MTD will
> have to read the subpage into its own buffer first, validate ECC, then
> copy 256 bytes from the internal buffer to your buffer. Compare this
> with MTD copying data directly to your buffer.
> 
> This was the original idea, and there was measurable difference on real
> setups. So this is actually a read speed optimization for those old
> setups.
> 
> Therefore, unless I misunderstood this patch - it introduces a
> regression to those old setups at least (forces MTD to use an
> intermediate buffer rather than copy data from NAND directly to the
> buffer supplied by UBI)
> 
> Now, one can argue this depends on how the underlying MTD driver works,
> this is true. One may argue that UBI should not make assumptions about
> the aspects of the MTD level like that - true as well. But that would
> need a bit better fix then.
> 
> I am inclined to Nack, but I feel like I may miss something, so just
> letting you know instead.

Artem,

you raise an interesting point.
So, we need to analyze this more before it can be merged.

Thanks,
//richard

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Sascha Hauer]

On Tue, Jun 28, 2016 at 04:00:29PM +0300, Artem Bityutskiy wrote:
> On Tue, 2016-06-28 at 13:51 +0200, Sascha Hauer wrote:
> > When reading the vid hdr from the device UBI always reads a whole
> > page. Instead, read only the data we actually need and speed up
> > attachment of UBI devices by potentially making use of reading
> > subpages if the NAND driver supports it.
> > 
> > Since the VID header may be at offset vid_hdr_shift in the page and
> > we can only read from the beginning of a page we have to add that
> > offset to the read size.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> > 
> > change since v1:
> > - properly handle vid_hdr_shift != 0
> > 
> >  drivers/mtd/ubi/io.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> > index 10cf3b5..ff8cafe 100644
> > --- a/drivers/mtd/ubi/io.c
> > +++ b/drivers/mtd/ubi/io.c
> > @@ -1019,7 +1019,7 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi,
> > int pnum,
> >  
> >  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
> >  	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
> > -			  ubi->vid_hdr_alsize);
> > +			  ubi->vid_hdr_shift + UBI_VID_HDR_SIZE);
> 
> There are pages and subpages underneath. Let me use old sizes. Say,
> 2KiB pages, and 512KiB subpages.
> 
> If the VID header is in the first subpage, the MTD level probably needs
> to read the entire subpage anyway, because of per-subpage ECC. This is
> why we give it a buffer of subpage size.

The gpmi NAND driver allows reading subpages (NAND_SUBPAGE_READ is set), but
it does not allow writing subpages (NAND_NO_SUBPAGE_WRITE is also set).
With this mtd->subpage_sft becomes 0 and ubi->hdrs_min_io_size becomes
the full page size.
UBI does not distinguish between the minimum read size and the minimum
write size. We could pass this information from mtd to UBI, then UBI
could make use of it.

> 
> Now if you give it a buffer of smaller size, say, 256 bytes, MTD will
> have to read the subpage into its own buffer first, validate ECC, then
> copy 256 bytes from the internal buffer to your buffer. Compare this
> with MTD copying data directly to your buffer.

BTW ubi_io_read_ec_hdr() also reads non subpage aligned sizes when
reading the EC header, so at least this function has the right default
for my setup ;)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Artem Bityutskiy]

On Tue, 2016-06-28 at 16:05 +0200, Sascha Hauer wrote:
> On Tue, Jun 28, 2016 at 04:00:29PM +0300, Artem Bityutskiy wrote:
> > On Tue, 2016-06-28 at 13:51 +0200, Sascha Hauer wrote:
> > > When reading the vid hdr from the device UBI always reads a whole
> > > page. Instead, read only the data we actually need and speed up
> > > attachment of UBI devices by potentially making use of reading
> > > subpages if the NAND driver supports it.
> > > 
> > > Since the VID header may be at offset vid_hdr_shift in the page
> > > and
> > > we can only read from the beginning of a page we have to add that
> > > offset to the read size.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > > 
> > > change since v1:
> > > - properly handle vid_hdr_shift != 0
> > > 
> > >  drivers/mtd/ubi/io.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> > > index 10cf3b5..ff8cafe 100644
> > > --- a/drivers/mtd/ubi/io.c
> > > +++ b/drivers/mtd/ubi/io.c
> > > @@ -1019,7 +1019,7 @@ int ubi_io_read_vid_hdr(struct ubi_device
> > > *ubi,
> > > int pnum,
> > >  
> > >  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
> > >  	read_err = ubi_io_read(ubi, p, pnum, ubi-
> > > >vid_hdr_aloffset,
> > > -			  ubi->vid_hdr_alsize);
> > > +			  ubi->vid_hdr_shift +
> > > UBI_VID_HDR_SIZE);
> > 
> > There are pages and subpages underneath. Let me use old sizes. Say,
> > 2KiB pages, and 512KiB subpages.
> > 
> > If the VID header is in the first subpage, the MTD level probably
> > needs
> > to read the entire subpage anyway, because of per-subpage ECC. This
> > is
> > why we give it a buffer of subpage size.
> 
> The gpmi NAND driver allows reading subpages (NAND_SUBPAGE_READ is
> set), but
> it does not allow writing subpages (NAND_NO_SUBPAGE_WRITE is also
> set).

OK. In the generic code (nand_base.c) though you can find that if the
read request is for entire page or subpage, then the data is transfared
from the chip to the supplied buffer, ECC is verified, and the data are
returned. If the read request is for a fraction of a page or subpage,
then the data are transferred from the chip to an internal buffer, ECC
is verified, and then the required piece of data are copied from the
internal buffer to the supplied buffer. I.e., more memory copy
operations.

The code is very twisted, but I think the logic is in
'nand_do_read_ops()', you'll notice that in the "not aligned" case the
special 'chip->buffers->databuf' is used, otherwise the buffer supplied
by UBI is used.

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Brian Norris]

Hi,

On Tue, Jun 28, 2016 at 05:54:08PM +0300, Artem Bityutskiy wrote:
> On Tue, 2016-06-28 at 16:05 +0200, Sascha Hauer wrote:
> > The gpmi NAND driver allows reading subpages (NAND_SUBPAGE_READ is
> > set), but
> > it does not allow writing subpages (NAND_NO_SUBPAGE_WRITE is also
> > set).
> 
> OK. In the generic code (nand_base.c) though you can find that if the
> read request is for entire page or subpage, then the data is transfared
> from the chip to the supplied buffer, ECC is verified, and the data are
> returned. If the read request is for a fraction of a page or subpage,
> then the data are transferred from the chip to an internal buffer, ECC
> is verified, and then the required piece of data are copied from the
> internal buffer to the supplied buffer. I.e., more memory copy
> operations.
> 
> The code is very twisted, but I think the logic is in
> 'nand_do_read_ops()', you'll notice that in the "not aligned" case the
> special 'chip->buffers->databuf' is used, otherwise the buffer supplied
> by UBI is used.

Note that "not aligned" means "not aligned to mtd->writesize", and that
writesize is NOT the subpage size. So if some controller+flash+driver
actually supports subpage writes (there are few of these), then UBI
might already be requesting sub-page reads, and those reads are still
memcpy()'d in nand_do_read_ops(). This doesn't change the argument too
much, I suppose; it just means we have a bug in nand_do_read_ops() I
guess.

That's also beside the point for Sascha's case, if he's looking at
gpmi-nand (which does not support subpage writes).

Brian

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Boris Brezillon]

On Tue, 28 Jun 2016 10:46:38 -0700
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi,
> 
> On Tue, Jun 28, 2016 at 05:54:08PM +0300, Artem Bityutskiy wrote:
> > On Tue, 2016-06-28 at 16:05 +0200, Sascha Hauer wrote:  
> > > The gpmi NAND driver allows reading subpages (NAND_SUBPAGE_READ is
> > > set), but
> > > it does not allow writing subpages (NAND_NO_SUBPAGE_WRITE is also
> > > set).  
> > 
> > OK. In the generic code (nand_base.c) though you can find that if the
> > read request is for entire page or subpage, then the data is transfared
> > from the chip to the supplied buffer, ECC is verified, and the data are
> > returned. If the read request is for a fraction of a page or subpage,
> > then the data are transferred from the chip to an internal buffer, ECC
> > is verified, and then the required piece of data are copied from the
> > internal buffer to the supplied buffer. I.e., more memory copy
> > operations.
> > 
> > The code is very twisted, but I think the logic is in
> > 'nand_do_read_ops()', you'll notice that in the "not aligned" case the
> > special 'chip->buffers->databuf' is used, otherwise the buffer supplied
> > by UBI is used.  
> 
> Note that "not aligned" means "not aligned to mtd->writesize", and that
> writesize is NOT the subpage size. So if some controller+flash+driver
> actually supports subpage writes (there are few of these), then UBI
> might already be requesting sub-page reads, and those reads are still
> memcpy()'d in nand_do_read_ops(). This doesn't change the argument too
> much, I suppose; it just means we have a bug in nand_do_read_ops() I
> guess.

Is this really a bug, or just a sub-optimal implementation?

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Brian Norris]

On Tue, Jun 28, 2016 at 04:05:00PM +0200, Sascha Hauer wrote:
> On Tue, Jun 28, 2016 at 04:00:29PM +0300, Artem Bityutskiy wrote:
> > If the VID header is in the first subpage, the MTD level probably needs
> > to read the entire subpage anyway, because of per-subpage ECC. This is
> > why we give it a buffer of subpage size.
> 
> The gpmi NAND driver allows reading subpages (NAND_SUBPAGE_READ is set), but
> it does not allow writing subpages (NAND_NO_SUBPAGE_WRITE is also set).
> With this mtd->subpage_sft becomes 0 and ubi->hdrs_min_io_size becomes
> the full page size.
> UBI does not distinguish between the minimum read size and the minimum
> write size. We could pass this information from mtd to UBI, then UBI
> could make use of it.

In reading Artem's first objection, I was noticing the same thing and
came to a similar conclusion. I guess we can't really get by with UBI
just guessing at the ideal read size. Right now, it gets it too large
most of the time, since often, NAND controllers can support sub-page
reads, but not sub-page writes (or even if the controller can, the flash
can't -- like all MLC or even many modern SLC), so a single number
(writesize >> subpage_sf) is not sufficient.

Brian

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Artem Bityutskiy]

On Tue, 2016-06-28 at 10:49 -0700, Brian Norris wrote:
> In reading Artem's first objection, I was noticing the same thing and
> came to a similar conclusion. I guess we can't really get by with UBI
> just guessing at the ideal read size. Right now, it gets it too large
> most of the time, since often, NAND controllers can support sub-page
> reads, but not sub-page writes (or even if the controller can, the
> flash
> can't -- like all MLC or even many modern SLC), so a single number
> (writesize >> subpage_sf) is not sufficient.

Yes, this is a good opportunity for speed optimizations indeed.

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Brian Norris]

Hi Artem,

I'll comment on the other branches of this thread, but one thing here:

On Tue, Jun 28, 2016 at 04:00:29PM +0300, Artem Bityutskiy wrote:
> Therefore, unless I misunderstood this patch - it introduces a
> regression to those old setups at least (forces MTD to use an
> intermediate buffer rather than copy data from NAND directly to the
> buffer supplied by UBI)

It's really a balance between speed of the flash and speed of the
memcpy(). I believe Boris may have benchmarked some of this recently,
but I'm really inclined to believe that reading several times as much as
you need from flash is much worse than doing some extra memcpy(). So
even if we introduce an extra memcpy(), it might still be worth it to
save the extra wait-for-flash time.

Intuitively, I expect that these days, the I/O time is much more
significant than any memcpy().

Brian

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Artem Bityutskiy]

On Tue, 2016-06-28 at 10:43 -0700, Brian Norris wrote:
> Hi Artem,
> 
> I'll comment on the other branches of this thread, but one thing
> here:
> 
> On Tue, Jun 28, 2016 at 04:00:29PM +0300, Artem Bityutskiy wrote:
> > Therefore, unless I misunderstood this patch - it introduces a
> > regression to those old setups at least (forces MTD to use an
> > intermediate buffer rather than copy data from NAND directly to the
> > buffer supplied by UBI)
> 
> It's really a balance between speed of the flash and speed of the
> memcpy().

Sure.

>  I believe Boris may have benchmarked some of this recently,
> but I'm really inclined to believe that reading several times as much
> as
> you need from flash is much worse than doing some extra memcpy().

That's probably true.

>  So
> even if we introduce an extra memcpy(), it might still be worth it to
> save the extra wait-for-flash time.

Right.

> Intuitively, I expect that these days, the I/O time is much more
> significant than any memcpy().

All good points. Besides indeed in case of the subpage the memcpy() is
present anyway for (for unexpected reasons).

So yeah, I think the concern I rose is a non-issue and we could proceed
with Sascha's patch. Thanks!

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Boris Brezillon]

On Wed, 29 Jun 2016 06:49:48 +0300
Artem Bityutskiy <dedekind1@gmail.com> wrote:

> On Tue, 2016-06-28 at 10:43 -0700, Brian Norris wrote:
> > Hi Artem,
> > 
> > I'll comment on the other branches of this thread, but one thing
> > here:
> > 
> > On Tue, Jun 28, 2016 at 04:00:29PM +0300, Artem Bityutskiy wrote:  
> > > Therefore, unless I misunderstood this patch - it introduces a
> > > regression to those old setups at least (forces MTD to use an
> > > intermediate buffer rather than copy data from NAND directly to the
> > > buffer supplied by UBI)  
> > 
> > It's really a balance between speed of the flash and speed of the
> > memcpy().  
> 
> Sure.
> 
> >  I believe Boris may have benchmarked some of this recently,
> > but I'm really inclined to believe that reading several times as much
> > as
> > you need from flash is much worse than doing some extra memcpy().  
> 
> That's probably true.
> 
> >  So
> > even if we introduce an extra memcpy(), it might still be worth it to
> > save the extra wait-for-flash time.  
> 
> Right.
> 
> > Intuitively, I expect that these days, the I/O time is much more
> > significant than any memcpy().  
> 
> All good points. Besides indeed in case of the subpage the memcpy() is
> present anyway for (for unexpected reasons).
> 
> So yeah, I think the concern I rose is a non-issue and we could proceed
> with Sascha's patch. Thanks!

I see I don't have to convince you with real numbers, but, as pointed
by Brian, memcpy() is indeed way faster than NAND I/Os (and ECC
correction steps).

Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Richard Weinberger]

Am 04.07.2016 um 11:38 schrieb Boris Brezillon:
> On Wed, 29 Jun 2016 06:49:48 +0300
> Artem Bityutskiy <dedekind1@gmail.com> wrote:
> 
>> On Tue, 2016-06-28 at 10:43 -0700, Brian Norris wrote:
>>> Hi Artem,
>>>
>>> I'll comment on the other branches of this thread, but one thing
>>> here:
>>>
>>> On Tue, Jun 28, 2016 at 04:00:29PM +0300, Artem Bityutskiy wrote:  
>>>> Therefore, unless I misunderstood this patch - it introduces a
>>>> regression to those old setups at least (forces MTD to use an
>>>> intermediate buffer rather than copy data from NAND directly to the
>>>> buffer supplied by UBI)  
>>>
>>> It's really a balance between speed of the flash and speed of the
>>> memcpy().  
>>
>> Sure.
>>
>>>  I believe Boris may have benchmarked some of this recently,
>>> but I'm really inclined to believe that reading several times as much
>>> as
>>> you need from flash is much worse than doing some extra memcpy().  
>>
>> That's probably true.
>>
>>>  So
>>> even if we introduce an extra memcpy(), it might still be worth it to
>>> save the extra wait-for-flash time.  
>>
>> Right.
>>
>>> Intuitively, I expect that these days, the I/O time is much more
>>> significant than any memcpy().  
>>
>> All good points. Besides indeed in case of the subpage the memcpy() is
>> present anyway for (for unexpected reasons).
>>
>> So yeah, I think the concern I rose is a non-issue and we could proceed
>> with Sascha's patch. Thanks!
> 
> I see I don't have to convince you with real numbers, but, as pointed
> by Brian, memcpy() is indeed way faster than NAND I/Os (and ECC
> correction steps).

Thanks for all the input! :-)
I'll merge this patch in 4.8.

Thanks,
//richard