[SLOF] [PATCH] rtas-nvram: optimize erase

[Nikunj A Dadhania]

Thomas Huth <thuth at redhat.com> writes:

> On 05.05.2016 08:13, Nikunj A Dadhania wrote:
>> As this was done at byte granularity, erasing complete nvram(64K
>> default) took a lot of time. To reduce the number of rtas call per byte
>> write which is expensive, the erase is done in a block of 1024.
>> 
>> After this patch there is ~450msec improvement during boot. Default qemu
>> booting does not provide file backed nvram, so every boot there would be
>> full erase of 64K.
>
> Wow, that's a pretty good improvement!
>
>> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
>> ---
>>  lib/libnvram/nvram.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/lib/libnvram/nvram.c b/lib/libnvram/nvram.c
>> index 473814e..5a895ee 100644
>> --- a/lib/libnvram/nvram.c
>> +++ b/lib/libnvram/nvram.c
>> @@ -80,6 +80,8 @@ static volatile uint8_t nvram[NVRAM_LENGTH]; /* FAKE */
>>  
>>  #elif defined(RTAS_NVRAM)
>>  
>> +#define RTAS_ERASE_BUF_SZ 1024
>> +unsigned char erase_buf[RTAS_ERASE_BUF_SZ] = {0};
>
> No need for the "= {0}" here since you memset() the buffer later anyway.
>
> Actually, could you maybe move this buffer into the nvram_fetch()
> function so that it gets a stack variable, so we can save this memory
> from the global space? It should work, at least if you'd decrease
> RTAS_ERASE_BUF_SZ to 512, since this is the size that is used in
> fast_rfill() for a local array, too.
>
>>  static inline void nvram_fetch(unsigned int offset, void *buf, unsigned int len)
>>  {
>>   	struct hv_rtas_call rtas = {
>> @@ -372,9 +374,18 @@ partition_t get_partition_fs(char *name, int namelen)
>>  void erase_nvram(int offset, int len)
>>  {
>>  	int i;
>> +#ifdef RTAS_NVRAM
>> +	int chunk;
>>  
>> +	memset(erase_buf, 0, RTAS_ERASE_BUF_SZ);
>> +	for (i = len; i > 0; i -= RTAS_ERASE_BUF_SZ, offset += RTAS_ERASE_BUF_SZ) {
>> +		chunk = (i > RTAS_ERASE_BUF_SZ)? RTAS_ERASE_BUF_SZ : i;
>> +		nvram_store(offset, erase_buf, chunk);
>> +	}
>> +#else
>>  	for (i=offset; i<offset+len; i++)
>>  		nvram_write_byte(i, 0);
>> +#endif
>>  }
>
> Yet another idea: There seems to be buffer called "nvram_buffer"
> available in nvram.c already, which can be used as scratch space?
> (I hope I understood that code right...)

Yes, I have cross checked, board-qemu/slof/rtas-nvram.fs allocates the
memory equal to nvram_size. This is then initialized via nvram_init
call, that sets both nvram_buffer and NVRAM_LENGTH.

> So maybe that buffer could be used to clear the whole area at once?
> Something like:
>
> void erase_nvram(int offset, int len)
> {
>  	int i;
>
> #ifdef RTAS_NVRAM
> 	uint8_t *erase_buf = get_nvram_buffer(len);
> 	if (erase_buf) {
> 		/* Speed up by erasing all memory at once */
> 		memset(erase_buf, 0, len);
> 		nvram_store(offset, erase_buf, len);
> 		free_nvram_buffer(erase_buf);
> 		return;
> 	}
> 	/* If get_nvram_buffer failed, fall through to default code */
> #endif
>
> 	for (i=offset; i<offset+len; i++)
> 		nvram_write_byte(i, 0);
> }
>
> That whole concept with the nvram_buffer looks a little bit badly
> documented to me, but as far as I understood the code, it could work?

Yes,should work, will post the patch after testing.

Regards
Nikunj