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

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@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(-)

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@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....

> +}
> diff --git a/core/opal.c b/core/opal.c
> index 8095f73..ebd3937 100644
> --- a/core/opal.c
> +++ b/core/opal.c
> @@ -201,7 +201,6 @@ void add_opal_node(void)
>  	else
>  		dt_add_property_strings(opal_node, "compatible", "ibm,opal-v3");
>  
> -	dt_add_property_cells(opal_node, "opal-msg-async-num", OPAL_MAX_ASYNC_COMP);
>  	dt_add_property_cells(opal_node, "opal-msg-size", sizeof(struct opal_msg));
>  	dt_add_property_u64(opal_node, "opal-base-address", base);
>  	dt_add_property_u64(opal_node, "opal-entry-address", entry);
> diff --git a/include/opal-msg.h b/include/opal-msg.h
> index a75bc4e..982026e 100644
> --- a/include/opal-msg.h
> +++ b/include/opal-msg.h
> @@ -37,5 +37,8 @@ int _opal_queue_msg(enum opal_msg_type msg_type, void *data,
>  			(u64[]) {__VA_ARGS__});
>  
>  void opal_init_msg(void);
> +void alloc_opal_msg_list(int num);
> +void update_max_async_completions(int num);
> +void add_dt_max_async_completions(void);
>  
>  #endif /* __OPALMSG_H */

Cyril Bur <cyrilbur@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@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.