[Skiboot] [PATCH v2 2/2] prd: PRD framework

[Stewart Smith]

Neelesh Gupta <neelegup at linux.vnet.ibm.com> writes:
>>> +	lock(&prd_lock);
>>> +	node = list_pop(&prd_free_list, struct prd_node, link);
>>> +	if (!node) { /* Free list exhausted */
>>> +		node = zalloc(sizeof(*node));
>>> +		if (!node) {
>>> +			prlog(PR_ERR, "Failed to allocate prd node\n");
>>> +			unlock(&prd_lock);
>>> +			return OPAL_NO_MEM;
>>> +		}
>>> +	}
>> Why not just dynamically allocate everything? Why have a free list at
>> all if we're just going to allocate until ENOMEM?
>
> Have sufficiently allocated during init() and added them to the free list to
> cater to multiple messages which should not exhaust the free list and reduce
> fragmentation, but if it does then it is not an error and we dynamically 
> allocate..
> further add and maintain it in the list.
>
>>
>> Also, I see extra allocation but nowhere where these extra ones are freed?
>
> These are not freed, added and maintained as part of the 'free' list.

This is a problem - a flood of these will hit ENOMEM and that won't be
good for keeping the system running at all (many OPAL calls would fail
without being able to allocate memory).


>>> --- a/include/platform.h
>>> +++ b/include/platform.h
>>> @@ -100,6 +100,8 @@ struct platform {
>>>   	 */
>>>   	void		(*external_irq)(unsigned int chip_id);
>>>   
>>> +	void		(*local_irq)(unsigned int chip_id);
>>> +
>>>   	/*
>>>   	 * nvram ops.
>>>   	 *
>> Why is this a platform op? It seems identical for the time being... or
>> is it just missing in rhesus?
>
> Is not the prd interrupts platform dependent? they reach to OPAL thru 
> same channel
> but could be of different nature between the platforms..?

Let's change it to a platform op when/if there's something that needs to
be platform specific.