| Message ID | 20171128073809.15786-1-nikunj@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | base: increase catpad buffer | expand |
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
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
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
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
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
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
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
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
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(-)