nand_base: optimize checking of erased buffers

Submitted by Pavel Machek on April 21, 2017, 10:51 a.m.

Details

Message ID 20170421105107.GA7259@amd
State Accepted
Delegated to: Boris Brezillon
Headers show

Commit Message

Pavel Machek April 21, 2017, 10:51 a.m.
If we see ~0UL in flash, there's no need for hweight, and no need to
check number of bitflips. So this should be net win.

Signed-off-by: Pavel Machek <pavel@denx.de>

Comments

Mason May 17, 2017, 11:27 a.m.
On 21/04/2017 12:51, Pavel Machek wrote:

> If we see ~0UL in flash, there's no need for hweight, and no need to
> check number of bitflips. So this should be net win.
> 
> Signed-off-by: Pavel Machek <pavel@denx.de>
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index b0524f8..96c27ec 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1357,7 +1357,10 @@ static int nand_check_erased_buf(void *buf, int len, int bitflips_threshold)
>  
>  	for (; len >= sizeof(long);
>  	     len -= sizeof(long), bitmap += sizeof(long)) {
> -		weight = hweight_long(*((unsigned long *)bitmap));

I hadn't noticed this earlier. There is, obviously, an implicit
requirement that 'buf' must be 4-byte aligned on 32-bit platforms,
and 8-byte aligned on 64-bit platforms.

This is not true for my platform, as the ecc pointer is
chip->oob_poi + 10

I suppose it's not a problem if the platform can handle
unaligned loads, otherwise it spells trouble.


> +		unsigned long d = *((unsigned long *)bitmap);
> +		if (d == ~0UL)
> +			continue;
> +		weight = hweight_long(d);
>  		bitflips += BITS_PER_LONG - weight;
>  		if (unlikely(bitflips > bitflips_threshold))
>  			return -EBADMSG;
> 

The optimization makes sense in itself, but given that it's on an
error path (?) I'm not sure it will bring any tangible benefits?

Regards.
Mason May 17, 2017, 11:39 a.m.
On 17/05/2017 13:27, Mason wrote:

> On 21/04/2017 12:51, Pavel Machek wrote:
> 
>> If we see ~0UL in flash, there's no need for hweight, and no need to
>> check number of bitflips. So this should be net win.
>>
>> Signed-off-by: Pavel Machek <pavel@denx.de>
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index b0524f8..96c27ec 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -1357,7 +1357,10 @@ static int nand_check_erased_buf(void *buf, int len, int bitflips_threshold)
>>  
>>  	for (; len >= sizeof(long);
>>  	     len -= sizeof(long), bitmap += sizeof(long)) {
>> -		weight = hweight_long(*((unsigned long *)bitmap));
> 
> I hadn't noticed this earlier. There is, obviously, an implicit
> requirement that 'buf' must be 4-byte aligned on 32-bit platforms,
> and 8-byte aligned on 64-bit platforms.
> 
> This is not true for my platform, as the ecc pointer is
> chip->oob_poi + 10

Doh! As Boris points out, the prologue/epilogue handle
all alignment & size issues.

Regards.
Pavel Machek May 17, 2017, 11:52 a.m.
> This is not true for my platform, as the ecc pointer is
> chip->oob_poi + 10
> 
> I suppose it's not a problem if the platform can handle
> unaligned loads, otherwise it spells trouble.
> 
> 
> > +		unsigned long d = *((unsigned long *)bitmap);
> > +		if (d == ~0UL)
> > +			continue;
> > +		weight = hweight_long(d);
> >  		bitflips += BITS_PER_LONG - weight;
> >  		if (unlikely(bitflips > bitflips_threshold))
> >  			return -EBADMSG;
> > 
> 
> The optimization makes sense in itself, but given that it's on an
> error path (?) I'm not sure it will bring any tangible benefits?

Well, if it is critical enough to do magic it does, it is also
important enough not to do expensive bit-counting needlessly...

Patch hide | download patch | download mbox

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b0524f8..96c27ec 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1357,7 +1357,10 @@  static int nand_check_erased_buf(void *buf, int len, int bitflips_threshold)
 
 	for (; len >= sizeof(long);
 	     len -= sizeof(long), bitmap += sizeof(long)) {
-		weight = hweight_long(*((unsigned long *)bitmap));
+		unsigned long d = *((unsigned long *)bitmap);
+		if (d == ~0UL)
+			continue;
+		weight = hweight_long(d);
 		bitflips += BITS_PER_LONG - weight;
 		if (unlikely(bitflips > bitflips_threshold))
 			return -EBADMSG;