[v2] boot: do not use catpad to concatenate strings

Message ID 20171201102946.15717-1-nikunj@linux.vnet.ibm.com
State Superseded
Headers show
Series
  • [v2] boot: do not use catpad to concatenate strings
Related show

Commit Message

Nikunj A Dadhania Dec. 1, 2017, 10:29 a.m.
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(-)

Comments

Nikunj A Dadhania Dec. 6, 2017, 4:45 a.m. | #1
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
Alexey Kardashevskiy Dec. 7, 2017, 12:23 a.m. | #2
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.
Thomas Huth Dec. 7, 2017, 6:51 a.m. | #3
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
Segher Boessenkool Dec. 7, 2017, 8:04 a.m. | #4
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
Nikunj A Dadhania Dec. 7, 2017, 10:24 a.m. | #5
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
Nikunj A Dadhania Dec. 7, 2017, 10:26 a.m. | #6
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
David Gibson Dec. 7, 2017, 10:36 a.m. | #7
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.
Segher Boessenkool Dec. 7, 2017, 11:44 a.m. | #8
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
Thomas Huth Dec. 7, 2017, 11:59 a.m. | #9
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

Patch

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
 ;