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

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik at ozlabs.ru> writes:

> On 11/12/17 19:57, Nikunj A Dadhania wrote:
>> Alexey Kardashevskiy <aik at ozlabs.ru> writes:
>> 
>>> On 11/12/17 17:30, Nikunj A Dadhania wrote:
>>>> Hi Alexey,
>>>>
>>>> Alexey Kardashevskiy <aik at 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 at 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