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

[Alexey Kardashevskiy]

On 12/12/17 14:47, Nikunj A Dadhania wrote:
> 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


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 at 800000020000000/ethernet at 1
/pci at 800000020000000/ethernet at 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
> 


-- 
Alexey