diff mbox series

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

Message ID 20171208053251.9958-1-nikunj@linux.vnet.ibm.com
State Superseded
Headers show
Series [v3] boot: do not use catpad to concatenate strings | expand

Commit Message

Nikunj A Dadhania Dec. 8, 2017, 5:32 a.m. UTC
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 dynamically allocated buffer(16K) 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 | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 5 deletions(-)

Comments

Alexey Kardashevskiy Dec. 11, 2017, 6:13 a.m. UTC | #1
On 08/12/17 16:32, 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 dynamically allocated buffer(16K) here.
> 
> Reported here: https://github.com/qemu/SLOF/issues/3
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

I've read the v1..v3 of this patch and (to my embarrassment) I do not
understand how this works at all :)

Above you said "concatenation is not required" but there is
bootdev-string-cat, how is that not concat? We did cat to bootdevice, now -
to bootdev-string-cat, but this is not just it?

For example:

0 > $bootdev type  /pci@800000020000000/ethernet@1
/pci@800000020000000/ethernet@2 disk cdrom net net1  ok


Thanks.


> 
> ---
> 
> 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 | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 48 insertions(+), 5 deletions(-)
> 
> diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs
> index 1fd7439..6d16c54 100644
> --- a/slof/fs/boot.fs
> +++ b/slof/fs/boot.fs
> @@ -15,8 +15,27 @@
>  VARIABLE state-valid false state-valid !
>  CREATE go-args 2 cells allot go-args 2 cells erase
>  
> +4000 CONSTANT bootdev-size
> +0 VALUE bootdev-buf
> +
>  \ \\\\\\\\\\\\\\ Structure/Implementation Dependent Methods
>  
> +: alloc-bootdev-buf ( -- )
> +   bootdev-size alloc-mem ?dup 0= ABORT" Unable to allocate bootdev buffer!"
> +   dup bootdev-size erase
> +   to bootdev-buf
> +;
> +
> +: free-bootdev-buf ( -- )
> +   bootdev-buf bootdev-size free-mem
> +   0 to bootdev-buf
> +;
> +
> +: bootdev-string-cat ( addr1 len1 addr2 len2 -- addr1 len1+len2 )
> +   dup 3 pick + bootdev-size > ABORT" bootdev size too big!"
> +   string-cat
> +;
> +
>  : $bootargs
>     bootargs 2@ ?dup IF
>     ELSE s" diagnostic-mode?" evaluate and IF s" diag-file" evaluate
> @@ -24,14 +43,23 @@ CREATE go-args 2 cells allot go-args 2 cells erase
>  ;
>  
>  : $bootdev ( -- device-name len )
> -   bootdevice 2@ dup IF s"  " $cat THEN
> +   alloc-bootdev-buf
> +   bootdevice 2@ ?dup IF
> +      swap bootdev-buf 2 pick move
> +      bootdev-buf swap s"  " bootdev-string-cat
> +   ELSE
> +      \ use bootdev-buf for concatenating diag mode/boot-device if any
> +      drop bootdev-buf 0
> +   THEN
>     s" diagnostic-mode?" evaluate IF
>        s" diag-device" evaluate
>     ELSE
>        s" boot-device" evaluate
>     THEN
> -   $cat \ prepend bootdevice setting from vpd-bootlist
> +   ( bootdev len str len1 )
> +   bootdev-string-cat \ concatenate both
>     strdup
> +   free-bootdev-buf
>     ?dup 0= IF
>        disable-watchdog
>        drop true ABORT" No boot device!"
> @@ -51,7 +79,14 @@ 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
> +      alloc-bootdev-buf
> +      swap bootdev-buf 2 pick move
> +      bootdev-buf swap s"  " bootdev-string-cat
> +      2swap bootdev-string-cat
> +   ELSE drop THEN
> +   set-boot-device
> +   bootdev-buf 0 <> IF free-bootdev-buf THEN
>  ;
>  
>  ' (add-boot-device) to add-boot-device
> @@ -221,11 +256,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
>  ;
>  
>
Nikunj A Dadhania Dec. 11, 2017, 6:30 a.m. UTC | #2
Hi Alexey,

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 08/12/17 16:32, 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 dynamically allocated buffer(16K) here.
>> 
>> Reported here: https://github.com/qemu/SLOF/issues/3
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>
> I've read the v1..v3 of this patch and (to my embarrassment) I do not
> understand how this works at all :)
>
> Above you said "concatenation is not required" but there is
> bootdev-string-cat, how is that not concat? We did cat to bootdevice, now -
> to bootdev-string-cat, but this is not just it?

Earlier the concatenation was done using a common buffer $catpad

slof/fs/base.fs:CREATE $catpad 400 allot

Which had a limitation of 1K, my first patch was to increase the size of
$catpad and Segher said that $catpad was kept small on purpose for
concatenating quickly without much overhead.

Version-2 patch did following things:

    1) introduced a similar static buffer for bootdev concatenation without
       using $cat.
    2) Replaced concatenation in LOAD and LOAD-NEXT by open coding EVALUATE word.

Version-3 made the static buffer allocate from heap instead of a static
buffer.

> For example:
>
> 0 > $bootdev type  /pci@800000020000000/ethernet@1
> /pci@800000020000000/ethernet@2 disk cdrom net net1  ok

HTH

Regards
Nikunj
Alexey Kardashevskiy Dec. 11, 2017, 8:02 a.m. UTC | #3
On 11/12/17 17:30, Nikunj A Dadhania wrote:
> Hi Alexey,
> 
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> On 08/12/17 16:32, 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 dynamically allocated buffer(16K) here.
>>>
>>> Reported here: https://github.com/qemu/SLOF/issues/3
>>>
>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>>
>> I've read the v1..v3 of this patch and (to my embarrassment) I do not
>> understand how this works at all :)
>>
>> Above you said "concatenation is not required" but there is
>> bootdev-string-cat, how is that not concat? We did cat to bootdevice, now -
>> to bootdev-string-cat, but this is not just it?
> 
> Earlier the concatenation was done using a common buffer $catpad
> 
> slof/fs/base.fs:CREATE $catpad 400 allot
> 
> Which had a limitation of 1K, my first patch was to increase the size of
> $catpad and Segher said that $catpad was kept small on purpose for
> concatenating quickly without much overhead.
> 
> Version-2 patch did following things:
> 
>     1) introduced a similar static buffer for bootdev concatenation without
>        using $cat.

This part I understood :)


>     2) Replaced concatenation in LOAD and LOAD-NEXT by open coding EVALUATE word.

This part I do not understand - why is this change needed or how does it
make it better?

There is a global list of boot devices - bootdevice; and for some reason
now there is another one - bootdev-buf, both are strings...

And there is also a load-list list which is what for? :)


> 
> Version-3 made the static buffer allocate from heap instead of a static
> buffer.
> 
>> For example:
>>
>> 0 > $bootdev type  /pci@800000020000000/ethernet@1
>> /pci@800000020000000/ethernet@2 disk cdrom net net1  ok
> 
> HTH
> 
> Regards
> Nikunj
>
Nikunj A Dadhania Dec. 11, 2017, 8:57 a.m. UTC | #4
Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 11/12/17 17:30, Nikunj A Dadhania wrote:
>> Hi Alexey,
>> 
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>> 
>>> On 08/12/17 16:32, 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 dynamically allocated buffer(16K) here.
>>>>
>>>> Reported here: https://github.com/qemu/SLOF/issues/3
>>>>
>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>>>
>>> I've read the v1..v3 of this patch and (to my embarrassment) I do not
>>> understand how this works at all :)
>>>
>>> Above you said "concatenation is not required" but there is
>>> bootdev-string-cat, how is that not concat? We did cat to bootdevice, now -
>>> to bootdev-string-cat, but this is not just it?
>> 
>> Earlier the concatenation was done using a common buffer $catpad
>> 
>> slof/fs/base.fs:CREATE $catpad 400 allot
>> 
>> Which had a limitation of 1K, my first patch was to increase the size of
>> $catpad and Segher said that $catpad was kept small on purpose for
>> concatenating quickly without much overhead.
>> 
>> Version-2 patch did following things:
>> 
>>     1) introduced a similar static buffer for bootdev concatenation without
>>        using $cat.
>
> This part I understood :)
>
>
>>     2) Replaced concatenation in LOAD and LOAD-NEXT by open coding EVALUATE word.
>
> This part I do not understand - why is this change needed or how does it
> make it better?

Because we were  concatenating and then evaluating during runtime, newer
code plays directly with args and populates the stack and calls
parse-load, so concatenation is not required anymore.

> There is a global list of boot devices - bootdevice; and for some reason
> now there is another one - bootdev-buf, both are strings...

No, bootdev-buf is a temporary buffer, its not replacing bootdevice.


> And there is also a load-list list which is what for? :)

Not sure what you are referring to.

Regards
Nikunj
Segher Boessenkool Dec. 11, 2017, 12:26 p.m. UTC | #5
Hi!

On Mon, Dec 11, 2017 at 12:00:25PM +0530, Nikunj A Dadhania wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> > I've read the v1..v3 of this patch and (to my embarrassment) I do not
> > understand how this works at all :)
> >
> > Above you said "concatenation is not required" but there is
> > bootdev-string-cat, how is that not concat? We did cat to bootdevice, now -
> > to bootdev-string-cat, but this is not just it?
> 
> Earlier the concatenation was done using a common buffer $catpad
> 
> slof/fs/base.fs:CREATE $catpad 400 allot
> 
> Which had a limitation of 1K, my first patch was to increase the size of
> $catpad and Segher said that $catpad was kept small on purpose for
> concatenating quickly without much overhead.

No, I said you do not concatenate much, on purpose.  This is to avoid
quadratic time.  Since the way the code used $cat is bad, it doesn't
matter if the buffer was too small for it; it is big enough for all
legitimate users.

I don't know if your patch achieves proper scaling.


Segher
Alexey Kardashevskiy Dec. 12, 2017, 2:48 a.m. UTC | #6
On 11/12/17 19:57, Nikunj A Dadhania wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> On 11/12/17 17:30, Nikunj A Dadhania wrote:
>>> Hi Alexey,
>>>
>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>
>>>> On 08/12/17 16:32, 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 dynamically allocated buffer(16K) here.
>>>>>
>>>>> Reported here: https://github.com/qemu/SLOF/issues/3
>>>>>
>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>>>>
>>>> I've read the v1..v3 of this patch and (to my embarrassment) I do not
>>>> understand how this works at all :)
>>>>
>>>> Above you said "concatenation is not required" but there is
>>>> bootdev-string-cat, how is that not concat? We did cat to bootdevice, now -
>>>> to bootdev-string-cat, but this is not just it?
>>>
>>> Earlier the concatenation was done using a common buffer $catpad
>>>
>>> slof/fs/base.fs:CREATE $catpad 400 allot
>>>
>>> Which had a limitation of 1K, my first patch was to increase the size of
>>> $catpad and Segher said that $catpad was kept small on purpose for
>>> concatenating quickly without much overhead.
>>>
>>> Version-2 patch did following things:
>>>
>>>     1) introduced a similar static buffer for bootdev concatenation without
>>>        using $cat.
>>
>> This part I understood :)
>>
>>
>>>     2) Replaced concatenation in LOAD and LOAD-NEXT by open coding EVALUATE word.
>>
>> This part I do not understand - why is this change needed or how does it
>> make it better?
> 
> Because we were  concatenating and then evaluating during runtime, newer
> code plays directly with args and populates the stack and calls
> parse-load, so concatenation is not required anymore.


This must be some different concatenation then as the patch uses new
bootdev-buf for concatenation.


>> There is a global list of boot devices - bootdevice; and for some reason
>> now there is another one - bootdev-buf, both are strings...
> 
> No, bootdev-buf is a temporary buffer, its not replacing bootdevice.

Ufff. I am lost now. $bootdev adds something to the dictionary (via
strdup), and so does (set-boot-device), and "bootdevice" points to what?


Since you understand this stuff, can you please outline what is called what
and where it is all stored?

(add-boot-device) is called when devices are discovered, the result is in
the dictionary, "bootdevice" points to a concatenated string. What happens
then?


> 
> 
>> And there is also a load-list list which is what for? :)
> 
> Not sure what you are referring to.


slof/fs/boot.fs  :

: load-next ( -- success ). \ Continue after go failed
   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
;


This uses "load-list" - what is that for?


I would have pushed this out already if they were 2 patches - one replacing
$cat with bootdev-string-cat and the other doing this open coded thing but
I do not know if these parts are independent and where is the line :)
Nikunj A Dadhania Dec. 12, 2017, 3:47 a.m. UTC | #7
Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 11/12/17 19:57, Nikunj A Dadhania wrote:
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>> 
>>> On 11/12/17 17:30, Nikunj A Dadhania wrote:
>>>> Hi Alexey,
>>>>
>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>>
>>>>> On 08/12/17 16:32, 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 dynamically allocated buffer(16K) here.
>>>>>>
>>>>>> Reported here: https://github.com/qemu/SLOF/issues/3
>>>>>>
>>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>>>>>
>>>>> I've read the v1..v3 of this patch and (to my embarrassment) I do not
>>>>> understand how this works at all :)
>>>>>
>>>>> Above you said "concatenation is not required" but there is
>>>>> bootdev-string-cat, how is that not concat? We did cat to bootdevice, now -
>>>>> to bootdev-string-cat, but this is not just it?
>>>>
>>>> Earlier the concatenation was done using a common buffer $catpad
>>>>
>>>> slof/fs/base.fs:CREATE $catpad 400 allot
>>>>
>>>> Which had a limitation of 1K, my first patch was to increase the size of
>>>> $catpad and Segher said that $catpad was kept small on purpose for
>>>> concatenating quickly without much overhead.
>>>>
>>>> Version-2 patch did following things:
>>>>
>>>>     1) introduced a similar static buffer for bootdev concatenation without
>>>>        using $cat.
>>>
>>> This part I understood :)
>>>
>>>
>>>>     2) Replaced concatenation in LOAD and LOAD-NEXT by open coding EVALUATE word.
>>>
>>> This part I do not understand - why is this change needed or how does it
>>> make it better?
>> 
>> Because we were  concatenating and then evaluating during runtime, newer
>> code plays directly with args and populates the stack and calls
>> parse-load, so concatenation is not required anymore.
>
>
> This must be some different concatenation then as the patch uses new
> bootdev-buf for concatenation.

Let us take the example:

   set-boot-args s" parse-load " $bootdev $cat strdup evaluate
                                          ^^^^^^^^^^^

We were concatenating the word " parse-load" and our bootdevice list
that was input to evaluate. In the discussion with Segher, he suggested
to open code evaluate, that eliminates the requirement for
concatenation. Newer "load" does not use $cat anymore.

   set-boot-args
   save-source  -1 to source-id
   $bootdev dup #ib ! span ! to ib
   0 >in !
   ['] parse-load catch restore-source throw


>
>>> There is a global list of boot devices - bootdevice; and for some reason
>>> now there is another one - bootdev-buf, both are strings...
>> 
>> No, bootdev-buf is a temporary buffer, its not replacing bootdevice.
>
> Ufff. I am lost now. $bootdev adds something to the dictionary (via
> strdup), and so does (set-boot-device), and "bootdevice" points to
> what?

slof/fs/loaders.fs:CREATE bootdevice 2 cells allot bootdevice 2 cells erase

Its where you store the boot device string and its length. Whenever you
read/update the list of boot devices, this is where finally the list is
updated. Something sort of a pointer + length


> Since you understand this stuff, can you please outline what is called what
> and where it is all stored?
>
> (add-boot-device) is called when devices are discovered, the result is in
> the dictionary, "bootdevice" points to a concatenated string. What happens
> then?

At every discovery of boot device, old list of boot device and new one
is concatenated and stored back in bootdevice.

During the loading stage, this list is passed as argument to parse-load.
It goes one by one through this list trying to boot:

There are three conditions:
1) Fails loading (not bootable device)
2) Execption during loading (bootable device but something went wrong)
3) Successfully loaded (boots to the guest)


For first case we just go to next device. Second case is caught in
"boot" word and we need to resume back from the next eligible boot
device. That is where load-next comes into picture, but till what point
did we complete in the list of boot devices? That info is there in
load-list.

And finally if nothing works we drop to slof prompt. 

Thomas/Segher, please feel free to add/comment

>>> And there is also a load-list list which is what for? :)
>> 
>> Not sure what you are referring to.
>
> slof/fs/boot.fs  :
>
> : load-next ( -- success ). \ Continue after go failed
>    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
> ;


> This uses "load-list" - what is that for?

Explained above.

> I would have pushed this out already if they were 2 patches - one replacing
> $cat with bootdev-string-cat and the other doing this open coded thing but
> I do not know if these parts are independent and where is the line :)

Yes, i was thinking on that line as well. Maybe easier to read them. The
will be solved with both the patches though.

Regards
Nikunj
Alexey Kardashevskiy Dec. 12, 2017, 6:44 a.m. UTC | #8
On 12/12/17 14:47, Nikunj A Dadhania wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> On 11/12/17 19:57, Nikunj A Dadhania wrote:
>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>
>>>> On 11/12/17 17:30, Nikunj A Dadhania wrote:
>>>>> Hi Alexey,
>>>>>
>>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>>>
>>>>>> On 08/12/17 16:32, 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 dynamically allocated buffer(16K) here.
>>>>>>>
>>>>>>> Reported here: https://github.com/qemu/SLOF/issues/3
>>>>>>>
>>>>>>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>>>>>>
>>>>>> I've read the v1..v3 of this patch and (to my embarrassment) I do not
>>>>>> understand how this works at all :)
>>>>>>
>>>>>> Above you said "concatenation is not required" but there is
>>>>>> bootdev-string-cat, how is that not concat? We did cat to bootdevice, now -
>>>>>> to bootdev-string-cat, but this is not just it?
>>>>>
>>>>> Earlier the concatenation was done using a common buffer $catpad
>>>>>
>>>>> slof/fs/base.fs:CREATE $catpad 400 allot
>>>>>
>>>>> Which had a limitation of 1K, my first patch was to increase the size of
>>>>> $catpad and Segher said that $catpad was kept small on purpose for
>>>>> concatenating quickly without much overhead.
>>>>>
>>>>> Version-2 patch did following things:
>>>>>
>>>>>     1) introduced a similar static buffer for bootdev concatenation without
>>>>>        using $cat.
>>>>
>>>> This part I understood :)
>>>>
>>>>
>>>>>     2) Replaced concatenation in LOAD and LOAD-NEXT by open coding EVALUATE word.
>>>>
>>>> This part I do not understand - why is this change needed or how does it
>>>> make it better?
>>>
>>> Because we were  concatenating and then evaluating during runtime, newer
>>> code plays directly with args and populates the stack and calls
>>> parse-load, so concatenation is not required anymore.
>>
>>
>> This must be some different concatenation then as the patch uses new
>> bootdev-buf for concatenation.
> 
> Let us take the example:
> 
>    set-boot-args s" parse-load " $bootdev $cat strdup evaluate
>                                           ^^^^^^^^^^^
> 
> We were concatenating the word " parse-load" and our bootdevice list
> that was input to evaluate. In the discussion with Segher, he suggested
> to open code evaluate, that eliminates the requirement for
> concatenation. Newer "load" does not use $cat anymore.
> 
>    set-boot-args
>    save-source  -1 to source-id
>    $bootdev dup #ib ! span ! to ib
>    0 >in !
>    ['] parse-load catch restore-source throw


Ah, I see... Hm. But why do not we just call parse-load directly, without
evaluate or this really not obvious open coded version of evaluate? :)

It all looks unnecessary complicated :(



> 
> 
>>
>>>> There is a global list of boot devices - bootdevice; and for some reason
>>>> now there is another one - bootdev-buf, both are strings...
>>>
>>> No, bootdev-buf is a temporary buffer, its not replacing bootdevice.
>>
>> Ufff. I am lost now. $bootdev adds something to the dictionary (via
>> strdup), and so does (set-boot-device), and "bootdevice" points to
>> what?
> 
> slof/fs/loaders.fs:CREATE bootdevice 2 cells allot bootdevice 2 cells erase
> 
> Its where you store the boot device string and its length. Whenever you
> read/update the list of boot devices, this is where finally the list is
> updated. Something sort of a pointer + length
> 
> 
>> Since you understand this stuff, can you please outline what is called what
>> and where it is all stored?
>>
>> (add-boot-device) is called when devices are discovered, the result is in
>> the dictionary, "bootdevice" points to a concatenated string. What happens
>> then?
> 
> At every discovery of boot device, old list of boot device and new one
> is concatenated and stored back in bootdevice.
> 
> During the loading stage, this list is passed as argument to parse-load.
> It goes one by one through this list trying to boot:
> 
> There are three conditions:
> 1) Fails loading (not bootable device)
> 2) Execption during loading (bootable device but something went wrong)
> 3) Successfully loaded (boots to the guest)
> 
> 
> For first case we just go to next device. Second case is caught in
> "boot" word and we need to resume back from the next eligible boot
> device. That is where load-next comes into picture, but till what point
> did we complete in the list of boot devices? That info is there in
> load-list.

I have $bootdev = "/pci@800000020000000/ethernet@1
/pci@800000020000000/ethernet@2 disk cdrom net net1" and I see slof trying
them all but I do not see load-next called at all, hence my question...




> 
> And finally if nothing works we drop to slof prompt. 
> 
> Thomas/Segher, please feel free to add/comment
> 
>>>> And there is also a load-list list which is what for? :)
>>>
>>> Not sure what you are referring to.
>>
>> slof/fs/boot.fs  :
>>
>> : load-next ( -- success ). \ Continue after go failed
>>    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
>> ;
> 
> 
>> This uses "load-list" - what is that for?
> 
> Explained above.
> 
>> I would have pushed this out already if they were 2 patches - one replacing
>> $cat with bootdev-string-cat and the other doing this open coded thing but
>> I do not know if these parts are independent and where is the line :)
> 
> Yes, i was thinking on that line as well. Maybe easier to read them. The
> will be solved with both the patches though.
> 
> Regards
> Nikunj
>
Segher Boessenkool Dec. 12, 2017, 5:19 p.m. UTC | #9
On Tue, Dec 12, 2017 at 09:17:25AM +0530, Nikunj A Dadhania wrote:
> slof/fs/loaders.fs:CREATE bootdevice 2 cells allot bootdevice 2 cells erase

That is just

CREATE bootdevice 0 , 0 ,

which is a bit easier to read ;-)


Segher
Segher Boessenkool Dec. 12, 2017, 5:29 p.m. UTC | #10
On Tue, Dec 12, 2017 at 05:44:04PM +1100, Alexey Kardashevskiy wrote:
> > We were concatenating the word " parse-load" and our bootdevice list
> > that was input to evaluate. In the discussion with Segher, he suggested
> > to open code evaluate, that eliminates the requirement for
> > concatenation. Newer "load" does not use $cat anymore.
> > 
> >    set-boot-args
> >    save-source  -1 to source-id
> >    $bootdev dup #ib ! span ! to ib
> >    0 >in !
> >    ['] parse-load catch restore-source throw
> 
> 
> Ah, I see... Hm. But why do not we just call parse-load directly, without
> evaluate or this really not obvious open coded version of evaluate? :)
> 
> It all looks unnecessary complicated :(

Since parse-load reads from the input device, via the parse area, you
need to set up your own, and save / restore it around it.

The CATCH is to ensure the restore happens even if something down in
parse-load calls THROW (or ABORT etc.).

I did suggest to have this factored out so you could do something like

  $bootdev ['] parse-load execute-with-input


Segher
Alexey Kardashevskiy Dec. 13, 2017, 1:51 a.m. UTC | #11
On 13/12/17 04:29, Segher Boessenkool wrote:
> On Tue, Dec 12, 2017 at 05:44:04PM +1100, Alexey Kardashevskiy wrote:
>>> We were concatenating the word " parse-load" and our bootdevice list
>>> that was input to evaluate. In the discussion with Segher, he suggested
>>> to open code evaluate, that eliminates the requirement for
>>> concatenation. Newer "load" does not use $cat anymore.
>>>
>>>    set-boot-args
>>>    save-source  -1 to source-id
>>>    $bootdev dup #ib ! span ! to ib
>>>    0 >in !
>>>    ['] parse-load catch restore-source throw
>>
>>
>> Ah, I see... Hm. But why do not we just call parse-load directly, without
>> evaluate or this really not obvious open coded version of evaluate? :)
>>
>> It all looks unnecessary complicated :(
> 
> Since parse-load reads from the input device, via the parse area, you
> need to set up your own, and save / restore it around it.
> 
> The CATCH is to ensure the restore happens even if something down in
> parse-load calls THROW (or ABORT etc.).
> 
> I did suggest to have this factored out so you could do something like
> 
>   $bootdev ['] parse-load execute-with-input


Factor out "de-alias do-load" bits from parse-load and call it directly,
may be?
Segher Boessenkool Dec. 13, 2017, 8:23 p.m. UTC | #12
On Wed, Dec 13, 2017 at 12:51:24PM +1100, Alexey Kardashevskiy wrote:
> >> Ah, I see... Hm. But why do not we just call parse-load directly, without
> >> evaluate or this really not obvious open coded version of evaluate? :)
> >>
> >> It all looks unnecessary complicated :(
> > 
> > Since parse-load reads from the input device, via the parse area, you
> > need to set up your own, and save / restore it around it.
> > 
> > The CATCH is to ensure the restore happens even if something down in
> > parse-load calls THROW (or ABORT etc.).
> > 
> > I did suggest to have this factored out so you could do something like
> > 
> >   $bootdev ['] parse-load execute-with-input
> 
> 
> Factor out "de-alias do-load" bits from parse-load and call it directly,
> may be?

I think that's a lot of refactoring...  But if you feel up to it, sure,
that should improve the code :-)


Segher
diff mbox series

Patch

diff --git a/slof/fs/boot.fs b/slof/fs/boot.fs
index 1fd7439..6d16c54 100644
--- a/slof/fs/boot.fs
+++ b/slof/fs/boot.fs
@@ -15,8 +15,27 @@ 
 VARIABLE state-valid false state-valid !
 CREATE go-args 2 cells allot go-args 2 cells erase
 
+4000 CONSTANT bootdev-size
+0 VALUE bootdev-buf
+
 \ \\\\\\\\\\\\\\ Structure/Implementation Dependent Methods
 
+: alloc-bootdev-buf ( -- )
+   bootdev-size alloc-mem ?dup 0= ABORT" Unable to allocate bootdev buffer!"
+   dup bootdev-size erase
+   to bootdev-buf
+;
+
+: free-bootdev-buf ( -- )
+   bootdev-buf bootdev-size free-mem
+   0 to bootdev-buf
+;
+
+: bootdev-string-cat ( addr1 len1 addr2 len2 -- addr1 len1+len2 )
+   dup 3 pick + bootdev-size > ABORT" bootdev size too big!"
+   string-cat
+;
+
 : $bootargs
    bootargs 2@ ?dup IF
    ELSE s" diagnostic-mode?" evaluate and IF s" diag-file" evaluate
@@ -24,14 +43,23 @@  CREATE go-args 2 cells allot go-args 2 cells erase
 ;
 
 : $bootdev ( -- device-name len )
-   bootdevice 2@ dup IF s"  " $cat THEN
+   alloc-bootdev-buf
+   bootdevice 2@ ?dup IF
+      swap bootdev-buf 2 pick move
+      bootdev-buf swap s"  " bootdev-string-cat
+   ELSE
+      \ use bootdev-buf for concatenating diag mode/boot-device if any
+      drop bootdev-buf 0
+   THEN
    s" diagnostic-mode?" evaluate IF
       s" diag-device" evaluate
    ELSE
       s" boot-device" evaluate
    THEN
-   $cat \ prepend bootdevice setting from vpd-bootlist
+   ( bootdev len str len1 )
+   bootdev-string-cat \ concatenate both
    strdup
+   free-bootdev-buf
    ?dup 0= IF
       disable-watchdog
       drop true ABORT" No boot device!"
@@ -51,7 +79,14 @@  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
+      alloc-bootdev-buf
+      swap bootdev-buf 2 pick move
+      bootdev-buf swap s"  " bootdev-string-cat
+      2swap bootdev-string-cat
+   ELSE drop THEN
+   set-boot-device
+   bootdev-buf 0 <> IF free-bootdev-buf THEN
 ;
 
 ' (add-boot-device) to add-boot-device
@@ -221,11 +256,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
 ;