[Skiboot] [PATCH V4 1/3] opal-msg: Add support to increase async completions and msg_free_list

[Stewart Smith]

Cyril Bur <cyrilbur at gmail.com> writes:
> On Thu, 2017-06-29 at 23:50 +0530, Shilpasri G Bhat wrote:
>> Add support to increase the maximum async completions passed to kernel
>> and also add helpers to increase the pre-allocation of free buffers in
>> msg_free_list.
>> 
>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat at linux.vnet.ibm.com>
>> ---
>> - No changes from V3
>>  core/init.c        |  2 ++
>>  core/opal-msg.c    | 22 ++++++++++++++++++++--
>>  core/opal.c        |  1 -
>>  include/opal-msg.h |  3 +++
>>  4 files changed, 25 insertions(+), 3 deletions(-)
>> 
>> diff --git a/core/init.c b/core/init.c
>> index 02bd30c..c58ef3a 100644
>> --- a/core/init.c
>> +++ b/core/init.c
>> @@ -1055,6 +1055,8 @@ void __noreturn __nomcount main_cpu_entry(const void *fdt)
>>  	/* On P9, switch to radix mode by default */
>>  	cpu_set_radix_mode();
>>  
>> +	add_dt_max_async_completions();
>> +
>>  	load_and_boot_kernel(false);
>>  }
>>  
>> diff --git a/core/opal-msg.c b/core/opal-msg.c
>> index 1971467..0ac891d 100644
>> --- a/core/opal-msg.c
>> +++ b/core/opal-msg.c
>> @@ -19,9 +19,12 @@
>>  #include <opal-msg.h>
>>  #include <opal-api.h>
>>  #include <lock.h>
>> +#include <device.h>
>>  
>>  #define OPAL_MAX_MSGS		(OPAL_MSG_TYPE_MAX + OPAL_MAX_ASYNC_COMP - 1)
>>  
>
> So this leaves this #define using OPAL_MAX_ASYNC_COMP, but that number
> isn't really relevant anymore is it? Since the next patch will change
> the max_async_comp variable, which is what you put the in device
> tree...
>
>> +static int max_async_comp;
>> +
>>  struct opal_msg_entry {
>>  	struct list_node link;
>>  	void (*consumed)(void *data);
>> @@ -149,12 +152,12 @@ static int64_t opal_check_completion(uint64_t *buffer, uint64_t size,
>>  }
>>  opal_call(OPAL_CHECK_ASYNC_COMPLETION, opal_check_completion, 3);
>>  
>> -void opal_init_msg(void)
>> +void alloc_opal_msg_list(int num)
>>  {
>>  	struct opal_msg_entry *entry;
>>  	int i;
>>  
>> -	for (i = 0; i < OPAL_MAX_MSGS; i++, entry++) {
>> +	for (i = 0; i < num; i++, entry++) {
>>                  entry = zalloc(sizeof(*entry));
>>                  if (!entry)
>>                          goto err;
>> @@ -170,3 +173,18 @@ err:
>>          }
>>  }
>>  
>> +void add_dt_max_async_completions(void)
>> +{
>> +	dt_add_property_cells(opal_node, "opal-msg-async-num", max_async_comp);
>> +}
>> +
>> +void update_max_async_completions(int num)
>> +{
>> +	max_async_comp += num;
>> +}
>> +
>> +void opal_init_msg(void)
>> +{
>> +	max_async_comp = OPAL_MAX_ASYNC_COMP;
>> +	alloc_opal_msg_list(OPAL_MAX_MSGS);
>
> I'm confused, you haven't really changed the code since the num you
> pass to alloc_opal_msg_list() is what was originally in the for loop.
> But you do provide the ability to change max_async_comp. I would expect
> that the msg_free_list list would need to grow accordingly (I could be
> wrong). Ah so I did some looking around _opal_queue_msg() would grow
> the list, perhaps you're correct and we don't want to allocate all of
> msg_free_list at boot? But it seems that we do at the moment... just to
> avoid runtime malloc() which we don't like doing... Stewart?
>
> It isn't a big change update_max_async_completitions() could do
>
> alloc_opal_msg_list(num);
>
> to add more entries to the list?
>
>
> Also this makes the both those defines misleading now, especially at
> skiboot runtime if the occ code changes max_async_comp. Might I suggest
> renaming the defines, I suppose you'd need to turn OPAL_MAX_MSGS into a
> function that sums OPAL_MSG_TYPE_MAX and max_async_comp and then
> OPAL_MAX_ASYNC_COMP could be OPAL_MIN_ASYNC_COMP or
> OPAL_DEFAULT_ASYNC_COMP....

Increasingly I think we should just make it either 64 or 2^32.

(or maybe 64 and then clean up the bits in opal that make it a terrible
idea to set it to 2^32).

There's nothing really fundamentally limiting the number of concurrent
operations that OPAL can do... it's more of a suggestion to the kernel
that if you keep it under this number nothing really bad is going to
happen (like ENOMEM for trying to construct an infinitely long queue of
messages in OPAL).

These days though, we have a pool allocator and returning OPAL_BUSY
isn't he worst thing in the world.

-- 
Stewart Smith
OPAL Architect, IBM.