| Message ID | 20171201102946.15717-1-nikunj@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] boot: do not use catpad to concatenate strings | expand |
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: > The catpad size is 1K size, which can be hit easily hit with around 20 devices > with bootindex. > > Open code EVALUATE such that concatenation is not required. Replace usage of > $cat with a 16K buffer allocated here. > > Reported here: https://github.com/qemu/SLOF/issues/3 > > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> Ping? Regards Nikunj
On 06/12/17 15:45, Nikunj A Dadhania wrote: > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: > >> The catpad size is 1K size, which can be hit easily hit with around 20 devices >> with bootindex. >> >> Open code EVALUATE such that concatenation is not required. Replace usage of >> $cat with a 16K buffer allocated here. >> >> Reported here: https://github.com/qemu/SLOF/issues/3 >> >> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> > > Ping? I'll wait till tomorrow evening and push it out if there is no objection.
On 01.12.2017 11:29, Nikunj A Dadhania wrote: > The catpad size is 1K size, which can be hit easily hit with around 20 devices > with bootindex. > > Open code EVALUATE such that concatenation is not required. Replace usage of > $cat with a 16K buffer allocated here. > > 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/boot.fs | 30 +++++++++++++++++++++++++----- > 1 file changed, 25 insertions(+), 5 deletions(-) > > diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs > index 1fd7439..8a30195 100644 > --- a/slof/fs/boot.fs > +++ b/slof/fs/boot.fs > @@ -15,6 +15,9 @@ > VARIABLE state-valid false state-valid ! > CREATE go-args 2 cells allot go-args 2 cells erase > > +4000 CONSTANT BOOT_DEV_SIZE > +CREATE bootdev-buf BOOT_DEV_SIZE allot I somehow dislike the idea that we statically reserve such big arrays ... would it be feasible to do this with alloc-mem on the fly instead? Thomas
On Thu, Dec 07, 2017 at 07:51:31AM +0100, Thomas Huth wrote: > > diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs > > index 1fd7439..8a30195 100644 > > --- a/slof/fs/boot.fs > > +++ b/slof/fs/boot.fs > > @@ -15,6 +15,9 @@ > > VARIABLE state-valid false state-valid ! > > CREATE go-args 2 cells allot go-args 2 cells erase > > > > +4000 CONSTANT BOOT_DEV_SIZE > > +CREATE bootdev-buf BOOT_DEV_SIZE allot > > I somehow dislike the idea that we statically reserve such big arrays > ... would it be feasible to do this with alloc-mem on the fly instead? And don't use underscores in names please; even if you like underscores better than dashes, surely you like consistency more. There's no need for caps either. Etc. I couldn't follow the code, fwiw, so it may be that it is not factored well (or it does totally strange things). Segher
Thomas Huth <thuth@redhat.com> writes: > On 01.12.2017 11:29, Nikunj A Dadhania wrote: >> The catpad size is 1K size, which can be hit easily hit with around 20 devices >> with bootindex. >> >> Open code EVALUATE such that concatenation is not required. Replace usage of >> $cat with a 16K buffer allocated here. >> >> 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/boot.fs | 30 +++++++++++++++++++++++++----- >> 1 file changed, 25 insertions(+), 5 deletions(-) >> >> diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs >> index 1fd7439..8a30195 100644 >> --- a/slof/fs/boot.fs >> +++ b/slof/fs/boot.fs >> @@ -15,6 +15,9 @@ >> VARIABLE state-valid false state-valid ! >> CREATE go-args 2 cells allot go-args 2 cells erase >> >> +4000 CONSTANT BOOT_DEV_SIZE >> +CREATE bootdev-buf BOOT_DEV_SIZE allot > > I somehow dislike the idea that we statically reserve such big arrays > ... would it be feasible to do this with alloc-mem on the fly instead? Let me try that out Regards Nikunj
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Thu, Dec 07, 2017 at 07:51:31AM +0100, Thomas Huth wrote: >> > diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs >> > index 1fd7439..8a30195 100644 >> > --- a/slof/fs/boot.fs >> > +++ b/slof/fs/boot.fs >> > @@ -15,6 +15,9 @@ >> > VARIABLE state-valid false state-valid ! >> > CREATE go-args 2 cells allot go-args 2 cells erase >> > >> > +4000 CONSTANT BOOT_DEV_SIZE >> > +CREATE bootdev-buf BOOT_DEV_SIZE allot >> >> I somehow dislike the idea that we statically reserve such big arrays >> ... would it be feasible to do this with alloc-mem on the fly instead? > > And don't use underscores in names please; even if you like underscores > better than dashes, surely you like consistency more. There's no need > for caps either. Etc. Sure. > I couldn't follow the code, fwiw, so it may be that it is not factored > well (or it does totally strange things). There were multiple locations that needed removal of $cat, while getting rid of static buffer will try re-factoring. Regards Nikunj
On Thu, Dec 07, 2017 at 03:54:45PM +0530, Nikunj A Dadhania wrote: > Thomas Huth <thuth@redhat.com> writes: > > > On 01.12.2017 11:29, Nikunj A Dadhania wrote: > >> The catpad size is 1K size, which can be hit easily hit with around 20 devices > >> with bootindex. > >> > >> Open code EVALUATE such that concatenation is not required. Replace usage of > >> $cat with a 16K buffer allocated here. > >> > >> 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/boot.fs | 30 +++++++++++++++++++++++++----- > >> 1 file changed, 25 insertions(+), 5 deletions(-) > >> > >> diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs > >> index 1fd7439..8a30195 100644 > >> --- a/slof/fs/boot.fs > >> +++ b/slof/fs/boot.fs > >> @@ -15,6 +15,9 @@ > >> VARIABLE state-valid false state-valid ! > >> CREATE go-args 2 cells allot go-args 2 cells erase > >> > >> +4000 CONSTANT BOOT_DEV_SIZE > >> +CREATE bootdev-buf BOOT_DEV_SIZE allot > > > > I somehow dislike the idea that we statically reserve such big arrays > > ... would it be feasible to do this with alloc-mem on the fly instead? > > Let me try that out Don't try too hard. I mean, sure a large static array is kinda ugly, but when it comes down it the whole guest's memory is there with nothing else to use it for until SLOF's done.
On Thu, Dec 07, 2017 at 09:36:44PM +1100, David Gibson wrote: > > >> VARIABLE state-valid false state-valid ! > > >> CREATE go-args 2 cells allot go-args 2 cells erase > > >> > > >> +4000 CONSTANT BOOT_DEV_SIZE > > >> +CREATE bootdev-buf BOOT_DEV_SIZE allot > > > > > > I somehow dislike the idea that we statically reserve such big arrays > > > ... would it be feasible to do this with alloc-mem on the fly instead? > > > > Let me try that out > > Don't try too hard. I mean, sure a large static array is kinda ugly, but > when it comes down it the whole guest's memory is there with nothing > else to use it for until SLOF's done. Sure, 16kB is nothing. But 16kB is not enough in general, or are there any guarantees? Arbitrary (and undocumented, and not checked for!) limits tend to be broken, and such things are hard to debug. (Just adding a check would make things much nicer already: ( size) dup bootdev-size > ABORT" bootdev size too big!" or similar). Segher
On 07.12.2017 11:36, David Gibson wrote: > On Thu, Dec 07, 2017 at 03:54:45PM +0530, Nikunj A Dadhania wrote: >> Thomas Huth <thuth@redhat.com> writes: >> >>> On 01.12.2017 11:29, Nikunj A Dadhania wrote: >>>> The catpad size is 1K size, which can be hit easily hit with around 20 devices >>>> with bootindex. >>>> >>>> Open code EVALUATE such that concatenation is not required. Replace usage of >>>> $cat with a 16K buffer allocated here. >>>> >>>> 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/boot.fs | 30 +++++++++++++++++++++++++----- >>>> 1 file changed, 25 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs >>>> index 1fd7439..8a30195 100644 >>>> --- a/slof/fs/boot.fs >>>> +++ b/slof/fs/boot.fs >>>> @@ -15,6 +15,9 @@ >>>> VARIABLE state-valid false state-valid ! >>>> CREATE go-args 2 cells allot go-args 2 cells erase >>>> >>>> +4000 CONSTANT BOOT_DEV_SIZE >>>> +CREATE bootdev-buf BOOT_DEV_SIZE allot >>> >>> I somehow dislike the idea that we statically reserve such big arrays >>> ... would it be feasible to do this with alloc-mem on the fly instead? >> >> Let me try that out > > Don't try too hard. I mean, sure a large static array is kinda ugly, but > when it comes down it the whole guest's memory is there with nothing > else to use it for until SLOF's done. SLOF is located at the end of the memory, so the available amount of its usable memory with "allot" is also limited. Of course we could reserve more memory for SLOF again one day, but discovering that we run into that situation again will also be a pain. So let's try at least to not waste to much memory right now, ok? Thomas
diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs index 1fd7439..8a30195 100644 --- a/slof/fs/boot.fs +++ b/slof/fs/boot.fs @@ -15,6 +15,9 @@ VARIABLE state-valid false state-valid ! CREATE go-args 2 cells allot go-args 2 cells erase +4000 CONSTANT BOOT_DEV_SIZE +CREATE bootdev-buf BOOT_DEV_SIZE allot + \ \\\\\\\\\\\\\\ Structure/Implementation Dependent Methods : $bootargs @@ -24,13 +27,17 @@ CREATE go-args 2 cells allot go-args 2 cells erase ; : $bootdev ( -- device-name len ) - bootdevice 2@ dup IF s" " $cat THEN + bootdev-buf BOOT_DEV_SIZE erase + bootdevice 2@ dup IF + swap bootdev-buf 2 pick move + bootdev-buf swap s" " string-cat + THEN s" diagnostic-mode?" evaluate IF s" diag-device" evaluate ELSE s" boot-device" evaluate THEN - $cat \ prepend bootdevice setting from vpd-bootlist + string-cat \ concatenate both strdup ?dup 0= IF disable-watchdog @@ -51,7 +58,12 @@ CREATE go-args 2 cells allot go-args 2 cells erase ' (set-boot-device) to set-boot-device : (add-boot-device) ( str len -- ) \ Concatenate " str" to "bootdevice" - bootdevice 2@ ?dup IF $cat-space ELSE drop THEN set-boot-device + bootdevice 2@ ?dup IF + swap bootdev-buf 2 pick move + bootdev-buf swap s" " string-cat + 2swap string-cat + ELSE drop THEN + set-boot-device ; ' (add-boot-device) to add-boot-device @@ -221,11 +233,19 @@ 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 - load-list 2@ ?dup IF s" parse-load " 2swap $cat strdup evaluate + load-list 2@ ?dup IF + save-source -1 to source-id + dup #ib ! span ! to ib + 0 >in ! + ['] parse-load catch restore-source throw ELSE drop false THEN ;
The catpad size is 1K size, which can be hit easily hit with around 20 devices with bootindex. Open code EVALUATE such that concatenation is not required. Replace usage of $cat with a 16K buffer allocated here. 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/boot.fs | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)