base: increase catpad buffer

Message ID 20171128073809.15786-1-nikunj@linux.vnet.ibm.com
State Superseded
Headers show
Series
  • base: increase catpad buffer
Related show

Commit Message

Nikunj A Dadhania Nov. 28, 2017, 7:38 a.m.
Current buffer of 1K is not sufficient, and causes exception if more than 20
devices are used with bootindex. Increase the buffer size to 16K

Reported here: https://github.com/qemu/SLOF/issues/3

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

---

qemu-system-ppc64 -nographic -nodefaults -serial stdio -monitor pty -m 2G \
-device virtio-scsi-pci    \
`for ((i=2;i<=50;i++)) ; \
do echo -n "  -drive file=/tmp/storage$i.qcow2,if=none,id=drive$i,format=qcow2 \
-device
scsi-hd,drive=drive$i,id=disk$i,channel=0,scsi-id=0,lun=$i,bootindex=$i"; \
done;` \
-drive file=guest.disk,if=none,id=drv1,format=qcow2,cache=none \
-device scsi-hd,drive=drv1,bootindex=1 -boot menu=on,splash-time=3000,strict=on
---
 slof/fs/base.fs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Segher Boessenkool Nov. 28, 2017, 8:25 a.m. | #1
On Tue, Nov 28, 2017 at 01:08:09PM +0530, Nikunj A Dadhania wrote:
> Current buffer of 1K is not sufficient, and causes exception if more than 20
> devices are used with bootindex. Increase the buffer size to 16K

Concatenating many strings this way is quadratic in the total length,
which is very painful with 1k already but ridiculously slow with 16k.
Use a better method?  $cat is a nice simple lazy utility word, it is
not good for constructing unbounded lists.


Segher


> diff --git a/slof/fs/base.fs b/slof/fs/base.fs
> index edd474e..b885d28 100644
> --- a/slof/fs/base.fs
> +++ b/slof/fs/base.fs
> @@ -53,7 +53,7 @@ VARIABLE mask -1 mask !
>  ;
>  
>  
> -CREATE $catpad 400 allot
> +CREATE $catpad 4000 allot
>  : $cat ( str1 len1 str2 len2 -- str3 len3 )
>     >r >r dup >r $catpad swap move
>     r> dup $catpad + r> swap r@ move
Nikunj A Dadhania Nov. 28, 2017, 10:01 a.m. | #2
Segher Boessenkool <segher@kernel.crashing.org> writes:

> On Tue, Nov 28, 2017 at 01:08:09PM +0530, Nikunj A Dadhania wrote:
>> Current buffer of 1K is not sufficient, and causes exception if more than 20
>> devices are used with bootindex. Increase the buffer size to 16K
>
> Concatenating many strings this way is quadratic in the total length,
> which is very painful with 1k already but ridiculously slow with 16k.
> Use a better method?  $cat is a nice simple lazy utility word, it is
> not good for constructing unbounded lists.

: load
[...]
   set-boot-args s" parse-load " $bootdev $cat strdup evaluate
;

Thats where we are hitting the limit. Maybe we can allocate and copy
both these strings without using the catpad?

Regards
Nikunj
Segher Boessenkool Nov. 28, 2017, 12:08 p.m. | #3
On Tue, Nov 28, 2017 at 03:31:43PM +0530, Nikunj A Dadhania wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Tue, Nov 28, 2017 at 01:08:09PM +0530, Nikunj A Dadhania wrote:
> >> Current buffer of 1K is not sufficient, and causes exception if more than 20
> >> devices are used with bootindex. Increase the buffer size to 16K
> >
> > Concatenating many strings this way is quadratic in the total length,
> > which is very painful with 1k already but ridiculously slow with 16k.
> > Use a better method?  $cat is a nice simple lazy utility word, it is
> > not good for constructing unbounded lists.
> 
> : load
> [...]
>    set-boot-args s" parse-load " $bootdev $cat strdup evaluate
> ;
> 
> Thats where we are hitting the limit. Maybe we can allocate and copy
> both these strings without using the catpad?

Do you need to at all?  parse-load wants to have the $bootdev string
as input buffer, so you can do

: load
  [...]
  set-boot-args
  save-source  -1 to source-id
  $boot-dev  dup #ib !  span !  to ib
  ['] parse-load catch  restore-source  throw  ;

This is just the core of EVALUATE, but calling PARSE-LOAD instead of
INTERPRET.

And yes, this can of course itself be factored better:

: $source ( str len -- )  -1 to source-id  dup #ib ! span ! to #ib  ;

: execute-with-source ( xt str len -- ??? )
  save-source  $source  catch  restore-source  throw  ;

: load
  [...]
  ['] parse-load $boot-dev execute-with-source  ;

\ and evaluate is just:
: evaluate ( str len -- ??? )  ['] interpret -rot execute-with-source  ;


Segher
Nikunj A Dadhania Nov. 29, 2017, 5:16 a.m. | #4
Segher Boessenkool <segher@kernel.crashing.org> writes:

> On Tue, Nov 28, 2017 at 03:31:43PM +0530, Nikunj A Dadhania wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> > On Tue, Nov 28, 2017 at 01:08:09PM +0530, Nikunj A Dadhania wrote:
>> >> Current buffer of 1K is not sufficient, and causes exception if more than 20
>> >> devices are used with bootindex. Increase the buffer size to 16K
>> >
>> > Concatenating many strings this way is quadratic in the total length,
>> > which is very painful with 1k already but ridiculously slow with 16k.
>> > Use a better method?  $cat is a nice simple lazy utility word, it is
>> > not good for constructing unbounded lists.
>> 
>> : load
>> [...]
>>    set-boot-args s" parse-load " $bootdev $cat strdup evaluate
>> ;
>> 
>> Thats where we are hitting the limit. Maybe we can allocate and copy
>> both these strings without using the catpad?
>
> Do you need to at all?  parse-load wants to have the $bootdev string
> as input buffer, so you can do
>
> : load
>   [...]
>   set-boot-args
>   save-source  -1 to source-id
>   $boot-dev  dup #ib !  span !  to ib
>   ['] parse-load catch  restore-source  throw  ;

parse-load eats away 4bytes of the first string. Trying to figure out
why ?

No NVRAM common partition, re-initializing...
Scanning USB 
Using default console: /vdevice/vty@71000000

List of bootdev: /pci@800000020000000/scsi@0/disk@100000000000000 HALT

Trying to load:  from: @800000020000000/scsi@0/disk@100000000000000 ... 
E3405: No such device

E3407: Load failed

Thanks,
Nikunj
Nikunj A Dadhania Nov. 30, 2017, 5:23 a.m. | #5
Hi Segher,

Segher Boessenkool <segher@kernel.crashing.org> writes:

> On Tue, Nov 28, 2017 at 03:31:43PM +0530, Nikunj A Dadhania wrote:
>> Segher Boessenkool <segher@kernel.crashing.org> writes:
>> > On Tue, Nov 28, 2017 at 01:08:09PM +0530, Nikunj A Dadhania wrote:
>> >> Current buffer of 1K is not sufficient, and causes exception if more than 20
>> >> devices are used with bootindex. Increase the buffer size to 16K
>> >
>> > Concatenating many strings this way is quadratic in the total length,
>> > which is very painful with 1k already but ridiculously slow with 16k.
>> > Use a better method?  $cat is a nice simple lazy utility word, it is
>> > not good for constructing unbounded lists.
>> 
>> : load
>> [...]
>>    set-boot-args s" parse-load " $bootdev $cat strdup evaluate
>> ;
>> 
>> Thats where we are hitting the limit. Maybe we can allocate and copy
>> both these strings without using the catpad?
>
> Do you need to at all?  parse-load wants to have the $bootdev string
> as input buffer, so you can do
>
> : load
>   [...]
>   set-boot-args
>   save-source  -1 to source-id
>   $boot-dev  dup #ib !  span !  to ib
>   ['] parse-load catch  restore-source  throw  ;

Had to add " 0 >in !" as used in interpret. Works after that, need your
input if the below is correct? 

diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs
index 1fd7439..c2b7551 100644
--- a/slof/fs/boot.fs
+++ b/slof/fs/boot.fs
@@ -221,7 +221,11 @@ defer go ( -- )
    ELSE
       drop
    THEN
-   set-boot-args s" parse-load " $bootdev $cat strdup evaluate
+   set-boot-args
+   save-source  -1 to source-id
+   $bootdev dup #ib ! span ! to ib
+   0 >in !
+   ['] parse-load catch restore-source throw
 ;
 
 : load-next ( -- success )     \ Continue after go failed


Regards
Nikunj
Segher Boessenkool Nov. 30, 2017, 4:12 p.m. | #6
Hi!

On Thu, Nov 30, 2017 at 10:53:57AM +0530, Nikunj A Dadhania wrote:
> >> Thats where we are hitting the limit. Maybe we can allocate and copy
> >> both these strings without using the catpad?
> >
> > Do you need to at all?  parse-load wants to have the $bootdev string
> > as input buffer, so you can do
> >
> > : load
> >   [...]
> >   set-boot-args
> >   save-source  -1 to source-id
> >   $boot-dev  dup #ib !  span !  to ib
> >   ['] parse-load catch  restore-source  throw  ;
> 
> Had to add " 0 >in !" as used in interpret. Works after that, need your

Ah, that makes sense yes.

> input if the below is correct? 
> 
> diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs
> index 1fd7439..c2b7551 100644
> --- a/slof/fs/boot.fs
> +++ b/slof/fs/boot.fs
> @@ -221,7 +221,11 @@ defer go ( -- )
>     ELSE
>        drop
>     THEN
> -   set-boot-args s" parse-load " $bootdev $cat strdup evaluate
> +   set-boot-args
> +   save-source  -1 to source-id
> +   $bootdev dup #ib ! span ! to ib
> +   0 >in !
> +   ['] parse-load catch restore-source throw
>  ;

Sure, looks fine...  The better factored thing would be nicer, have
all the things that dig deep in the interpreter's innards separate.
But your  0 >in !  thing is correct.


Segher
Nikunj A Dadhania Dec. 1, 2017, 5:18 a.m. | #7
Segher Boessenkool <segher@kernel.crashing.org> writes:

> Hi!
>
> On Thu, Nov 30, 2017 at 10:53:57AM +0530, Nikunj A Dadhania wrote:
>> >> Thats where we are hitting the limit. Maybe we can allocate and copy
>> >> both these strings without using the catpad?
>> >
>> > Do you need to at all?  parse-load wants to have the $bootdev string
>> > as input buffer, so you can do
>> >
>> > : load
>> >   [...]
>> >   set-boot-args
>> >   save-source  -1 to source-id
>> >   $boot-dev  dup #ib !  span !  to ib
>> >   ['] parse-load catch  restore-source  throw  ;
>> 
>> Had to add " 0 >in !" as used in interpret. Works after that, need your
>
> Ah, that makes sense yes.
>
>> input if the below is correct? 
>> 
>> diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs
>> index 1fd7439..c2b7551 100644
>> --- a/slof/fs/boot.fs
>> +++ b/slof/fs/boot.fs
>> @@ -221,7 +221,11 @@ defer go ( -- )
>>     ELSE
>>        drop
>>     THEN
>> -   set-boot-args s" parse-load " $bootdev $cat strdup evaluate
>> +   set-boot-args
>> +   save-source  -1 to source-id
>> +   $bootdev dup #ib ! span ! to ib
>> +   0 >in !
>> +   ['] parse-load catch restore-source throw
>>  ;
>
> Sure, looks fine...  The better factored thing would be nicer, have
> all the things that dig deep in the interpreter's innards separate.

I tried that but wasnt working and couldnt figure out why :(

> But your  0 >in !  thing is correct.

Thanks, will send updated patch.

Regards
Nikunj

Patch

diff --git a/slof/fs/base.fs b/slof/fs/base.fs
index edd474e..b885d28 100644
--- a/slof/fs/base.fs
+++ b/slof/fs/base.fs
@@ -53,7 +53,7 @@  VARIABLE mask -1 mask !
 ;
 
 
-CREATE $catpad 400 allot
+CREATE $catpad 4000 allot
 : $cat ( str1 len1 str2 len2 -- str3 len3 )
    >r >r dup >r $catpad swap move
    r> dup $catpad + r> swap r@ move